Thread: Bug: Reading from single byte character column type may cause out of bounds memory reads.

Hi Spyridon,

> The column "single_byte_col" is supposed to store only 1 byte. Nevertheless, the INSERT command implicitly casts the
'🀆'text into "char". This means that only the first byte of '🀆' ends up stored in the column. 
> gdb reports that "pg_mblen(p) = 4" (line 1046), which is expected since the pg_mblen('🀆') is indeed 4. Later at line
1050,the memcpy will copy 4 bytes instead of 1, hence an out of bounds memory read happens for pointer 's', which
effectivelycopies random bytes. 

Many thanks for reporting this!

> - OS: Ubuntu 20.04
> - PSQL version 14.4

I can confirm the bug exists in the `master` branch as well and
doesn't depend on the platform.

Although the bug is easy to fix for this particular case (see the
patch) I'm not sure if this solution is general enough. E.g. is there
something that generally prevents pg_mblen() from doing out of bound
reading in cases similar to this one? Should we prevent such an INSERT
from happening instead?

--
Best regards,
Aleksander Alekseev

Aleksander Alekseev <aleksander@timescale.com> writes:
> Although the bug is easy to fix for this particular case (see the
> patch) I'm not sure if this solution is general enough. E.g. is there
> something that generally prevents pg_mblen() from doing out of bound
> reading in cases similar to this one? Should we prevent such an INSERT
> from happening instead?

This is ultimately down to char_text() generating a string that's alleged
to be a valid "text" type value, but it contains illegally-encoded data.
Where we need to fix it is there: if we try to make every single
text-using function be 100% bulletproof against wrongly-encoded data,
we'll still be fixing bugs at the heat death of the universe.

I complained about this in [1], but that thread died off before reaching a
clear consensus about exactly what to do.

            regards, tom lane

[1] https://www.postgresql.org/message-id/flat/2318797.1638558730%40sss.pgh.pa.us

On 2022-07-13 We 11:11, Tom Lane wrote:
> Aleksander Alekseev <aleksander@timescale.com> writes:
>> Although the bug is easy to fix for this particular case (see the
>> patch) I'm not sure if this solution is general enough. E.g. is there
>> something that generally prevents pg_mblen() from doing out of bound
>> reading in cases similar to this one? Should we prevent such an INSERT
>> from happening instead?
> This is ultimately down to char_text() generating a string that's alleged
> to be a valid "text" type value, but it contains illegally-encoded data.
> Where we need to fix it is there: if we try to make every single
> text-using function be 100% bulletproof against wrongly-encoded data,
> we'll still be fixing bugs at the heat death of the universe.
>
> I complained about this in [1], but that thread died off before reaching a
> clear consensus about exactly what to do.
>
>             regards, tom lane
>
> [1] https://www.postgresql.org/message-id/flat/2318797.1638558730%40sss.pgh.pa.us
>
>


Looks like the main controversy was about the output format. Make an
executive decision and pick one.


cheers


andrew

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

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2022-07-13 We 11:11, Tom Lane wrote:
>> I complained about this in [1], but that thread died off before reaching a
>> clear consensus about exactly what to do.
>> [1] https://www.postgresql.org/message-id/flat/2318797.1638558730%40sss.pgh.pa.us

> Looks like the main controversy was about the output format. Make an
> executive decision and pick one.

Done, see other thread.

            regards, tom lane

В письме от среда, 13 июля 2022 г. 16:14:39 MSK пользователь Aleksander
Alekseev написал:

Hi! Let me join the review process. Postgres data types is field of expertise I
am interested in.

0. Though it looks like a steady bug, I can't reproduce it. Not using
valgrind, not using ASan (address sanitizer should catch reading out of bounds
too). I am running Debian Bullseye, and tried to build both postgresl 14.4 and
current master.

Never the less I would dig into this issue. And start with the parts that is
not covered by the patch, but seems to be important for me.

1. typename "char" (with quotes) is very-very-very confusing. it is described
in documentation, but you need to be postgres expert or careful documentation
reader, to notice important difference between "char" and char.
What is the history if "char" type? Is it specified by some standard? May be it
is good point to create more understandable alias, like byte_char, ascii_char
or something for usage in practice, and keep "char" for backward compatibility
only.

2. I would totally agree with  Tom Lane and Isaac Morland, that problem should
be also fixed  on the side of type conversion.  There is whole big thread about
it. Guess we should come to some conclusion there

3.Fixing out of bound reading for broken unicode is also important.  Though
for now I am not quite sure it is possible.


> -            p += pg_mblen(p);
> +        {
> +            int t = pg_mblen(p);
> +            p += t;
> +            max_copy_bytes -= t;
> +        }

Minor issue: Here I would change variable name from "t" to "char_len" or
something, to make code more easy to understand.

Major issue: is pg_mblen function safe to call with broken encoding at the end
of buffer? What if last byte of the buffer is 0xF0 and you call pg_mblen for it?


>+        copy_bytes = p - s;
>+        if(copy_bytes > max_copy_bytes)
>+            copy_bytes = max_copy_bytes;

Here I would suggest to add comment about broken utf encoding case. That would
explain why we might come to situation when we can try to copy more than we
have.

I would also suggest to issue a warning here. I guess person that uses
postgres would prefer to know that he managed to stuff into postgres a string
with broken utf encoding, before it comes to some terrible consequences.

