Decision by Monday: PQescapeString() vs. encoding violation

On Fri, 2025-02-14 at 17:27 -0800, Noah Misch wrote:

Commit 5dc1e42b4fa6a4434afa7d7cdcf0291351a7b873 for this week's CVE-2025-1094
changed how PQescapeString()[1] reacts to input that is not valid in the
client encoding.  Before that commit, the function would ignore encoding
problems except at the end of the string.  Now, it replaces the bad sequence
up to the length implied by the first byte.  For example, if UTF8 input has
0xc2 followed by an ASCII byte, the function removes both bytes.

Jeff Davis reported to the security team that
http://www.unicode.org/reports/tr36/#Ill-Formed_Subsequences forbids something
like this, saying a UTF-8 converter "must not consume the [second byte] if it
continues".  (That's my summary, not his.  He might reply to add color here.)
While PQescapeString() is not a UTF-8 converter, that standard still felt to
multiple security team members like a decent argument for removing only the
invalid 0xc2, not the byte following it.

Security team members disagreed on whether next week's releases are the last
reasonable chance to change this, or whether changing it in e.g. May would be
reasonable.  If applications make changes to cope with the new behavior, that
could be an argument against further change.

Question for all: would you switch to the "remove fewer bytes" behavior in
next week's releases, switch later, or switch never?  Why so?  Please answer
in the next 24hr if possible, given the little time until we wrap next week's
releases on Monday.  I regret the late notice.

I don't have a very strong opinion, but I'm leaning towards removing fewer bytes.

My reasoning is that most encoding confusion comes from Microsoft's tenacious
clinging to non-Unicode encodings, and in my part of the world single-byte
encodings are prevalent on Windows systems. Removing more bytes from the string
would mean removing more characters, which would mutilate the string more than
necessary.

About when to make that change: having fewer backward-incompatible behavior
changes is desirable, which would speak for doing it now. If you feel that the
short time frame is long enough for you to write good, reliable code, great.
If you feel that it would be better for the quality of the code to have more
time, take the time. Better one more subtle change than another security bug.

Yours,
Laurenz Albe

--

*E-Mail Disclaimer*
Der Inhalt dieser E-Mail ist ausschliesslich fuer den
bezeichneten Adressaten bestimmt. Wenn Sie nicht der vorgesehene Adressat
dieser E-Mail oder dessen Vertreter sein sollten, so beachten Sie bitte,
dass jede Form der Kenntnisnahme, Veroeffentlichung, Vervielfaeltigung oder
Weitergabe des Inhalts dieser E-Mail unzulaessig ist. Wir bitten Sie, sich
in diesem Fall mit dem Absender der E-Mail in Verbindung zu setzen.

*CONFIDENTIALITY NOTICE & DISCLAIMER
*This message and any attachment are
confidential and may be privileged or otherwise protected from disclosure
and solely for the use of the person(s) or entity to whom it is intended.
If you have received this message in error and are not the intended
recipient, please notify the sender immediately and delete this message and
any attachment from your system. If you are not the intended recipient, be
advised that any use of this message is prohibited and may be unlawful, and
you must not copy this message or attachment or disclose the contents to
any other person.

On 2025-02-14 Fr 8:27 PM, Noah Misch wrote:

The security team has a question below about how best to proceed with a recent
behavior change.

Commit 5dc1e42b4fa6a4434afa7d7cdcf0291351a7b873 for this week's CVE-2025-1094
changed how PQescapeString()[1] reacts to input that is not valid in the
client encoding. Before that commit, the function would ignore encoding
problems except at the end of the string. Now, it replaces the bad sequence
up to the length implied by the first byte. For example, if UTF8 input has
0xc2 followed by an ASCII byte, the function removes both bytes.

