Re: [HACKERS] possible design bug with PQescapeString()
FYI I have sent an email to cores to ask if I am OK to bring another but closely related to this issue to open discussions, whose details have already been sent to them. The reason why I'm asking is, if this issue could be open, then the issue might be open too and that makes discussions easier. At this point, I get no response from them so far. -- Tatsuo Ishii SRA OSS, Inc. Japan Tatsuo Ishii [EMAIL PROTECTED] writes: I guess I understand whay you are saying. However, I am not allowed to talk to you about it unless cores allow me. Probably we need some closed forum to discuss this kind of security issues. Considering that you've already described the problem on pgsql-hackers, I hardly see how further discussion is going to create a bigger security breach than already exists. (I'm of the opinion that the problem is mostly a client problem anyway; AFAICS the issue only comes up if client software fails to consider encoding issues while doing escaping. There is certainly no way that we can magically solve the problem in a new PG release, and so trying to keep it quiet until we can work out a solution seems pointless.) regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend ---(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: [HACKERS] possible design bug with PQescapeString()
On 2006-02-20, Tatsuo Ishii [EMAIL PROTECTED] wrote: In further investigation, Akio Ishida found this kind of attack is possible even with EUC_JP/UTF-8. How? The details have been sent to cores. -- Tatsuo Ishii SRA OSS, Inc. Japan ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] possible design bug with PQescapeString()
But actually I'd argue that
letting the client programmer supply the encoding is still a pretty
dangerous practice. Your example demonstrates that if the encoding
PQescapeString is told is different from the encoding the backend parser
thinks is in use, problems result. Perhaps we should pass the PGconn
to new-PQescapeString and let it dig the client encoding out of that.
Sound good to pass PGconn to new-PQescapeString. Here is the proposed
calling sequence for the new function:
size_t PQescapeStringWithConn (const PGconn *conn, char *to, const char
*from, size_t length)
If this is ok, I will implement for 8.2.
Here is the promised patches for 8.2. If there's no objection, I will
commit tomorrow.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
cvs diff: Diffing src/interfaces/libpq
Index: src/interfaces/libpq/fe-exec.c
===
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.179
diff -c -r1.179 fe-exec.c
*** src/interfaces/libpq/fe-exec.c 25 Jan 2006 20:44:32 - 1.179
--- src/interfaces/libpq/fe-exec.c 26 Feb 2006 14:16:30 -
***
*** 2373,2378
--- 2373,2435
}
/*
+ * Escaping arbitrary strings to get valid SQL literal strings.
+ * mostly same as PQescapeString() except that this function is
+ * multibyte aware. The encoding info is retrieved from conn. So you
+ * should set proper client encoding before using this.
+ *
+ * Replaces \\ with and ' with ''.
+ *
+ * length is the length of the source string. (Note: if a terminating NUL
+ * is encountered sooner, PQescapeString 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
+ PQescapeStringWithConn(PGconn *conn, char *to, const char *from, size_t
length)
+ {
+ const char *source = from;
+ char *target = to;
+ size_t remaining = length;
+ int len;
+
+ if (!conn)
+ {
+ *target = '\0';
+ return 0;
+ }
+
+ while (remaining 0 *source != '\0')
+ {
+ if (SQL_STR_DOUBLE(*source))
+ {
+ *target++ = *source;
+ *target++ = *source++;
+ }
+ else
+ {
+ len = PQmblen(source, conn-client_encoding);
+ while (*source != '\0' remaining 0 len 0)
+ {
+ *target++ = *source++;
+ remaining--;
+ len--;
+ }
+ }
+ }
+
+ /* Write the terminating NUL character. */
+ *target = '\0';
+
+ return target - to;
+ }
+
+ /*
*PQescapeBytea - converts from binary string to the
*minimal encoding necessary to include the string in an SQL
*INSERT statement with a bytea type column as the target.
Index: src/interfaces/libpq/libpq-fe.h
===
RCS file: /cvsroot/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.124
diff -c -r1.124 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 26 Dec 2005 14:58:06 - 1.124
--- src/interfaces/libpq/libpq-fe.h 26 Feb 2006 14:16:30 -
***
*** 436,441
--- 436,442
/* Quoting strings before inclusion in queries. */
extern size_t PQescapeString(char *to, const char *from, size_t length);
+ extern size_t PQescapeStringWithConn(PGconn *conn, char *to, const char
*from, size_t length);
extern unsigned char *PQescapeBytea(const unsigned char *bintext, size_t
binlen,
size_t *bytealen);
extern unsigned char *PQunescapeBytea(const unsigned char *strtext,
cvs diff: Diffing src/interfaces/libpq/po
cvs diff: Diffing doc/src/sgml
Index: doc/src/sgml/libpq.sgml
===
RCS file: /cvsroot/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.201
diff -c -r1.201 libpq.sgml
*** doc/src/sgml/libpq.sgml 26 Dec 2005 14:58:04 - 1.201
--- doc/src/sgml/libpq.sgml 26 Feb 2006 14:16:34 -
***
*** 2253,2258
--- 2253,2323
/para
/sect2
+ sect2 id=libpq-exec-escape-string-with-conn
+ titleEscaping Strings for Inclusion in SQL Commands/title
+
+indexterm
zone=libpq-exec-escape-string-with-connprimaryPQescapeStringWithConn//
+indexterm zone=libpq-exec-escape-string-with-connprimaryescaping
strings//
+
+ para
+ functionPQescapeStringWithConn/function escapes a string
Re: [HACKERS] possible design bug with PQescapeString()
But actually I'd argue that letting the client programmer supply the encoding is still a pretty dangerous practice. Your example demonstrates that if the encoding PQescapeString is told is different from the encoding the backend parser thinks is in use, problems result. Perhaps we should pass the PGconn to new-PQescapeString and let it dig the client encoding out of that. Sound good to pass PGconn to new-PQescapeString. Here is the proposed calling sequence for the new function: size_t PQescapeStringWithConn (const PGconn *conn, char *to, const char *from, size_t length) If this is ok, I will implement for 8.2. Here is the promised patches for 8.2. If there's no objection, I will commit tomorrow. Just a reminder - to work on win32, the new function needs an entry in exports.txt. //Magnus ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] possible design bug with PQescapeString()
But actually I'd argue that letting the client programmer supply the encoding is still a pretty dangerous practice. Your example demonstrates that if the encoding PQescapeString is told is different from the encoding the backend parser thinks is in use, problems result. Perhaps we should pass the PGconn to new-PQescapeString and let it dig the client encoding out of that. Sound good to pass PGconn to new-PQescapeString. Here is the proposed calling sequence for the new function: size_t PQescapeStringWithConn (const PGconn *conn, char *to, const char *from, size_t length) If this is ok, I will implement for 8.2. Here is the promised patches for 8.2. If there's no objection, I will commit tomorrow. Just a reminder - to work on win32, the new function needs an entry in exports.txt. Thanks. Could you tell me what the number column in the file is? Can I assign arbitary number for the function? (maybe next to the last number?) I'm not familiar with win32 build. -- Tatsuo Ishii SRA OSS, Inc. Japan ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] possible design bug with PQescapeString()
Sound good to pass PGconn to new-PQescapeString. Here is the proposed calling sequence for the new function: size_t PQescapeStringWithConn (const PGconn *conn, char *to, const char *from, size_t length) If this is ok, I will implement for 8.2. Here is the promised patches for 8.2. If there's no objection, I will commit tomorrow. Just a reminder - to work on win32, the new function needs an entry in exports.txt. Thanks. Could you tell me what the number column in the file is? Can I assign arbitary number for the function? (maybe next to the last number?) I'm not familiar with win32 build. It just has to be unique, and never change for each function. So just assign the next one in sequence. //Magnus ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] possible design bug with PQescapeString()
On 2006-02-26, Tatsuo Ishii [EMAIL PROTECTED] wrote: On 2006-02-20, Tatsuo Ishii [EMAIL PROTECTED] wrote: In further investigation, Akio Ishida found this kind of attack is possible even with EUC_JP/UTF-8. How? The details have been sent to cores. I wasn't asking out of idle curiosity. Some preliminary investigation that I have done suggests that when using UTF-8, the proposed changes do not fix the problem (and may make matters worse). So I want to know whether the problem that I'm looking at is the same thing as the one you're looking at. UTF-8 has the property that neither ' nor \ can appear as part of a valid multibyte sequence. But many places in postgres are extremely sloppy about handling _invalid_ utf-8, and unless you're prepared to make the escape routine fail outright in such cases (which I would strongly favour), it is likely that there will always be ways to get malformed sequences into the backend (which itself is far too lax about parsing them). -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services ---(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: [HACKERS] possible design bug with PQescapeString()
On 2006-02-26, Tatsuo Ishii [EMAIL PROTECTED] wrote: On 2006-02-20, Tatsuo Ishii [EMAIL PROTECTED] wrote: In further investigation, Akio Ishida found this kind of attack is possible even with EUC_JP/UTF-8. How? The details have been sent to cores. I wasn't asking out of idle curiosity. Some preliminary investigation that I have done suggests that when using UTF-8, the proposed changes do not fix the problem (and may make matters worse). So I want to know whether the problem that I'm looking at is the same thing as the one you're looking at. UTF-8 has the property that neither ' nor \ can appear as part of a valid multibyte sequence. But many places in postgres are extremely sloppy about handling _invalid_ utf-8, and unless you're prepared to make the escape routine fail outright in such cases (which I would strongly favour), it is likely that there will always be ways to get malformed sequences into the backend (which itself is far too lax about parsing them). I guess I understand whay you are saying. However, I am not allowed to talk to you about it unless cores allow me. Probably we need some closed forum to discuss this kind of security issues. The forum should be consisted with cores and people who are admitted by cores. Cores, what do you think? -- Tatsuo Ishii SRA OSS, Inc. Japan ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] possible design bug with PQescapeString()
Tatsuo Ishii [EMAIL PROTECTED] writes: I guess I understand whay you are saying. However, I am not allowed to talk to you about it unless cores allow me. Probably we need some closed forum to discuss this kind of security issues. Considering that you've already described the problem on pgsql-hackers, I hardly see how further discussion is going to create a bigger security breach than already exists. (I'm of the opinion that the problem is mostly a client problem anyway; AFAICS the issue only comes up if client software fails to consider encoding issues while doing escaping. There is certainly no way that we can magically solve the problem in a new PG release, and so trying to keep it quiet until we can work out a solution seems pointless.) regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] possible design bug with PQescapeString()
Tatsuo Ishii [EMAIL PROTECTED] writes: I suggest that PQescapeString() should have a parameter to specify the encoding of to. You mean the encoding of from, no? Oops, from, yes. But actually I'd argue that letting the client programmer supply the encoding is still a pretty dangerous practice. Your example demonstrates that if the encoding PQescapeString is told is different from the encoding the backend parser thinks is in use, problems result. Perhaps we should pass the PGconn to new-PQescapeString and let it dig the client encoding out of that. Sound good to pass PGconn to new-PQescapeString. Here is the proposed calling sequence for the new function: size_t PQescapeStringWithConn (const PGconn *conn, char *to, const char *from, size_t length) If this is ok, I will implement for 8.2. In further investigation, Akio Ishida found this kind of attack is possible even with EUC_JP/UTF-8. So it seems the problem is not only with SJIS, BIG5 and GBK. I think we need to add PQescapeStringWithConn or whatever as soon as possible. -- Tatsuo Ishii SRA OSS, Inc. Japan You could still get burnt if the client encoding changes between the invocation of new-PQescapeString and the sending of the constructed command, but that's a fairly unlikely case. The bottom line to this though is that these encodings are just plain dangerous. I'm more than half tempted to suggest that the only secure answer is to drop support for these encodings. Consider for example an application that isn't using PQescapeString but has its own double-backslashes-and-quotes logic embedded. You can break it if you can manage to get the backend to think that the client encoding is SJIS or similar. That's a hazard we're basically not ever going to be able to prevent. Dropping support for SJIS and so on will not be practical at all since these encodings has been widely used and I don't see these encodings are deprecated in the near future. I think dropping the support will simply prevent people from using PostgreSQL. Especially in Windows world, these encodings are pretty common. I know that these encodings are broken in their design and actually I hate them:-) But this is real world and we have to live with them... -- Tatsuo Ishii SRA OSS, Inc. Japan ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] possible design bug with PQescapeString()
On 2006-02-20, Tatsuo Ishii [EMAIL PROTECTED] wrote: In further investigation, Akio Ishida found this kind of attack is possible even with EUC_JP/UTF-8. How? -- Andrew, Supernews http://www.supernews.com - individual and corporate NNTP services ---(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: [HACKERS] possible design bug with PQescapeString()
* Tatsuo Ishii: Users can input value for var from a web form. The attacker inputs following string: (0x95+0x27);DELETE FROM members;-- where 0x95+0x27 is actually a SJIS mutibyte KANJI. Programmer applies PQescapeString() to it and gets: 0x95+0x27+0x27;DELETE FROM members;-- Uh-oh, this is my fault. PQescapeString should escape all characters greater than 126. Unfortunately, there is nothing we can do about this in the current function because tha twould need four times the lenggth of the input string (plus one). Drat. (I don't think you should have to consider the encoding in the client; strange things may happen if there is an interpretation conflict between the client and the backend.) ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [HACKERS] possible design bug with PQescapeString()
* Tatsuo Ishii: Users can input value for var from a web form. The attacker inputs following string: (0x95+0x27);DELETE FROM members;-- where 0x95+0x27 is actually a SJIS mutibyte KANJI. Programmer applies PQescapeString() to it and gets: 0x95+0x27+0x27;DELETE FROM members;-- Uh-oh, this is my fault. PQescapeString should escape all characters greater than 126. Unfortunately, there is nothing we can do about this in the current function because tha twould need four times the lenggth of the input string (plus one). Drat. Please don't do that. That would break all applications those use the mutibyte encodings including UTF-8. (I don't think you should have to consider the encoding in the client; strange things may happen if there is an interpretation conflict between the client and the backend.) No. For the sake PQmblen() is provided. What I (and I guess Tom too) am thinking is like this: attacker's input: (0x95+0x27);DELETE FROM members;-- new-PQescapeString() treats this: 0x95+0x27;DELETE FROM members;-- because the encoding is SJIS. And the result SQL will be: SELECT * FROM members WHERE member_name = '0x95+0x27;DELETE FROM members;--'; The attacker loses. -- Tatsuo Ishii SRA OSS, Inc. Japan ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [HACKERS] possible design bug with PQescapeString()
* Tatsuo Ishii: Uh-oh, this is my fault. PQescapeString should escape all characters greater than 126. Unfortunately, there is nothing we can do about this in the current function because tha twould need four times the lenggth of the input string (plus one). Drat. Please don't do that. That would break all applications those use the mutibyte encodings including UTF-8. Why? Doesn't the server perform unquoting *before* multi-byte processing? -- Ah, it doesn't. Perhaps this is the part which should be fixed? (I don't think you should have to consider the encoding in the client; strange things may happen if there is an interpretation conflict between the client and the backend.) No. For the sake PQmblen() is provided. What I (and I guess Tom too) am thinking is like this: attacker's input: (0x95+0x27);DELETE FROM members;-- new-PQescapeString() treats this: 0x95+0x27;DELETE FROM members;-- But this still needs knowledge of SJIS at the client side (and both client and backend must have the same notion of SJIS). ---(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: [HACKERS] possible design bug with PQescapeString()
Uh-oh, this is my fault. PQescapeString should escape all characters greater than 126. Unfortunately, there is nothing we can do about this in the current function because tha twould need four times the lenggth of the input string (plus one). Drat. Please don't do that. That would break all applications those use the mutibyte encodings including UTF-8. Why? Doesn't the server perform unquoting *before* multi-byte processing? -- Ah, it doesn't. Perhaps this is the part which should be fixed? No no. Probably you misunderstand why we need quoting. If special characters such as ' or \ appears, it should be quoted. But you should not if it's a part of multibyte characters. (I don't think you should have to consider the encoding in the client; strange things may happen if there is an interpretation conflict between the client and the backend.) No. For the sake PQmblen() is provided. What I (and I guess Tom too) am thinking is like this: attacker's input: (0x95+0x27);DELETE FROM members;-- new-PQescapeString() treats this: 0x95+0x27;DELETE FROM members;-- But this still needs knowledge of SJIS at the client side (and both client and backend must have the same notion of SJIS). No problem. We have the client encoding in PGConn. That's why Tom suggests PQescapeString() should have the PGCConn argument. -- Tatsuo Ishii SRA OSS, Inc. Japan ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [HACKERS] possible design bug with PQescapeString()
Florian Weimer [EMAIL PROTECTED] writes: Uh-oh, this is my fault. PQescapeString should escape all characters greater than 126. No, that doesn't work, because the de-escaping on the backend side happens *after* conversion to the backend encoding. If you insert escapes into the middle of multibyte characters then you break the conversion. Tatsuo's description of the problem is accurate (though I'm not sure I agree with his solution ;-)) regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [HACKERS] possible design bug with PQescapeString()
On Sun, Feb 19, 2006 at 12:13:48PM -0500, Tom Lane wrote: Florian Weimer [EMAIL PROTECTED] writes: Uh-oh, this is my fault. PQescapeString should escape all characters greater than 126. No, that doesn't work, because the de-escaping on the backend side happens *after* conversion to the backend encoding. If you insert escapes into the middle of multibyte characters then you break the conversion. Well, most encodings provide an easy way to determine leader and follower characters. The PQmblen() and related functions can help here. Something like: if( PQmblen(enc,ptr) 1 ) copy bytes else if( SQL_STR_DOUBLE( *ptr ) ) etc... Assuming there are no multibyte string terminators... And assuming you actually know what encoding the server expects. However, the real solution seems to me to be to use something like PQexecParams and ship the arguments outside the query string, thus avoiding the issue entirely. Have a nice day, -- Martijn van Oosterhout kleptog@svana.org http://svana.org/kleptog/ Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a tool for doing 5% of the work and then sitting around waiting for someone else to do the other 95% so you can sue them. signature.asc Description: Digital signature
Re: [HACKERS] possible design bug with PQescapeString()
Tatsuo Ishii [EMAIL PROTECTED] writes: I suggest that PQescapeString() should have a parameter to specify the encoding of to. You mean the encoding of from, no? But actually I'd argue that letting the client programmer supply the encoding is still a pretty dangerous practice. Your example demonstrates that if the encoding PQescapeString is told is different from the encoding the backend parser thinks is in use, problems result. Perhaps we should pass the PGconn to new-PQescapeString and let it dig the client encoding out of that. You could still get burnt if the client encoding changes between the invocation of new-PQescapeString and the sending of the constructed command, but that's a fairly unlikely case. The bottom line to this though is that these encodings are just plain dangerous. I'm more than half tempted to suggest that the only secure answer is to drop support for these encodings. Consider for example an application that isn't using PQescapeString but has its own double-backslashes-and-quotes logic embedded. You can break it if you can manage to get the backend to think that the client encoding is SJIS or similar. That's a hazard we're basically not ever going to be able to prevent. regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [HACKERS] possible design bug with PQescapeString()
Tatsuo Ishii [EMAIL PROTECTED] writes: I suggest that PQescapeString() should have a parameter to specify the encoding of to. You mean the encoding of from, no? Oops, from, yes. But actually I'd argue that letting the client programmer supply the encoding is still a pretty dangerous practice. Your example demonstrates that if the encoding PQescapeString is told is different from the encoding the backend parser thinks is in use, problems result. Perhaps we should pass the PGconn to new-PQescapeString and let it dig the client encoding out of that. Sound good to pass PGconn to new-PQescapeString. Here is the proposed calling sequence for the new function: size_t PQescapeStringWithConn (const PGconn *conn, char *to, const char *from, size_t length) If this is ok, I will implement for 8.2. You could still get burnt if the client encoding changes between the invocation of new-PQescapeString and the sending of the constructed command, but that's a fairly unlikely case. The bottom line to this though is that these encodings are just plain dangerous. I'm more than half tempted to suggest that the only secure answer is to drop support for these encodings. Consider for example an application that isn't using PQescapeString but has its own double-backslashes-and-quotes logic embedded. You can break it if you can manage to get the backend to think that the client encoding is SJIS or similar. That's a hazard we're basically not ever going to be able to prevent. Dropping support for SJIS and so on will not be practical at all since these encodings has been widely used and I don't see these encodings are deprecated in the near future. I think dropping the support will simply prevent people from using PostgreSQL. Especially in Windows world, these encodings are pretty common. I know that these encodings are broken in their design and actually I hate them:-) But this is real world and we have to live with them... -- Tatsuo Ishii SRA OSS, Inc. Japan ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