> Hi Spyridon,
>
> > The column "single_byte_col" is supposed to store only 1 byte.
> > Nevertheless, the INSERT command implicitly casts the '🀆' text into
> > "char". This means that only the first byte of '🀆' ends up stored in the
> > column. gdb reports that "pg_mblen(p) = 4" (line 1046), which is expected
> > since the pg_mblen('🀆') is indeed 4. Later at line 1050, the memcpy will
> > copy 4 bytes instead of 1, hence an out of bounds memory read happens for
> > pointer 's', which effectively copies random bytes.
> Many thanks for reporting this!
>
> > - OS: Ubuntu 20.04
> > - PSQL version 14.4
>
> I can confirm the bug exists in the `master` branch as well and
> doesn't depend on the platform.
>
> Although the bug is easy to fix for this particular case (see the
> patch) I'm not sure if this solution is general enough. E.g. is there
> something that generally prevents pg_mblen() from doing out of bound
> reading in cases similar to this one? Should we prevent such an INSERT
> from happening instead?


--
Nikolay Shaplov aka Nataraj
Fuzzing Engineer at Postgres Professional
Matrix IM: @dhyan:nataraj.su

Spyridon Dimitrios Agathos <spyridon.dimitrios.agathos@gmail.com> writes:
> this is to verify that the .patch proposed here:
> https://www.postgresql.org/message-id/flat/2318797.1638558730%40sss.pgh.pa.us
> fixes the issue.

> Looking forward to the next steps.

That's been committed into HEAD and v15, without pushback so far.
So the complained-of case is no longer reachable in those branches.

I think we should reject Aleksander's patch, on the grounds that
it's now unnecessary --- or if you want to argue that it's still
necessary, then it's woefully inadequate, because there are surely
a bunch of other text-processing functions that will also misbehave
on wrongly-encoded data.  But our general policy for years has been
that we check incoming text for encoding validity and then presume
that it is valid in manipulation operations.

What remains to be debated is whether to push ec62ce55a into the
stable branches.  While we've not had pushback about the change
in 15beta3, that hasn't been out very long, so I don't know how
much faith to put in the lack of complaints.  Should we wait
longer before deciding?

I'm leaning to the idea that we should not back-patch, because
this issue has been there for years with few complaints; it's
not clear that closing the hole is worth creating a compatibility
hazard in minor releases.  On the other hand, you could argue
that we should back-patch so that back-branch charin() will
understand the strings that can now be emitted by v15 charout().
Failing to do so will result in a different sort of compatibility
problem.

            regards, tom lane

On Thu, Sep 01, 2022 at 03:35:52PM -0400, Tom Lane wrote:
> Spyridon Dimitrios Agathos <spyridon.dimitrios.agathos@gmail.com> writes:
> > this is to verify that the .patch proposed here:
> > https://www.postgresql.org/message-id/flat/2318797.1638558730%40sss.pgh.pa.us
> > fixes the issue.
> 
> > Looking forward to the next steps.
> 
> That's been committed into HEAD and v15, without pushback so far.
> So the complained-of case is no longer reachable in those branches.
> 
> I think we should reject Aleksander's patch, on the grounds that
> it's now unnecessary --- or if you want to argue that it's still
> necessary, then it's woefully inadequate, because there are surely
> a bunch of other text-processing functions that will also misbehave
> on wrongly-encoded data.  But our general policy for years has been
> that we check incoming text for encoding validity and then presume
> that it is valid in manipulation operations.

pg_upgrade carries forward invalid text.  A presumption of encoding validity
won't be justified any sooner than a presumption of not finding HEAP_MOVED_OFF
flags.  Hence, I think there should exist another policy that text-processing
functions prevent severe misbehavior when processing invalid text.
Out-of-bounds memory access qualifies as severe.

> I'm leaning to the idea that we should not back-patch, because
> this issue has been there for years with few complaints; it's
> not clear that closing the hole is worth creating a compatibility
> hazard in minor releases.

I would not back-patch.

> On the other hand, you could argue
> that we should back-patch so that back-branch charin() will
> understand the strings that can now be emitted by v15 charout().
> Failing to do so will result in a different sort of compatibility
> problem.

If concerned, I'd back-patch enough of the read side only, not the output
side.  I wouldn't bother, though.

Noah Misch <noah@leadboat.com> writes:
> On Thu, Sep 01, 2022 at 03:35:52PM -0400, Tom Lane wrote:
>> I think we should reject Aleksander's patch, on the grounds that
>> it's now unnecessary --- or if you want to argue that it's still
>> necessary, then it's woefully inadequate, because there are surely
>> a bunch of other text-processing functions that will also misbehave
>> on wrongly-encoded data.  But our general policy for years has been
>> that we check incoming text for encoding validity and then presume
>> that it is valid in manipulation operations.

> pg_upgrade carries forward invalid text.  A presumption of encoding validity
> won't be justified any sooner than a presumption of not finding HEAP_MOVED_OFF
> flags.  Hence, I think there should exist another policy that text-processing
> functions prevent severe misbehavior when processing invalid text.
> Out-of-bounds memory access qualifies as severe.

Well ... that sounds great in the abstract, but it's not clear to me
that the problem justifies either the amount of developer effort it'd
take to close all the holes, or the performance hits we'd likely take.
In any case, changing only text_substring() isn't going to move the
ball very far at all.

>> I'm leaning to the idea that we should not back-patch, because
>> this issue has been there for years with few complaints; it's
>> not clear that closing the hole is worth creating a compatibility
>> hazard in minor releases.

> I would not back-patch.

OK.  Let's close out this CF item as RWF, then.

            regards, tom lane