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
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. regards, tom lane ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
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
+
+ Escaping Identifier for Inclusion in SQL Commands
+
+PQescapeIdentifier
+escaping
strings
+
+
+ PQescapeIdentifier 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. PQescapeIdentifier performs this
+ operation.
+
+
+
+
+ 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 SQL injection attacks wherein unwanted
+ SQL commands are fed to your database.
+
+
+
+
+ Note that it is still necessary to do escaping of identifiers when
+ using functions that support parameterized queries such as
PQexecParams or
+ its sibling routines. Only literal values are automatically escaped
+ using these functions, not identifiers.
+
+
+ size_t PQescapeIdentifier (char *to, const char *from, size_t length);
+
+
+
+
+ The parameter from points to the first character of the string
+ that is to be escaped, and the length parameter gives the
+ number of characters in this string. A terminating zero byte is not
+ required, and should not be counted in length. (If
+ a terminating zero byte is found before length bytes are
+ processed, PQescapeIdentifier stops at the zero; the behavior
+ is thus rather like strncpy.)
+ to shall point to a
+ buffer that is able to hold at least one more character than twice
+ the value of length, otherwise the behavior is
+ undefined. A call to PQescapeIdentifier writes an escaped
+ version of the from string to the to
+ buffer, replacing special characters so that they cannot cause any
+ harm, and adding a terminating zero byte. The double quotes that
+ may surround PostgreSQL identifiers are not
+ included in the result string; they should be provided in the SQL
+ command that the result is inserted into.
+
+
+ PQescapeIdentifier returns the number of characters written
+ to to, not including the terminating zero byte.
+
+
+ Behavior is undefined if the to and from
+ strings overlap.
+
+
Escaping Binary Strings for Inclusion in SQL Commands
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 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 terminating NUL).
+ */
+ size_t
+ PQescapeIdentifier(char *to, const char *from, size_t length)
+ {
+ const char *source = from;
+ char *target = to;
+ size_t remaining = length;
+
+ while (remaining > 0 && *source != '\0')
+ {
+
Re: [PATCHES] [HACKERS] PQescapeIdentifier
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? Well, yes the plan was to accept user-supplied identifiers... 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. Yes, phpPgAdmin sure would. I imagine this would be a nightmare to address properly, so perhaps we should remove the function :( ---(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
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
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: > >> * 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: >> * 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
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
+
+ Escaping Identifier for Inclusion in SQL Commands
+
+PQescapeIdentifier
+escaping strings
+
+
+ PQescapeIdentifier 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. PQescapeIdentifier performs this
+ operation.
+
+
+
+
+ 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 SQL injection attacks wherein unwanted
+ SQL commands are fed to your database.
+
+
+
+
+ Note that it is still necessary to do escaping of identifiers when
+ using functions that support parameterized queries such as PQexecParams or
+ its sibling routines. Only literal values are automatically escaped
+ using these functions, not identifiers.
+
+
+ size_t PQescapeIdentifier (char *to, const char *from, size_t length);
+
+
+
+
+ The parameter from points to the first character of the string
+ that is to be escaped, and the length parameter gives the
+ number of characters in this string. A terminating zero byte is not
+ required, and should not be counted in length. (If
+ a terminating zero byte is found before length bytes are
+ processed, PQescapeIdentifier stops at the zero; the behavior
+ is thus rather like strncpy.)
+ to shall point to a
+ buffer that is able to hold at least one more character than twice
+ the value of length, otherwise the behavior is
+ undefined. A call to PQescapeIdentifier writes an escaped
+ version of the from string to the to
+ buffer, replacing special characters so that they cannot cause any
+ harm, and adding a terminating zero byte. The double quotes that
+ may surround PostgreSQL identifiers are not
+ included in the result string; they should be provided in the SQL
+ command that the result is inserted into.
+
+
+ PQescapeIdentifier returns the number of characters written
+ to to, not including the terminating zero byte.
+
+
+ Behavior is undefined if the to and from
+ strings overlap.
+
+
Escaping Binary Strings for Inclusion in SQL Commands
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 terminating NUL).
+ */
+ size_t
+ PQescapeIdentifier(char *to, const char *from, size_t length)
+ {
+ const char *source = from;
+ char *target = to;
+ size_t remaining = length;
+
+ while (remaining > 0 && *source != '\0')
+ {
+ if (*source == '"')
+ *target++ = *source;
+ *target++ = *source++;
+ remaining--;
+ }
+
+ /* Write the terminating NUL character. */
+ *target = '\0';
+
+ return target - to;
+ }
+
+ /*
* PQescapeBytea - converts from binar
