Thread: CREATE CAST code review
I looked through your CREATE CAST commit a little. Looks pretty good
but I had a few suggestions/concerns.
* The biggie is that I'm not satisfied with the permissions checking.
You have "To be able to create a cast, you must own the underlying
function. To be able to create a binary compatible cast, you must own
both the source and the target data type." (Same to drop a cast.)
It seems to me that this is quite dangerous, as any random luser who
can access both datatypes can create a function and then a cast,
thereby changing the behavior of *both* types in rather subtle ways,
and perhaps insinuating unexpected, untrusted code into queries issued
by people who weren't expecting any cast to be applied.
(Not to mention causing a denial-of-service for the legitimate definer
of that cast, if he hadn't gotten around to making it quite yet.)
The problem would be slightly less bad if function definition required
USAGE privilege on the arg/result types ... but not much.
I think I would prefer this definition: to create/drop a cast, you must
own both datatypes, plus the underlying function if any. What's the
rationale for having such a weak permissions scheme?
* I see that you are worried in pg_dump about which schema to associate
a cast with, if it's binary-compatible. I'm confused too; would it work
to dump the cast iff either of the source/dest datatypes are to be
dumped? (This might be a better rule even in the not-binary-compatible
case.)
* Various more-or-less minor coding gripes:
* pg_cast table not documented in catalogs.sgml.
* shoddy implementation of getObjectDescription() for cast. (I have
some to-do items in that routine myself, though, and will be happy to fix
this while at it.)
* since pg_cast depends on having a unique OID, it *must* have an OID
index to enforce that uniqueness; otherwise the system can fail after
OID wraparound.
* since you must define the OID index anyway, you may as well use it in
a systable scan in DropCastById, instead of using heapscan.
* in CreateCast, there's no need to use the relatively expensive
get_system_catalog_relid() lookup; you've got pg_cast open and so
you can just do RelationGetRelid on it.
regards, tom lane
PS: I also want to raise a flag that we still haven't resolved the
issues we discussed a few months ago, about exactly *which* implicit
casts should be provided. I think that's a different thread though.
Tom Lane writes: > I looked through your CREATE CAST commit a little. Looks pretty good > but I had a few suggestions/concerns. > > * The biggie is that I'm not satisfied with the permissions checking. Me neither. I had sent a message earlier about this but it went unnoticed, but I had to implement something that was a little more restrictive that the previous first come first serve scheme. > I think I would prefer this definition: to create/drop a cast, you must > own both datatypes, plus the underlying function if any. What's the > rationale for having such a weak permissions scheme? That doesn't quite work, because then no ordinary user can define a cast from some built-in type to his own type. What I'm thinking about is to implement the USAGE privilege on types, and then you need to have that to be allowed to create casts. (Possibly there should be an even more restrictive privilege invented here, but that would be a second step.) > * I see that you are worried in pg_dump about which schema to associate > a cast with, if it's binary-compatible. I'm confused too; would it work > to dump the cast iff either of the source/dest datatypes are to be > dumped? (This might be a better rule even in the not-binary-compatible > case.) I'm not sure about the implications of associating objects with schemas in pg_dump. I suppose there might be an option to dump only certain schemas, in which case it's tricky to associate a cast to any one schema. > PS: I also want to raise a flag that we still haven't resolved the > issues we discussed a few months ago, about exactly *which* implicit > casts should be provided. I think that's a different thread though. Yes, I have some thoughts on that which I plan to present soon. -- Peter Eisentraut peter_e@gmx.net
Peter Eisentraut <peter_e@gmx.net> writes:
> That doesn't quite work, because then no ordinary user can define a cast
> from some built-in type to his own type. What I'm thinking about is to
> implement the USAGE privilege on types, and then you need to have that to
> be allowed to create casts.
Still seems too weak. What about requiring ownership of at least one
of the types?
> I'm not sure about the implications of associating objects with schemas in
> pg_dump. I suppose there might be an option to dump only certain schemas,
That is the intention (it's not implemented yet).
regards, tom lane
Tom wrote: > Peter Eisentraut <peter_e@gmx.net> writes: > > That doesn't quite work, because then no ordinary user can define a cast > > from some built-in type to his own type. What I'm thinking about is to > > implement the USAGE privilege on types, and then you need to have that to > > be allowed to create casts. > > Still seems too weak. Yes. > What about requiring ownership of at least one > of the types? I was thinking that too, but, would it be possible to circumvent such a restriction with a "type in the middle" attack ? Create your own type and then 1. (auto)cast type1 to own type 2. (auto)cast own type to type2 ? Andreas
Tom Lane writes: > What about requiring ownership of at least one of the types? Yes, that would work. There would be a somewhat bizzare consequence, though: User U1 creates type T1, user U2 creates type T2. Then user U1 creates a cast from T1 to T2. Now user U2 would be allowed to drop that cast (unless we store a cast owner). I guess that lies in the nature of things. A much more complex yet powerful alternative would be to associate casts with schemas. For example, this would allow an ordinary user to create a cast from numeric to text in his own little world. But that might be going too far. > > I'm not sure about the implications of associating objects with schemas in > > pg_dump. I suppose there might be an option to dump only certain schemas, > > That is the intention (it's not implemented yet). My concern was that if you, say, have two schemas and a cast that involves types from both schemas. If you dump all of them, you have a consistent dump. But if you dump both schemas separately, do you dump the cast in both of them (thus making each schema's dump self-contained) or in only one of them (thus allowing concatenation the dumps). This issue generalizes to every kind of dependency in pg_dump. -- Peter Eisentraut peter_e@gmx.net
Zeugswetter Andreas SB SD writes: > > What about requiring ownership of at least one > > of the types? > > I was thinking that too, but, would it be possible to circumvent such > a restriction with a "type in the middle" attack ? > Create your own type and then > 1. (auto)cast type1 to own type > 2. (auto)cast own type to type2 ? But that doesn't affect casts between type1 and type2. PostgreSQL doesn't do indirect casts. -- Peter Eisentraut peter_e@gmx.net