Jeff Davis reported to the security team that
http://www.unicode.org/reports/tr36/#Ill-Formed_Subsequences forbids something
like this, saying a UTF-8 converter "must not consume the [second byte] if it
continues". (That's my summary, not his. He might reply to add color here.)
While PQescapeString() is not a UTF-8 converter, that standard still felt to
multiple security team members like a decent argument for removing only the
invalid 0xc2, not the byte following it. UTF8 is the most important encoding,
and other encodings tolerate this. Either way, the server will report an
encoding error. The difference doesn't have functional consequences if one
simply puts the function result in a query. The difference could matter for
debugging or if applications are postprocessing the PQescapeString() result in
some way. Postprocessing is not supported, but we'd still like to do the best
thing for applications that may already be doing it.

Security team members disagreed on whether next week's releases are the last
reasonable chance to change this, or whether changing it in e.g. May would be
reasonable. If applications make changes to cope with the new behavior, that
could be an argument against further change.

Question for all: would you switch to the "remove fewer bytes" behavior in
next week's releases, switch later, or switch never? Why so? Please answer
in the next 24hr if possible, given the little time until we wrap next week's
releases on Monday. I regret the late notice.

I'm attaching a WIP patch from Andres Freund. We may use it to adopt the
"remove fewer bytes" behavior, if that's the decision.

Thanks,
nm

[1] The commit changed other functions, but PQescapeString() is most
interesting for this discussion. Others have ways to report errors, or they
have reason to believe the input is already checked. New code should be using
the others and checking the error indicator.

Removing fewer bytes seems like the right thing to do, now or later. The
WIP patch itself is trivial. If it weren't I'd be opposed to doing it at
the last minute. Even so I'm a bit nervous - last minute changes have
bitten us before.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

I studied the v3 patch a little and realized that it only fixes the
behavior for the case of a complete-but-invalid multibyte character.
If we have an incomplete character at the end of the string, the
whole thing still gets dropped. Surely that's not what we want if
we are going to adopt this behavior.

Here's a v4 that fixes that. As a bonus, we can get rid of
duplicative coding since the "incomplete" and "invalid" cases
are now treated identically.

One minor point is that the error messages from PQescapeStringInternal
no longer distinguish "incomplete" from "invalid". I don't find that
to be a terribly useful distinction, so that's fine with me. But if
someone feels that's important to preserve, we could make it do
something like

if (conn)
{
if (remaining < charlen)
libpq_append_conn_error(conn, "incomplete multibyte character");
else
libpq_append_conn_error(conn, "invalid multibyte character");
}

regards, tom lane

Hi,

On 2025-02-15 13:08:03 -0500, Tom Lane wrote:

I studied the v3 patch a little and realized that it only fixes the
behavior for the case of a complete-but-invalid multibyte character.
If we have an incomplete character at the end of the string, the
whole thing still gets dropped. Surely that's not what we want if
we are going to adopt this behavior.

Good catch! The different behaviour made sense when we were dropping the
entire multi-byte character, but not anymore.

Here's a v4 that fixes that. As a bonus, we can get rid of
duplicative coding since the "incomplete" and "invalid" cases
are now treated identically.

One minor point is that the error messages from PQescapeStringInternal
no longer distinguish "incomplete" from "invalid". I don't find that
to be a terribly useful distinction, so that's fine with me. But if
someone feels that's important to preserve, we could make it do
something like

I think there's some value in the distinction, because incomplete characters
can happen a lot more easily due to truncating longer strings. I don't know if
it's worth the code complexity though.

From 935ccea6da82d0150f65672825c8f5e6f86bca89 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, 15 Feb 2025 12:58:22 -0500
Subject: [PATCH v4] Make escaping functions retain trailing bytes of an
invalid character.

Instead of dropping the trailing byte(s) of an invalid or incomplete
multibyte character, replace only the first byte with a known-invalid
sequence, and process the rest normally. This seems less likely to
confuse incautious callers than the behavior adopted in 5dc1e42b4.

Author: Andres Freund <andres@anarazel.de>
Reviewed-by: Jeff Davis <pgsql@j-davis.com>

This should have been a Reported-by, not a Reviewed-by, my mistake...

It seems that nobody is arguing against the "just skip one byte" behaviour, so
I'm inclined to push this fairly soon, even if Noah's "24 hours" haven't quite
elapsed. A few more cycles in the buildfarm wouldn't hurt.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

