possible design bug with PQescapeString()

I believe PQescapeString() has an important design bug and it casues a
security risk.

The function's signature is:

size_t PQescapeString (char *to, const char *from, size_t length);

As you might notice, it's impossible to specify encoding of "to". As a
result, it turns every occurrences of 0x27(') or 0x5c(\) to 0x270x27
or 0x5c0x5c. This is fine with ASCII, UTF-8, EUC-JP and so on. However
cetain Asian multibyte charsets such as SJIS, Big5 and GBK have a bit
pattern in that the second byte is 0x27(') or 0x5c(\). Applying
PQescapeString() to them will produce invalid character sequences.

But there's more. Problem is, PQescapeString() makes SQL injections
possible. Here is an example:

There is an application which selects particlular member info from a
table in this way:

SELECT * FROM members WHERE member_name = 'var';

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;--

and the result SQL will be:

SELECT * FROM members WHERE member_name = '0x95+0x27';DELETE FROM members;--';

You lose members table:-<

Conclusion:

I suggest that PQescapeString() should have a parameter to specify the
encoding of "to".

BTW it's irony that PQescapeStringt man page stats like this:-)

Tip: 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.
--
Tatsuo Ishii
SRA OSS, Inc. Japan

Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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

> Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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

* 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.)

> * 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

* 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).

> >> 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

Florian Weimer <fw(at)deneb(dot)enyo(dot)de> 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

On Sun, Feb 19, 2006 at 12:13:48PM -0500, Tom Lane wrote:
> Florian Weimer <fw(at)deneb(dot)enyo(dot)de> 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(at)svana(dot)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.

> > Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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
>

On 2006-02-20, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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

> On 2006-02-20, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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

> > > 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

On 2006-02-26, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> wrote:
>> On 2006-02-20, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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

> On 2006-02-26, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> wrote:
> >> On 2006-02-20, Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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

Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp> 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

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 <ishii(at)sraoss(dot)co(dot)jp> 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
>