Обсуждение: Re: [PATCHES] Fw: Isn't pg_statistic a security hole - Solution Proposal
[ -> hackers ] Tom Lane writes: > > Will you expect the function to do dequoting etc. as well? This might get > > out of hand. > > Hm. We already have such code available for nextval(), IMHO, nextval() isn't the greatest interface in the world. I do like the alternative (deprecated?) syntax sequence.nextval() because of the notational resemblence to OO. (We might even be able to turn this into something like an SQL99 "class" feature.) As I understand it, currently relation.function(a, b, c) ends up as being a function call function(relation, a, b, c) where the first argument is "text". This is probably an unnecessary fragility, since the oid of the relation should already be known by that time. So perhaps we could change this that the first argument gets passed in an Oid. Then we'd really only need the Oid version of Joe's has_*_privilege functions. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
Peter Eisentraut <peter_e@gmx.net> writes: > IMHO, nextval() isn't the greatest interface in the world. I do like the > alternative (deprecated?) syntax sequence.nextval() because of the > notational resemblence to OO. Try "nonexistent". I too would like a notation like that, because it would be more transparent to the user w.r.t. case folding and such. But it doesn't exist now. Observe, however, that such a notation would work well only for queries in which the sequence/table name is fixed and known when the query is written. I don't see a way to use it in the case where the name is being computed at runtime (eg, taken from a table column). So it doesn't really solve the problem posed by has_table_privilege. > As I understand it, currently > relation.function(a, b, c) > ends up as being a function call > function(relation, a, b, c) > where the first argument is "text". Sorry, that has nothing to do with reality. What we actually have is an equivalence between the two notationsrel.funcfunc(rel) where the semantics are that an entire tuple of the relation "rel" is passed to the function. This doesn't really gain us anything for the problem at hand (and we'll quite likely have to give it up anyway when we implement schemas, since SQL has very different ideas about what a.b.c means than our current parser does). regards, tom lane
> where the semantics are that an entire tuple of the relation "rel" is > passed to the function. This doesn't really gain us anything for the > problem at hand (and we'll quite likely have to give it up anyway when > we implement schemas, since SQL has very different ideas about what > a.b.c means than our current parser does). > I wasn't quite sure if there are changes I can/should make to has_table_privilege based on this discussion. Is there any action for me on this (other than finishing the regression test and creating documentation patches)? Thanks, -- Joe
"Joe Conway" <joe@conway-family.com> writes: > I wasn't quite sure if there are changes I can/should make to > has_table_privilege based on this discussion. My feeling is that the name-based variants of has_table_privilege should perform downcasing and truncation of the supplied strings before trying to use them as tablename or username; see get_seq_name in backend/commands/sequence.c for a model. (BTW, I only just now added truncation code to that routine, so look at current CVS. Perhaps the routine should be renamed and placed somewhere else, so that sequence.c and has_table_privilege can share it.) Peter's argument seemed to be that there shouldn't be name-based variants at all, with which I do not agree; but perhaps that's not what he meant. regards, tom lane
> My feeling is that the name-based variants of has_table_privilege should > perform downcasing and truncation of the supplied strings before trying > to use them as tablename or username; see get_seq_name in > backend/commands/sequence.c for a model. (BTW, I only just now added > truncation code to that routine, so look at current CVS. Perhaps the > routine should be renamed and placed somewhere else, so that sequence.c > and has_table_privilege can share it.) > Looking at get_seq_name, it does seem like it should be called something like get_object_name (or just get_name?) and moved to a common location. Am I correct in thinking that this function could/should be called by any other function (internal, C, plpgsql, or otherwise) which accepts a text representation of a system object name? What if I rename the get_seq_name function and move it to backend/utils/adt/name.c (and of course change the references to it in sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and truncate. -- Joe
> My feeling is that the name-based variants of has_table_privilege should > perform downcasing and truncation of the supplied strings before trying > to use them as tablename or username; see get_seq_name in > backend/commands/sequence.c for a model. (BTW, I only just now added > truncation code to that routine, so look at current CVS. Perhaps the > routine should be renamed and placed somewhere else, so that sequence.c > and has_table_privilege can share it.) > Looking at get_seq_name, it does seem like it should be called something like get_object_name (or just get_name?) and moved to a common location. Am I correct in thinking that this function could/should be called by any other function (internal, C, plpgsql, or otherwise) which accepts a text representation of a system object name? What if I rename the get_seq_name function and move it to backend/utils/adt/name.c (and of course change the references to it in sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and truncate. -- Joe
> My feeling is that the name-based variants of has_table_privilege should > perform downcasing and truncation of the supplied strings before trying > to use them as tablename or username; see get_seq_name in > backend/commands/sequence.c for a model. (BTW, I only just now added > truncation code to that routine, so look at current CVS. Perhaps the > routine should be renamed and placed somewhere else, so that sequence.c > and has_table_privilege can share it.) > Looking at get_seq_name, it does seem like it should be called something like get_object_name (or just get_name?) and moved to a common location. Am I correct in thinking that this function could/should be called by any other function (internal, C, plpgsql, or otherwise) which accepts a text representation of a system object name? What if I rename the get_seq_name function and move it to backend/utils/adt/name.c (and of course change the references to it in sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and truncate. -- Joe
> representation of a system object name? > > What if I rename the get_seq_name function and move it to > backend/utils/adt/name.c (and of course change the references to it in > sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and > truncate. > > -- Joe Yikes! Sorry about sending that last message 3 times -- I guess that's what I get for using an evil mail client ;-) Joe
Re: Re: [PATCHES] Fw: Isn't pg_statistic a security hole - Solution Proposal
От
Peter Eisentraut
Дата:
Tom Lane writes: > My feeling is that the name-based variants of has_table_privilege should > perform downcasing and truncation of the supplied strings before trying > to use them as tablename or username; see get_seq_name in > backend/commands/sequence.c for a model. I don't like this approach. It's ugly, non-intuitive, and inconvenient. Since these functions will primarily be used in building a sort of information schema and for querying system catalogs, we should use the approach that is or will be used there: character type values contain the table name already case-adjusted. Imagine the pain we would have to go through to *re-quote* the names we get from the system catalogs and information schema components before passing them to this function. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
Peter Eisentraut <peter_e@gmx.net> writes: > Since these functions will primarily be used in building a sort of > information schema and for querying system catalogs, we should use the > approach that is or will be used there: character type values contain the > table name already case-adjusted. Weren't you just arguing that such cases could/should use the OID, not the name at all? ISTM the name-based variants will primarily be used for user-entered names, and in that case the user can reasonably expect that a name will be interpreted the same way as if he'd written it out in a query. The nextval approach is ugly, I'll grant you, but it's also functional. We got complaints about nextval before we put that in; we get lots fewer now. regards, tom lane
Re: Re: [PATCHES] Fw: Isn't pg_statistic a security hole - Solution Proposal
От
Peter Eisentraut
Дата:
Tom Lane writes: > Weren't you just arguing that such cases could/should use the OID, not > the name at all? Yes, but if we're going to have name arguments, we should have sane ones. > ISTM the name-based variants will primarily be used for user-entered > names, and in that case the user can reasonably expect that a name > will be interpreted the same way as if he'd written it out in a query. That would be correct if the user were actually entering the name in the same way, i.e., unquoted or double-quoted. > The nextval approach is ugly, I'll grant you, but it's also functional. But it's incompatible with the SQL conventions. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
> > ISTM the name-based variants will primarily be used for user-entered > > names, and in that case the user can reasonably expect that a name > > will be interpreted the same way as if he'd written it out in a query. > > That would be correct if the user were actually entering the name in the > same way, i.e., unquoted or double-quoted. > > > The nextval approach is ugly, I'll grant you, but it's also functional. > > But it's incompatible with the SQL conventions. > Is the concern that the name-based variants of the function should be called like: select has_table_privilege(current_user, pg_class, 'insert'); or select has_table_privilege(current_user, "My QuotedRelname", 'insert'); instead of select has_table_privilege(current_user, 'pg_class', 'insert'); or select has_table_privilege(current_user, '"My QuotedRelname"', 'insert'); ? If so, what would be involved in fixing it? From an end user's perspective, I wouldn't mind the latter syntax, although the former is clearly more intuitive. But I'd rather have the second form than nothing (just MHO). -- Joe
"Joe Conway" <joe@conway-family.com> writes: > Is the concern that the name-based variants of the function should be called > like: > select has_table_privilege(current_user, pg_class, 'insert'); > or > select has_table_privilege(current_user, "My Quoted Relname", 'insert'); It'd be really nice to do that, but I don't see any reasonable way to do it. The problem is that the unquoted relation name will probably be resolved (to the wrong thing) before we discover that the function wants it to be resolved as a relation OID. Remember that the arguments of a function have to be resolved before we can even start to look up the function, since function lookup depends on the types of the arguments. I have just thought of a possible compromise. Peter is right that we don't want case conversion on table names that are extracted from catalogs. But I think we do want it on table names expressed as string literals. Could we make the assumption that table names in catalogs will be of type 'name'? If so, it'd work to make two versions of the has_table_privilege function, one taking type "name" and the other taking type "text". The "name" version would take its input as-is, the "text" version would do case folding and truncation. This would work transparently for queries selecting relation names from the system catalogs, and it'd also work transparently for queries using unmarked string literals (which will be preferentially resolved as type "text"). Worst case if the system makes the wrong choice is you throw in an explicit coercion to name or text. Comments? regards, tom lane
> I have just thought of a possible compromise. Peter is right that we > don't want case conversion on table names that are extracted from > catalogs. But I think we do want it on table names expressed as string > literals. Could we make the assumption that table names in catalogs > will be of type 'name'? If so, it'd work to make two versions of the > has_table_privilege function, one taking type "name" and the other > taking type "text". The "name" version would take its input as-is, > the "text" version would do case folding and truncation. This would > work transparently for queries selecting relation names from the system > catalogs, and it'd also work transparently for queries using unmarked > string literals (which will be preferentially resolved as type "text"). > Worst case if the system makes the wrong choice is you throw in an > explicit coercion to name or text. Comments? Seems you are adding a distinction between name and text that we never had before. Is it worth it to fix this case? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> My feeling is that the name-based variants of has_table_privilege should > perform downcasing and truncation of the supplied strings before trying > to use them as tablename or username; see get_seq_name in > backend/commands/sequence.c for a model. (BTW, I only just now added > truncation code to that routine, so look at current CVS. Perhaps the > routine should be renamed and placed somewhere else, so that sequence.c > and has_table_privilege can share it.) > Looking at get_seq_name, it does seem like it should be called something like get_object_name (or just get_name?) and moved to a common location. Am I correct in thinking that this function could/should be called by any other function (internal, C, plpgsql, or otherwise) which accepts a text representation of a system object name? What if I rename the get_seq_name function and move it to backend/utils/adt/name.c (and of course change the references to it in sequence.c)? Actually, now I'm wondering why nameout doesn't downcase and truncate. -- Joe
Re: Re: [PATCHES] Fw: Isn't pg_statistic a security hole - Solution Proposal
От
Peter Eisentraut
Дата:
Tom Lane writes: > Could we make the assumption that table names in catalogs > will be of type 'name'? I wouldn't want to guarantee it for the information schema. > If so, it'd work to make two versions of the has_table_privilege > function, one taking type "name" and the other taking type "text". > The "name" version would take its input as-is, the "text" version > would do case folding and truncation. Target typing is ugly. I've tried to look up the supposed \paraphrase{we had enough problems before we added the existing behaviour to setval, etc.} discussion but couldn't find it. My experience on the mailing list is that it goes the other way. The identifier quoting rules are already surprising enough for the uninitiated, but it's even more surprising that they even apply when syntactically no identifier is present. Between the behaviour of "what you see is what you get" and "this language is kind of confusing so you have to quote your strings twice with two different quoting characters" the choice is obvious to me. I'm also arguing for consistency with the standard. According to that, users will be able to do SELECT * FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = 'SoMeThInG'; and a load of similar queries. You can't change the case folding rules here unless you really want to go out of your way, and then you have really confused the heck out of users. We could make relname.func() work without breaking compatibility, ISTM, and then we only need the Oid version. For computing the relation name at execution time, the "plain" version is going to be more useful anyway. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
> Tom Lane writes: > > > Could we make the assumption that table names in catalogs > > will be of type 'name'? > > I wouldn't want to guarantee it for the information schema. > > > If so, it'd work to make two versions of the has_table_privilege > > function, one taking type "name" and the other taking type "text". > > The "name" version would take its input as-is, the "text" version > > would do case folding and truncation. > > Target typing is ugly. > > I've tried to look up the supposed \paraphrase{we had enough problems > before we added the existing behaviour to setval, etc.} discussion but > couldn't find it. My experience on the mailing list is that it goes the > other way. I am confused. What are you suggesting as far as having a name and text version of the functions? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Peter Eisentraut <peter_e@gmx.net> writes: >> Could we make the assumption that table names in catalogs >> will be of type 'name'? > I wouldn't want to guarantee it for the information schema. Your objections are not without merit, and in the interest of bringing this thing to closure I'll concede for now. I want to get on with this so that I can wrap up the pg_statistic view that started the whole thread. What I suggest we do is apply the portions of Joe's latest patch that support has_table_privilege with OID inputs and with NAME inputs, omitting the combinations that take TEXT inputs and do casefolding. We can add that part later if it proves that people do indeed want it. I have specific reasons for wanting to keep the functions accepting NAME rather than TEXT: that will save a run-time type conversion in the common case where one is reading the input from a system catalog, and it will at least provide automatic truncation of overlength names when one is accepting a literal. (I trust Peter won't object to that ;-).) We will probably have to revisit this territory when we implement schemas: there will need to be a way to input qualified table names like foo.bar, and a way to input NON qualified names like "foo.bar". But we can cross that bridge when we come to it. Comments, objections? regards, tom lane
Re: Re: [PATCHES] Fw: Isn't pg_statistic a security hole - Solution Proposal
От
Peter Eisentraut
Дата:
Tom Lane writes: > What I suggest we do is apply the portions of Joe's latest patch that > support has_table_privilege with OID inputs and with NAME inputs, > omitting the combinations that take TEXT inputs and do casefolding. > We can add that part later if it proves that people do indeed want it. Okay. > We will probably have to revisit this territory when we implement > schemas: there will need to be a way to input qualified table names > like foo.bar, and a way to input NON qualified names like "foo.bar". > But we can cross that bridge when we come to it. I figured we would add another argument to the function. -- Peter Eisentraut peter_e@gmx.net http://funkturm.homeip.net/~peter
> What I suggest we do is apply the portions of Joe's latest patch that > support has_table_privilege with OID inputs and with NAME inputs, > omitting the combinations that take TEXT inputs and do casefolding. > We can add that part later if it proves that people do indeed want it. > > I have specific reasons for wanting to keep the functions accepting > NAME rather than TEXT: that will save a run-time type conversion in the > common case where one is reading the input from a system catalog, and > it will at least provide automatic truncation of overlength names when > one is accepting a literal. (I trust Peter won't object to that ;-).) > I'll rework the patch per the above and resend. Thanks, -- Joe
"Joe Conway" <joseph.conway@home.com> writes: > I'll rework the patch per the above and resend. Too late ;-). I just finished ripping out the unneeded parts and applying. I made a few minor changes too, mostly removing unnecessary code (you don't need to call nameout, everyone else just uses NameStr) and trying to line up stylistically with other code. One actual bug noted: you were doing this in a couple of places: + tuple = SearchSysCache(RELOID, ObjectIdGetDatum(reloid), 0, 0, 0); + if (!HeapTupleIsValid(tuple)) { + elog(ERROR, "has_table_privilege: invalid relation oid %d", (int) reloid); + } + + relname = NameStr(((Form_pg_class) GETSTRUCT(tuple))->relname); + + ReleaseSysCache(tuple); Since relname is just a pointer into the tuple, expecting it to still be valid after you release the syscache entry is not kosher. There are several ways to deal with this, but what I actually did was to make use of lsyscache.c's get_rel_name, which pstrdup()s its result to avoid this trap. regards, tom lane
> > Too late ;-). I just finished ripping out the unneeded parts and > applying. Thanks! I take it I still need to do the documentation though ;) > > I made a few minor changes too, mostly removing unnecessary code > (you don't need to call nameout, everyone else just uses NameStr) > and trying to line up stylistically with other code. One actual > bug noted: you were doing this in a couple of places: > Once again, thanks for the "important safety tips". I saw references to this trap in the comments, and therefore should have known better. I guess only practice makes perfect (hopefully!). -- Joe
"Joe Conway" <joseph.conway@home.com> writes: >> Too late ;-). I just finished ripping out the unneeded parts and >> applying. > Thanks! I take it I still need to do the documentation though ;) I put in a few words in func.sgml, but feel free to improve on it. regards, tom lane