It seems that nobody is arguing against the "just skip one byte" behaviour, so
I'm inclined to push this fairly soon, even if Noah's "24 hours" haven't quite
elapsed. A few more cycles in the buildfarm wouldn't hurt.

Agreed. I thought there would be more discussion, but it seems
nobody really objects to changing this.

The other thing that was discussed in the security thread was
modifying PQescapeStringInternal and PQescapeInternal to produce
no more than one complaint about invalid multibyte characters,
on the grounds that input that's just plain in some other encoding
would otherwise produce a ton of repetitive messages. That seems
trivial enough to mechanize with a bool already_complained flag,
so I think we should incorporate that refinement while we're here.
I can write that patch if you're busy.

regards, tom lane

On Sat, Feb 15, 2025 at 01:23:51PM -0500, Andres Freund wrote:

On 2025-02-15 13:08:03 -0500, Tom Lane wrote:

I studied the v3 patch a little and realized that it only fixes the
behavior for the case of a complete-but-invalid multibyte character.
If we have an incomplete character at the end of the string, the
whole thing still gets dropped. Surely that's not what we want if
we are going to adopt this behavior.

Thanks for doing that.

Good catch! The different behaviour made sense when we were dropping the
entire multi-byte character, but not anymore.

Here's a v4 that fixes that. As a bonus, we can get rid of
duplicative coding since the "incomplete" and "invalid" cases
are now treated identically.

One minor point is that the error messages from PQescapeStringInternal
no longer distinguish "incomplete" from "invalid". I don't find that
to be a terribly useful distinction, so that's fine with me. But if
someone feels that's important to preserve, we could make it do
something like

I think there's some value in the distinction, because incomplete characters
can happen a lot more easily due to truncating longer strings. I don't know if
it's worth the code complexity though.

While I agree it doesn't feel like a terribly-important distinction, I'd
definitely keep that distinction today rather than drop it with so little
notice. (I'd feel fine about dropping it in a major release. I'd probably
not drop it in any minor release, but especially not this close to wrap.)

On Sat, Feb 15, 2025 at 01:35:10PM -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

It seems that nobody is arguing against the "just skip one byte" behaviour, so
I'm inclined to push this fairly soon, even if Noah's "24 hours" haven't quite
elapsed. A few more cycles in the buildfarm wouldn't hurt.

Works for me; we can always revert in the event of further discussion. For
what it's worth, I didn't intend this 24hr as a hard period for gating any
particular action.

Agreed. I thought there would be more discussion, but it seems
nobody really objects to changing this.

The other thing that was discussed in the security thread was
modifying PQescapeStringInternal and PQescapeInternal to produce
no more than one complaint about invalid multibyte characters,
on the grounds that input that's just plain in some other encoding
would otherwise produce a ton of repetitive messages. That seems
trivial enough to mechanize with a bool already_complained flag,
so I think we should incorporate that refinement while we're here.

+1

Noah Misch <noah@leadboat.com> writes:

On 2025-02-15 13:08:03 -0500, Tom Lane wrote:

The other thing that was discussed in the security thread was
modifying PQescapeStringInternal and PQescapeInternal to produce
no more than one complaint about invalid multibyte characters,
on the grounds that input that's just plain in some other encoding
would otherwise produce a ton of repetitive messages. That seems
trivial enough to mechanize with a bool already_complained flag,
so I think we should incorporate that refinement while we're here.

+1

On closer inspection, PQescapeInternal already issues only one
error message, since it does "return NULL" after detecting the
first error. So this makes PQescapeStringInternal behave more
like that.

regards, tom lane

Hi,

On 2025-02-15 14:12:06 -0500, Tom Lane wrote:

On closer inspection, PQescapeInternal already issues only one
error message, since it does "return NULL" after detecting the
first error. So this makes PQescapeStringInternal behave more
like that.

This looks good to me.

I looked through the diff between what test_escape -v before/after this change
prints out, looks good to me.

From 565b42cecf645b1dbde277512fd67836ee9081d1 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat, 15 Feb 2025 14:10:33 -0500
Subject: [PATCH v5] Make escaping functions retain trailing bytes of an
invalid character.

