Re: tsearch_core patch: permissions and security issues

You bring up a very good point. There are fifteen new commands being
added for full text indexing:

alter-fulltext-config.sgml alter-fulltext-owner.sgml
create-fulltext-dict.sgml drop-fulltext-dict.sgml
alter-fulltext-dict.sgml alter-fulltext-parser.sgml
create-fulltext-map.sgml drop-fulltext-map.sgml
alter-fulltext-dictset.sgml comment-fulltext.sgml
create-fulltext-parser.sgml drop-fulltext-parser.sgml
alter-fulltext-map.sgml create-fulltext-config.sgml
drop-fulltext-config.sgml

I think encoding is a good example to follow. We allow users to create
new conversions (CREATE CONVERSION), but we don't allow them to create
new encodings --- those are hard-coded in the backend. Which of the
following full text objects:

config
dict
map
dictset
parser

can we hard-code into the backend, and just update for every major
release like we do for encodings?

---------------------------------------------------------------------------

Tom Lane wrote:
> I've been looking at the tsearch patch a bit, and I think there needs to
> be more thought given to the permissions required to mess around with
> tsearch configuration objects.
>
> The TSParser objects reference functions declared to take and return
> INTERNAL arguments. This means that the underlying functions must be
> coded in C and can only be installed by a superuser, which in turn means
> that there is no scenario where it is really useful for a non-superuser
> to execute CREATE PARSER. What's more, allowing a non-superuser to do
> it creates security holes: if you can find an unrelated function taking
> the right number of INTERNAL arguments, you can install it as a TSParser
> support function. That trivially allows crashing the backend, and it
> could allow worse security holes than that.
>
> TSDictionary objects have exactly the same issues since they also depend
> on functions with INTERNAL arguments.
>
> At minimum this means that we should restrict CREATE/DROP/ALTER commands
> for these objects to superusers. (Which in turn means there's no point
> in tracking an ownership column for them; every superuser is the same as
> every other one, permissions-wise.) I'm wondering though whether this
> doesn't mean that we don't need manipulation commands for them at all.
> Is it likely that people will be adding parser or dictionary support to
> an installation on the fly? Maybe we can just create 'em all at initdb
> time and be done, similar to the way index access methods are treated.
> This doesn't say that it's not possible to add more; you can add an
> index access method on the fly too, if you want, by inserting stuff into
> pg_am by hand. I'm just wondering whether all that SQL-statement
> support and pg_dump support for custom parsers and dictionaries is
> really worth the code space and future maintenance effort it'll eat up.
>
> You could remove the immediate source of this objection if you could
> redesign the APIs for the underlying support functions to be more
> type-safe. I'm not sure how feasible or useful that would be though.
> The bottom-line question here is whether developing a new parser or
> dictionary implementation is really something that ordinary users might
> do. If not, then having all this SQL-level support for setting up
> catalog entries seems like wasted effort.
>
> TSConfiguration objects are a different story, since they have only
> type-safe dependencies on parsers, locales, and dictionaries. But they
> still need some more thought about permissions, because AFAICS mucking
> with a configuration can invalidate some other user's data. Do we want
> to allow runtime changes in a configuration that existing tsvector
> columns already depend on? How can we even recognize whether there is
> stored data that will be affected by a configuration change? (AFAICS
> the patch doesn't put anything into the pg_depend machinery that could
> deal with this.) And who gets to decide which configuration is default,
> anyway?
>
> I'm also a bit disturbed that you've made searches for TSConfiguration
> objects be search-path-sensitive. That is likely to create problems
> similar to those we've recently recognized for function lookup, eg,
> an insertion into a full-text-indexed column gets treated differently
> depending on the caller's search path. It's particularly bad to have
> the default object be search-path-dependent. We learned the hard way
> not to do that for default index operator classes; let's not make the
> same mistake again for tsearch configurations.
>
> Next, it took me a while to understand how Mapping objects fit into
> the scheme at all, and now that (I think) I understand, I'm wondering
> why treat them as an independent concept. Seems like the mapping from
> token types to dictionaries is really a property of a configuration,
> and we ought to be handling it through options of CREATE/ALTER
> CONFIGURATION commands, not as an apparently independent object type.
> The way the patch is doing it feels like implementing CREATE ATTRIBUTE
> as a separate command instead of having ALTER TABLE ADD COLUMN; it's
> just weird, and it's not obvious that dropping a configuration should
> make the associated mapping object go away.
>
> Lastly, I'm unhappy that the patch still keeps a lot of configuration
> information, such as stop word lists, in the filesystem rather than the
> database. It seems to me that the single easiest and most useful part
> of a configuration to change is the stop word list; but this setup
> guarantees that no one but a DBA can do that, and what's more that
> pg_dump won't record your changes. What's the point of having any
> non-superuser configuration capability at all, if stop words aren't part
> of what you can change?
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +