Re: [PATCHES] [HACKERS] PQescapeIdentifier
Tom Lane wrote: Christopher Kings-Lynne [EMAIL PROTECTED] writes: Hang on a second. Has someone considered the encoding issues this might suffer from, same as PQescapeString? That was the point I raised when I saw the commit. My advice is we shouldn't have PQescapeIdentifier at all. PQescapeIdentifierConn would be the thing to define, parallel to PQescapeStringConn. Patch reverted, TODO updated to: o Add PQescapeIdentifierConn() -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] PQescapeIdentifier
Hang on a second. Has someone considered the encoding issues this might suffer from, same as PQescapeString? I remember we discussed it briefly and I mentioned it's outta my league to prove one way or the other... Bruce Momjian wrote: Christopher Kings-Lynne wrote: TODO item done for 8.2: * Add PQescapeIdentifier() to libpq Someone probably needs to check this :) Updated patch applied. Thanks. Index: doc/src/sgml/libpq.sgml === RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.211 diff -c -c -r1.211 libpq.sgml *** doc/src/sgml/libpq.sgml 23 May 2006 22:13:19 - 1.211 --- doc/src/sgml/libpq.sgml 26 Jun 2006 23:54:12 - *** *** 2279,2284 --- 2279,2347 /para /sect2 + sect2 id=libpq-exec-escape-identifier + titleEscaping Identifier for Inclusion in SQL Commands/title + +indexterm zone=libpq-exec-escape-identifierprimaryPQescapeIdentifier// +indexterm zone=libpq-exec-escape-identifierprimaryescaping strings// + + para + functionPQescapeIdentifier/function escapes a string for use + as an identifier name within an SQL command. For example; table names, + column names, view names and user names are all identifiers. + Double quotes () must be escaped to prevent them from being interpreted + specially by the SQL parser. functionPQescapeIdentifier/ performs this + operation. + /para + + tip + para + It is especially important to do proper escaping when handling strings that + were received from an untrustworthy source. Otherwise there is a security + risk: you are vulnerable to quoteSQL injection/ attacks wherein unwanted + SQL commands are fed to your database. + /para + /tip + + para + Note that it is still necessary to do escaping of identifiers when + using functions that support parameterized queries such as functionPQexecParams/ or + its sibling routines. Only literal values are automatically escaped + using these functions, not identifiers. + + synopsis + size_t PQescapeIdentifier (char *to, const char *from, size_t length); + /synopsis + /para + + para + The parameter parameterfrom/ points to the first character of the string + that is to be escaped, and the parameterlength/ parameter gives the + number of characters in this string. A terminating zero byte is not + required, and should not be counted in parameterlength/. (If + a terminating zero byte is found before parameterlength/ bytes are + processed, functionPQescapeIdentifier/ stops at the zero; the behavior + is thus rather like functionstrncpy/.) + parameterto/ shall point to a + buffer that is able to hold at least one more character than twice + the value of parameterlength/, otherwise the behavior is + undefined. A call to functionPQescapeIdentifier/ writes an escaped + version of the parameterfrom/ string to the parameterto/ + buffer, replacing special characters so that they cannot cause any + harm, and adding a terminating zero byte. The double quotes that + may surround productnamePostgreSQL/ identifiers are not + included in the result string; they should be provided in the SQL + command that the result is inserted into. + /para + para + functionPQescapeIdentifier/ returns the number of characters written + to parameterto/, not including the terminating zero byte. + /para + para + Behavior is undefined if the parameterto/ and parameterfrom/ + strings overlap. + /para + /sect2 sect2 id=libpq-exec-escape-bytea titleEscaping Binary Strings for Inclusion in SQL Commands/title Index: src/interfaces/libpq/exports.txt === RCS file: /cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.11 diff -c -c -r1.11 exports.txt *** src/interfaces/libpq/exports.txt28 May 2006 22:42:05 - 1.11 --- src/interfaces/libpq/exports.txt26 Jun 2006 23:54:20 - *** *** 130,132 --- 130,134 PQencryptPassword 128 PQisthreadsafe129 enlargePQExpBuffer130 + PQescapeIdentifier131 + Index: src/interfaces/libpq/fe-exec.c === RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v retrieving revision 1.186 diff -c -c -r1.186 fe-exec.c *** src/interfaces/libpq/fe-exec.c 28 May 2006 21:13:54 - 1.186 --- src/interfaces/libpq/fe-exec.c 26 Jun 2006 23:54:21 - *** *** 2516,2521 --- 2516,2557 } /* + * Escaping arbitrary strings to get valid SQL identifier strings. + * + * Replaces with . + * + * length is the length of the source string. (Note: if a terminating NUL + * is encountered sooner, PQescapeIdentifier stops short of length; the behavior + * is thus rather like strncpy.) + * + * For safety the buffer at to must be at
Re: [PATCHES] [HACKERS] PQescapeIdentifier
Christopher Kings-Lynne wrote: TODO item done for 8.2: * Add PQescapeIdentifier() to libpq Someone probably needs to check this :) Updated patch applied. Thanks. -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + Index: doc/src/sgml/libpq.sgml === RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v retrieving revision 1.211 diff -c -c -r1.211 libpq.sgml *** doc/src/sgml/libpq.sgml 23 May 2006 22:13:19 - 1.211 --- doc/src/sgml/libpq.sgml 26 Jun 2006 23:54:12 - *** *** 2279,2284 --- 2279,2347 /para /sect2 + sect2 id=libpq-exec-escape-identifier + titleEscaping Identifier for Inclusion in SQL Commands/title + +indexterm zone=libpq-exec-escape-identifierprimaryPQescapeIdentifier// +indexterm zone=libpq-exec-escape-identifierprimaryescaping strings// + + para + functionPQescapeIdentifier/function escapes a string for use + as an identifier name within an SQL command. For example; table names, + column names, view names and user names are all identifiers. + Double quotes () must be escaped to prevent them from being interpreted + specially by the SQL parser. functionPQescapeIdentifier/ performs this + operation. + /para + + tip + para + It is especially important to do proper escaping when handling strings that + were received from an untrustworthy source. Otherwise there is a security + risk: you are vulnerable to quoteSQL injection/ attacks wherein unwanted + SQL commands are fed to your database. + /para + /tip + + para + Note that it is still necessary to do escaping of identifiers when + using functions that support parameterized queries such as functionPQexecParams/ or + its sibling routines. Only literal values are automatically escaped + using these functions, not identifiers. + + synopsis + size_t PQescapeIdentifier (char *to, const char *from, size_t length); + /synopsis + /para + + para + The parameter parameterfrom/ points to the first character of the string + that is to be escaped, and the parameterlength/ parameter gives the + number of characters in this string. A terminating zero byte is not + required, and should not be counted in parameterlength/. (If + a terminating zero byte is found before parameterlength/ bytes are + processed, functionPQescapeIdentifier/ stops at the zero; the behavior + is thus rather like functionstrncpy/.) + parameterto/ shall point to a + buffer that is able to hold at least one more character than twice + the value of parameterlength/, otherwise the behavior is + undefined. A call to functionPQescapeIdentifier/ writes an escaped + version of the parameterfrom/ string to the parameterto/ + buffer, replacing special characters so that they cannot cause any + harm, and adding a terminating zero byte. The double quotes that + may surround productnamePostgreSQL/ identifiers are not + included in the result string; they should be provided in the SQL + command that the result is inserted into. + /para + para + functionPQescapeIdentifier/ returns the number of characters written + to parameterto/, not including the terminating zero byte. + /para + para + Behavior is undefined if the parameterto/ and parameterfrom/ + strings overlap. + /para + /sect2 sect2 id=libpq-exec-escape-bytea titleEscaping Binary Strings for Inclusion in SQL Commands/title Index: src/interfaces/libpq/exports.txt === RCS file: /cvsroot/pgsql/src/interfaces/libpq/exports.txt,v retrieving revision 1.11 diff -c -c -r1.11 exports.txt *** src/interfaces/libpq/exports.txt 28 May 2006 22:42:05 - 1.11 --- src/interfaces/libpq/exports.txt 26 Jun 2006 23:54:20 - *** *** 130,132 --- 130,134 PQencryptPassword 128 PQisthreadsafe129 enlargePQExpBuffer130 + PQescapeIdentifier131 + Index: src/interfaces/libpq/fe-exec.c === RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v retrieving revision 1.186 diff -c -c -r1.186 fe-exec.c *** src/interfaces/libpq/fe-exec.c 28 May 2006 21:13:54 - 1.186 --- src/interfaces/libpq/fe-exec.c 26 Jun 2006 23:54:21 - *** *** 2516,2521 --- 2516,2557 } /* + * Escaping arbitrary strings to get valid SQL identifier strings. + * + * Replaces with . + * + * length is the length of the source string. (Note: if a terminating NUL + * is encountered sooner, PQescapeIdentifier stops short of length; the behavior + * is thus rather like strncpy.) + * + * For safety the buffer at to must be at least 2*length + 1 bytes long. + * A terminating NUL character is added to the output string, whether the + * input is NUL-terminated or not. + * + * Returns the actual length of the output (not counting the
Re: [PATCHES] [HACKERS] PQescapeIdentifier
Bruce Momjian [EMAIL PROTECTED] writes: * Add PQescapeIdentifier() to libpq Updated patch applied. Thanks. Have either of you inquired into the encoding-safety of this code? It certainly looks like no consideration was given for that. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] PQescapeIdentifier
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: * Add PQescapeIdentifier() to libpq Updated patch applied. Thanks. Have either of you inquired into the encoding-safety of this code? It certainly looks like no consideration was given for that. I thought of that but I assume we were not accepting user-supplied identifiers for this --- that this was only for application use. Am I wrong? -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] PQescapeIdentifier
Bruce Momjian [EMAIL PROTECTED] writes: Tom Lane wrote: Have either of you inquired into the encoding-safety of this code? It certainly looks like no consideration was given for that. I thought of that but I assume we were not accepting user-supplied identifiers for this --- that this was only for application use. Am I wrong? By definition, an escaping routine is not supposed to trust the data it is handed. We *will* be seeing a CVE report if this function has got any escaping vulnerability. If you insist on a practical example, I can certainly imagine someone thinking it'd be cool to allow searches on a user-selected column, and implementing that by passing the user-given column name straight into the query with only PQescapeIdentifier for safety. regards, tom lane ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] PQescapeIdentifier
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Tom Lane wrote: Have either of you inquired into the encoding-safety of this code? It certainly looks like no consideration was given for that. I thought of that but I assume we were not accepting user-supplied identifiers for this --- that this was only for application use. Am I wrong? By definition, an escaping routine is not supposed to trust the data it is handed. We *will* be seeing a CVE report if this function has got any escaping vulnerability. If you insist on a practical example, I can certainly imagine someone thinking it'd be cool to allow searches on a user-selected column, and implementing that by passing the user-given column name straight into the query with only PQescapeIdentifier for safety. OK, does someone want to fix it, or should I revert it? -- Bruce Momjian [EMAIL PROTECTED] EnterpriseDBhttp://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] [HACKERS] PQescapeIdentifier
Christopher Kings-Lynne [EMAIL PROTECTED] writes: Yes, phpPgAdmin sure would. I imagine this would be a nightmare to address properly, so perhaps we should remove the function :( Well, it's fixable, cf PQescapeStringConn, but we should fix it *before* it gets into the field not after. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