Instead of dropping the trailing byte(s) of an invalid or incomplete
multibyte character, replace only the first byte with a known-invalid
sequence, and process the rest normally. This seems less likely to
confuse incautious callers than the behavior adopted in 5dc1e42b4.

Author: Andres Freund <andres@anarazel.de>

I think you deserve primary authorship for the change now...

Are you planning to push / backpatch, or should I?

Greetings,

Andres Freund

Expanding on the reasoning a bit:

This discussion is only relevant only when the application meets the
following conditions:

A. Sends invalidly-encoded input to an escaping routine. Many
languages protect against this, such as python and rust. But other
languages, like C, Go, and Ruby do not.

B. Uses PQescapeStringConn() and ignores the error, or an escaping
routine that doesn't provide an error at all. (Earlier versions of
PQescapeStringConn ignored invalid sequences in the middle of the
string, but 5dc1e42b4f fixed that.)

C. Does some kind of post-processing to the escaped output to remove
invalidly-encoded data before sending it to the server. If the invalid
data wasn't removed, the server would throw an immediate error anyway.

Given that the situation already involves A, B & C, it's already
somewhat of a special case and I don't consider it a clear-and-present
security threat beyond what was fixed for the CVE. Instead, this is
more about following an accepted practice that is less likely to result
in security problems even if the application code is imperfect.

On Fri, 2025-02-14 at 17:27 -0800, Noah Misch wrote:

Question for all: would you switch to the "remove fewer bytes"
behavior in
next week's releases, switch later, or switch never?  Why so?

The reasoning why it's the right change are:

1. It seems reasonable to assume most invalid byte sequences in the
middle of a string usually resulted from some kind of byte splicing or
concatenation. For instance, an application taking untrusted input
bytes, validating only at the byte level, and then concatenating with
other text (which might be done by string interpolation or by applying
a template).

- Given the above assumption, valid initial bytes in a byte sequence
are much more likely to be the start of a new valid character than part
of the previous invalid character.

- If it's the start of a new valid character, it might be an
important delimiter, and removing it could have security consequences,
perhaps not for Postgres itself, but for whatever the application does
with that string (with a missing delimiter) later.

2. The comparable risk on the other side seems substantially lower.
Let's say your application considers 0x7C ('|') to be a very important
delimiter. If the untrusted input contains, e.g. "\xC2\x7C" (invalid
sequence), one could imagine PQescapeStringConn() saving the day by
removing both bytes. But that seems more like an accident than anything
else: the application needs to validate the untrusted input regardless;
otherwise the attacker could just supply a valid '|'.

- Caveat: the application's validation could be broken in such a way
that it skips over multibyte characters without checking for invalid
sequences, i.e. seeing the \xC2 and skipping over the \x7C without
escaping it. Unfortunately, that's exactly what PQescapeStringConn()
did until recently, so this is a reasonable argument in favor of
"remove more bytes". But we shouldn't favor multibyte-half-aware
validators at the cost of breaking byte-by-byte validators.

3. Unicode strongly recommends that we take the "remove fewer bytes"
option. They are in a good position to have an informed opinion about
the security consequences of either choice, so this is partially an
appeal to authority. But that also means that it's the way other string
handling functions are likley to behave, e.g. Ruby's String#scrub seems
to take the "remove fewer bytes" approach. Following convention has its
own security benefits.

Regards,
Jeff Davis

On Fri, 2025-02-14 at 17:27 -0800, Noah Misch wrote:

I'm attaching a WIP patch from Andres Freund.

I am not suggesting a change, but there's a minor point about the
behavior of the replacement that I'd like to highlight:

Unicode discusses a choice[1]https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G48534: "An ill-formed subsequence consisting of
more than one code unit could be treated as a single error or as
multiple errors."

The patch implements the latter. Escaping:
<7A F0 80 80 41 7A>
results in:
<7A C0 20 C0 20 C0 20 41 7A>

The Unicode standard suggests[2]https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G66453 that the former approach may provide
more consistency in how it's done, but that doesn't seem important or
relevant for our purposes. I'd favor whichever approach results in
simpler code.

