blkreftable.c needs more sanity checks
Hi,
It was reported to pgsql-security that it's possible to crash
pg_walsummary by constructing a WAL summary file which purports to
contain chunks of a size greater than MAX_ENTRIES_PER_CHUNK. It may
also be possible to crash the server in some contrived scenario. The
security team does not think this qualifies as a security issue, but
it is a bug. The solution is to insert error checks that catch
out-of-bounds chunk sizes. It turns out that we should have similar
checks for out-of-bounds fork numbers, else similar kinds of bad
things can happen. The attached patch adds those checks, and should be
back-patched to v17 where the WAL summarizer first appeared.
The reason why the impact of this is pretty limited is that WAL
summary files are only supposed to exist in $PGDATA, and if an
adversary has write access to $PGDATA, they will probably do something
much worse than introduce a maliciously-constructed WAL summary file.
As a practical matter, the main benefit of this fix is to contain the
damage in case of an *accidentally* corrupted WAL summary file. Of
course, such a file won't be usable for its intended purpose with or
without this fix, but with this fix, the outcome is more likely to be
an error and less likely to be a crash, either of the server or of
pg_walsummary.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v1-0001-Add-additional-sanity-checks-when-reading-a-blkre.patchapplication/octet-stream; name=v1-0001-Add-additional-sanity-checks-when-reading-a-blkre.patchDownload+21-1
On 14 Jul 2026, at 00:17, Robert Haas <robertmhaas@gmail.com> wrote:
The reason why the impact of this is pretty limited is that WAL
summary files are only supposed to exist in $PGDATA, and if an
adversary has write access to $PGDATA, they will probably do something
much worse than introduce a maliciously-constructed WAL summary file.
Agreed.
As a practical matter, the main benefit of this fix is to contain the
damage in case of an *accidentally* corrupted WAL summary file. Of
course, such a file won't be usable for its intended purpose with or
without this fix, but with this fix, the outcome is more likely to be
an error and less likely to be a crash, either of the server or of
pg_walsummary.
Makes sense, not that the error will be particularly actionable but it will
give more clues than a crash. Looking at the callsites they all call
DestroyBlockRefTableReader on BlockRefTableReaderNextRelation returning false
so leaving chunk_size allocated on error return does not leak.
+ reader->error_callback(reader->error_callback_arg,
+ "file \"%s\" has oversized chunk",
+ reader->error_filename);Any reason not to print the offending chunk size to aid debugging of this type
of should-never-happen scenario?
+1 on the patch and backpatch plan.
--
Daniel Gustafsson
On Tue, Jul 14, 2026 at 3:27 AM Daniel Gustafsson <daniel@yesql.se> wrote:
+ reader->error_callback(reader->error_callback_arg, + "file \"%s\" has oversized chunk", + reader->error_filename);Any reason not to print the offending chunk size to aid debugging of this type
of should-never-happen scenario?
I can if you want, but I think if this happens IRL, you've probably
run pg_walsummary on something that was never intended as a WAL
summary file in the first place, and knowing what random nibble you
interpreted as a chunk size probably isn't going to help anything.
More broadly: To me, this is kind of the epitome of an AI-generated
bug report. It is definitely wrong. It should definitely be fixed. But
if AI hadn't found it, and we'd just waited for someone to discover it
by running into it, I'd guess that it would have taken decades for
that to happen, or maybe it would never have happened. So in some
sense we might be better off not knowing, because fixing this kind of
theoretical bug is taking up a lot of resources right now that we
could be spending on problems that matter more. Fixing this kind of
stuff seems essential at this point if only to prevent the same issues
from being re-reported over and over again and consuming even more
resources, and even if that were no issue, I'm too conscientious to
want to ignore stuff like this once I know about it. At the same time,
the benefit to PostgreSQL users is de minimis.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 14 Jul 2026, at 13:21, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 14, 2026 at 3:27 AM Daniel Gustafsson <daniel@yesql.se> wrote:
+ reader->error_callback(reader->error_callback_arg, + "file \"%s\" has oversized chunk", + reader->error_filename);Any reason not to print the offending chunk size to aid debugging of this type
of should-never-happen scenario?I can if you want, but I think if this happens IRL, you've probably
run pg_walsummary on something that was never intended as a WAL
summary file in the first place, and knowing what random nibble you
interpreted as a chunk size probably isn't going to help anything.
A more likely scenario is that you are hacking in this code and have made silly
errors. It mostly seemed consistent with how the forknum error is reported,
but is not important.
More broadly: To me, this is kind of the epitome of an AI-generated
bug report. It is definitely wrong. It should definitely be fixed. But
if AI hadn't found it, and we'd just waited for someone to discover it
by running into it, I'd guess that it would have taken decades for
that to happen, or maybe it would never have happened. So in some
sense we might be better off not knowing, because fixing this kind of
theoretical bug is taking up a lot of resources right now that we
could be spending on problems that matter more. Fixing this kind of
stuff seems essential at this point if only to prevent the same issues
from being re-reported over and over again and consuming even more
resources, and even if that were no issue, I'm too conscientious to
want to ignore stuff like this once I know about it. At the same time,
the benefit to PostgreSQL users is de minimis.
No disagreement there, which is why I tried to get it reviewed quickly so it
can be applied and forgotten about and we can move on to more important
matters.
--
Daniel Gustafsson
On Tue, Jul 14, 2026 at 7:31 AM Daniel Gustafsson <daniel@yesql.se> wrote:
A more likely scenario is that you are hacking in this code and have made silly
errors. It mostly seemed consistent with how the forknum error is reported,
but is not important.
Upon reflection, it occurred to me that I would probably rather have
the chunk offset than the chunk size if this ever trips, so I added
both to the error message. Here's v2.
No disagreement there, which is why I tried to get it reviewed quickly so it
can be applied and forgotten about and we can move on to more important
matters.
Thanks for reviewing.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v2-0001-Add-additional-sanity-checks-when-reading-a-blkre.patchapplication/octet-stream; name=v2-0001-Add-additional-sanity-checks-when-reading-a-blkre.patchDownload+22-1
On 15 Jul 2026, at 17:36, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 14, 2026 at 7:31 AM Daniel Gustafsson <daniel@yesql.se> wrote:
A more likely scenario is that you are hacking in this code and have made silly
errors. It mostly seemed consistent with how the forknum error is reported,
but is not important.Upon reflection, it occurred to me that I would probably rather have
the chunk offset than the chunk size if this ever trips, so I added
both to the error message. Here's v2.
LGTM, +1 on applying.
--
Daniel Gustafsson
On Wed, Jul 15, 2026 at 11:40 AM Daniel Gustafsson <daniel@yesql.se> wrote:
LGTM, +1 on applying.
Done, thanks.
--
Robert Haas
EDB: http://www.enterprisedb.com