| From: | Ana Bobillo <anabobillo3(at)gmail(dot)com> |
|---|---|
| To: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | libpq: Don't read past the end of a message when tracing it |
| Date: | 2026-10-23 13:28:21 |
| Message-ID: | CAJnxHsiCrtervsGsxuEL2cynuRLe9JQyn9poiEH=ja_2GaEvAA@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
Hello,
The functions in src/interfaces/libpq/fe-trace.c that print a traced
protocol message field by field trust the counts and lengths found inside
the message: the length of a DataRow field, the number of fields in a
RowDescription, the zero byte that ends a string, and so on. None of them
is checked against the message's length word, so a malformed message from
the server makes a client that has called PQtrace() read past the end of
that message in its input buffer. Messages that libpq discards without
parsing them (a DataRow with no RowDescription before it, a message of a
type it doesn't expect) are read by the tracer alone, so the checks in
fe-protocol3.c don't cover them. The tracer also takes only -1 as a NULL
DataRow field, while getAnotherTuple() takes any negative length as NULL.
I sent this to security@ first. Alvaro answered that it isn't a
vulnerability, since it needs a hostile server or network, and that it
should go through the normal channels, possibly on master only, so here
it is.
The attached series applies to master (3c5a4a867f2):
0001 gives every reader in fe-trace.c (Byte1, Int16, Int32, String,
Nchar) the end of the message, and they stop there, so no
per-message printer can read past a message whatever it claims. A
message whose contents don't fit is printed as far as it goes and
reported as "mismatched message length: contents exceed length N".
Strings are measured within the message instead of with strlen().
A DataRow field with any negative length is printed as NULL, as
libpq reads the row, and CopyBothResponse reads its count of column
formats as CopyInResponse and CopyOutResponse do. Well-formed
messages are traced as before.
0002 stops tracing a pre-3.0 error message as a protocol 3 message. Such
a message has no length word, and since b8b3f861fbd every message
received during connection startup goes to the tracer, which then
took its first four characters for a length.
0003 adds a test. libpq_trace_regress includes fe-trace.c, as
test-oauth-curl.c includes oauth-curl.c, and traces one message
given on its command line in a buffer of exactly the message's size,
so that AddressSanitizer catches any read past its end;
t/007_trace.pl runs it over well-formed and malformed messages.
On master built with -Db_sanitize=address, 007_trace.pl fails 33 checks,
with heap-buffer-overflow reports in pqTraceOutputNchar,
pqTraceOutputInt32, pqTraceOutputInt16 and in strlen() called from
pqTraceOutputString. With the series all 84 checks pass, and the libpq
and libpq_pipeline suites pass with their traces unchanged.
0001's readers are as old as fe-trace.c (v14); 0002 concerns code that
came with b8b3f861fbd in v18.
This overlaps textually with "Add PQTRACE env to enable protocol tracing
in libpq" (commitfest 7081), which also reworks fe-trace.c. Its
environment variable would let tracing be switched on for any libpq
program, which makes these reads easier to reach. Whichever of the two
lands first, I'll rebase this series onto it as needed.
As an aside, the salt of an AuthenticationMD5Password request is never
traced, so every trace of an MD5 login reports "mismatched message
length: consumed 8, expected 12" for that message. That's a separate
small fix, which I can send in its own thread.
I'll add this to the next commitfest.
Thanks,
Ana Bobillo
| Attachment | Content-Type | Size |
|---|---|---|
| v1-0001-libpq-Don-t-read-past-the-end-of-a-message-when-t.patch | text/plain | 35.2 KB |
| v1-0002-libpq-Don-t-trace-a-pre-3.0-error-message-as-a-pr.patch | text/plain | 1.5 KB |
| v1-0003-libpq-Add-a-test-of-tracing-malformed-protocol-me.patch | text/plain | 10.8 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | CyberNative AI | 2026-10-23 13:28:30 | Re: Skip prefetch for block references that follow a FPW or WILL_INIT of the same block |
| Previous Message | Lucas Jeffrey | 2026-10-23 13:26:27 | Re: Re: [PATCH] Fix segmentation fault caused by reentrancy in RI_Fkey_cascade_del (ri_triggers.c) |