Regards,
Jeff Davis

[1]: https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G48534
https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G48534

[2]: https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G66453
https://www.unicode.org/versions/Unicode16.0.0/core-spec/chapter-3/#G66453

Hi,

On 2025-02-15 12:35:45 -0800, Jeff Davis wrote:

I am not suggesting a change, but there's a minor point about the
behavior of the replacement that I'd like to highlight:

Unicode discusses a choice[1]: "An ill-formed subsequence consisting of
more than one code unit could be treated as a single error or as
multiple errors."

The patch implements the latter. Escaping:
<7A F0 80 80 41 7A>
results in:
<7A C0 20 C0 20 C0 20 41 7A>

The Unicode standard suggests[2] that the former approach may provide
more consistency in how it's done, but that doesn't seem important or
relevant for our purposes. I'd favor whichever approach results in
simpler code.

It seems completely infeasible to me to to implement the "single error"
approach in a minor version. It'd afaict require non-trivial new
infrastructure. We can't just consume up to the next byte without a high bit,
because some encodings have subsequent bytes that are not guaranteed to have a
high bit set.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2025-02-15 12:35:45 -0800, Jeff Davis wrote:

I am not suggesting a change, but there's a minor point about the
behavior of the replacement that I'd like to highlight:
Unicode discusses a choice[1]: "An ill-formed subsequence consisting of
more than one code unit could be treated as a single error or as
multiple errors."

It seems completely infeasible to me to to implement the "single error"
approach in a minor version. It'd afaict require non-trivial new
infrastructure. We can't just consume up to the next byte without a high bit,
because some encodings have subsequent bytes that are not guaranteed to have a
high bit set.

Yeah. Also I think that probably depends on being able to tell the
difference between a first byte and a not-first byte of a multibyte
character, something that works in UTF-8 but not necessarily elsewhere.
As I commented in the security thread, Unicode's recommendations seem
pretty UTF-8-centric; I'm hesitant to adopt them wholesale in code
that has to deal with other encodings.

The v5 patch seems Good Enough(TM) to me. We can refine it later
perhaps; I don't think something like the above would affect
anything that external code should care about.

regards, tom lane

On Sat, 2025-02-15 at 15:43 -0500, Andres Freund wrote:

It seems completely infeasible to me to to implement the "single
error"
approach in a minor version.

+1, keep with the behavior in the proposed patches.

Even in the next major version, I'd be inclined to try to move toward
interfaces that can produce an error reliably rather than spending more
time trying to use a "better" pattern of invalid bytes.

I just wanted to point out that some kind of decision was made and link
to relevant background information.

Regards,
Jeff Davis

Andres Freund <andres@anarazel.de> writes:

Are you planning to push / backpatch, or should I?

I can take care of it, if you like.

regards, tom lane

Hi,

On 2025-02-15 15:52:01 -0500, Tom Lane wrote:

The v5 patch seems Good Enough(TM) to me.

Agreed.

We can refine it later perhaps; I don't think something like the above would
affect anything that external code should care about.

I don't really think it's worth spending cycles on this anytime soon. It makes
sense to put the effort in to replace invalid "characters" in a minimal way
when intending to actually use the "stripped" output permanently. But all
we're trying to do here is to a) ensure that the backend will error out b)
reduce the chances that other tooling (psql, xml parsers, ..) get confused due
to invalidly encoded data.

Greetings,

Andres Freund

On 2025-02-15 15:53:20 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

Are you planning to push / backpatch, or should I?

I can take care of it, if you like.

That'd be appreciated!

Andres Freund <andres@anarazel.de> writes:

On 2025-02-15 15:53:20 -0500, Tom Lane wrote:

I can take care of it, if you like.

That'd be appreciated!

Done.

regards, tom lane

On Sat, 2025-02-15 at 16:21 -0500, Tom Lane wrote:

Andres Freund <andres@anarazel.de> writes:

On 2025-02-15 15:53:20 -0500, Tom Lane wrote:

I can take care of it, if you like.

That'd be appreciated!

Done.

Thank you all.

Regards,
Jeff Davis