[PATCH] Harden two-phase GID handling and fork-number validation in WAL replay/decode paths
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
This thread has been committed, so CI has stopped here. Anything below is the last result it produced.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253210psql -h localhost -U postgresBuilt from patchset v1 (message #1), July 28, 2026 at 09:53 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253210_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253210_1 && git checkout t253210_1Patchset v1 (message #1) is on t253210_1
Hello,
While fuzzing WAL replay and decode paths on master, we found that the
two-phase redo code and several fork-number consumers trust
record-supplied values that can, in crafted or corrupted WAL, exceed
the bounds the rest of the code assumes. The write-time path already
rejects oversized two-phase GIDs (twophase.c:371), but the replay and
restore paths do not re-validate, and `gxact->gid` (GIDSIZE bytes) is
filled with plain `strcpy`. Similarly, fork numbers from records index
the 4-entry `forkNames[]` without range checks on some paths.
Per a suggestion on the security list (where the WAL-trust posture was
noted, but hardening the two-phase replay code was encouraged), here
are two small patches:
- **0001-Validate-two-phase-GID-length-in-replay-paths.patch**
Reject `gidlen == 0 || gidlen >= GIDSIZE` in `PrepareRedoAdd()`,
`RecoverPreparedTransactions()` (state-file restore), and
`ParsePrepareRecord()` (the same pattern into a stack buffer in
xactdesc.c), and replace the `strcpy` calls with `strlcpy`.
- **0002-Validate-fork-numbers-in-WAL-decode-paths.patch**
Range-check the block-header fork nibble in `DecodeXLogRecord()`
(`report_invalid_record`, consistent with neighbouring checks), and
clamp defensively in `GetRelationPath()`, which is shared
frontend/backend code and the common sink for `forkNames[]`
indexing (e.g. from `xl_smgr_create.forkNum`).
Without these checks, a malformed `XLOG_XACT_PREPARE` record overflows
`gxact->gid` into neighbouring two-phase shared memory during redo
(reproduced: multi-KB overwrite, freelist corruption), and an
out-of-range fork number reads outside `forkNames[]`. With them, both
inputs are rejected cleanly at replay/decode time (in redo, the ERROR
is promoted to FATAL, aborting recovery rather than corrupting state).
Verified against master (38afc3dcb): the tree builds cleanly,
`PREPARE TRANSACTION` / `COMMIT PREPARED` / recovery of prepared
transactions work normally, and crafted records that previously
corrupted shared memory are now rejected with the new error messages.
Best regards,
--
*Matt Suiche*
*Tolmo, Inc.*
Attachments:
t253210_10001-Validate-two-phase-GID-length-in-replay-paths.patchapplication/octet-stream; name=0001-Validate-two-phase-GID-length-in-replay-paths.patchDownload+37-2
0002-Validate-fork-numbers-in-WAL-decode-paths.patchapplication/octet-stream; name=0002-Validate-fork-numbers-in-WAL-decode-paths.patchDownload+17-0
On Mon, Jul 27, 2026 at 10:06:55AM +0200, Matt Suiche wrote:
Per a suggestion on the security list (where the WAL-trust posture was
noted, but hardening the two-phase replay code was encouraged), here
are two small patches:
Because WAL is trusted data. :)
- **0001-Validate-two-phase-GID-length-in-replay-paths.patch**
Reject `gidlen == 0 || gidlen >= GIDSIZE` in `PrepareRedoAdd()`,
`RecoverPreparedTransactions()` (state-file restore), and
`ParsePrepareRecord()` (the same pattern into a stack buffer in
xactdesc.c), and replace the `strcpy` calls with `strlcpy`.
I can get behind the two strcpy() -> strlcpy() switches in
MarkAsPreparingGuts() and PrepareRedoAdd() on the ground that it is
going to silence LLMs and static fuzzers. Once we do that, the gidlen
checks don't really matter; we can just drop them.
The ParsePrepareRecord() check is a bit more debatable to have, but
rather than having a check, I think that we should just switch the
strncpy() to a strlcpy() bounded by GIDSIZE and call it a day. This
would offer the same protection, keep all code paths in line, and
avoid including an invasive logging.h for the sake of a defensive
check (aka I really don't want this level of dependency, WAL desc
files gain in portability with less dependencies).
- **0002-Validate-fork-numbers-in-WAL-decode-paths.patch**
Range-check the block-header fork nibble in `DecodeXLogRecord()`
(`report_invalid_record`, consistent with neighbouring checks), and
clamp defensively in `GetRelationPath()`, which is shared
frontend/backend code and the common sink for `forkNames[]`
indexing (e.g. from `xl_smgr_create.forkNum`).
+ if (blk->forknum > MAX_FORKNUM)
+ {
+ report_invalid_record(state,
+ "invalid fork number %u at %X/%08X",
+ blk->forknum,
+ LSN_FORMAT_ARGS(state->ReadRecPtr));
+ goto err;
+ }Hmm. Why not. We have similar checks.
+ /*
+ * WAL decode/display paths can reach here with unvalidated fork
+ * numbers; clamp as defense in depth so we never index forkNames[]
+ * out of bounds. Callers that can ereport should validate first.
+ */
+ if (forkNumber < MAIN_FORKNUM || forkNumber > MAX_FORKNUM)
+ forkNumber = MAIN_FORKNUM;The change is a bad idea to me. It means that an incorrect record
(which would not really happen due to CRC check anyway) would now rely
on an incorrect context.
Without these checks, a malformed `XLOG_XACT_PREPARE` record overflows
`gxact->gid` into neighbouring two-phase shared memory during redo
(reproduced: multi-KB overwrite, freelist corruption), and an
out-of-range fork number reads outside `forkNames[]`. With them, both
inputs are rejected cleanly at replay/decode time (in redo, the ERROR
is promoted to FATAL, aborting recovery rather than corrupting state).
Note: none of that is worth a backpatch. These are just additional
defenses. The strlcpy() changes and the DecodeXLogRecord() are OK,
but let's drop the rest.
--
Michael
Because WAL is trusted data. :)
Okay :)
Makes sense to me.
strcpy -> strlcpy is a pretty big win in general
On Tue, Jul 28, 2026 at 8:47 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jul 27, 2026 at 10:06:55AM +0200, Matt Suiche wrote:
Per a suggestion on the security list (where the WAL-trust posture was
noted, but hardening the two-phase replay code was encouraged), here
are two small patches:Because WAL is trusted data. :)
- **0001-Validate-two-phase-GID-length-in-replay-paths.patch**
Reject `gidlen == 0 || gidlen >= GIDSIZE` in `PrepareRedoAdd()`,
`RecoverPreparedTransactions()` (state-file restore), and
`ParsePrepareRecord()` (the same pattern into a stack buffer in
xactdesc.c), and replace the `strcpy` calls with `strlcpy`.I can get behind the two strcpy() -> strlcpy() switches in
MarkAsPreparingGuts() and PrepareRedoAdd() on the ground that it is
going to silence LLMs and static fuzzers. Once we do that, the gidlen
checks don't really matter; we can just drop them.The ParsePrepareRecord() check is a bit more debatable to have, but
rather than having a check, I think that we should just switch the
strncpy() to a strlcpy() bounded by GIDSIZE and call it a day. This
would offer the same protection, keep all code paths in line, and
avoid including an invasive logging.h for the sake of a defensive
check (aka I really don't want this level of dependency, WAL desc
files gain in portability with less dependencies).- **0002-Validate-fork-numbers-in-WAL-decode-paths.patch**
Range-check the block-header fork nibble in `DecodeXLogRecord()`
(`report_invalid_record`, consistent with neighbouring checks), and
clamp defensively in `GetRelationPath()`, which is shared
frontend/backend code and the common sink for `forkNames[]`
indexing (e.g. from `xl_smgr_create.forkNum`).+ if (blk->forknum > MAX_FORKNUM) + { + report_invalid_record(state, + "invalid fork number %u at %X/%08X", + blk->forknum, + LSN_FORMAT_ARGS(state->ReadRecPtr)); + goto err; + }Hmm. Why not. We have similar checks.
+ /* + * WAL decode/display paths can reach here with unvalidated fork + * numbers; clamp as defense in depth so we never index forkNames[] + * out of bounds. Callers that can ereport should validate first. + */ + if (forkNumber < MAIN_FORKNUM || forkNumber > MAX_FORKNUM) + forkNumber = MAIN_FORKNUM;The change is a bad idea to me. It means that an incorrect record
(which would not really happen due to CRC check anyway) would now rely
on an incorrect context.Without these checks, a malformed `XLOG_XACT_PREPARE` record overflows
`gxact->gid` into neighbouring two-phase shared memory during redo
(reproduced: multi-KB overwrite, freelist corruption), and an
out-of-range fork number reads outside `forkNames[]`. With them, both
inputs are rejected cleanly at replay/decode time (in redo, the ERROR
is promoted to FATAL, aborting recovery rather than corrupting state).Note: none of that is worth a backpatch. These are just additional
defenses. The strlcpy() changes and the DecodeXLogRecord() are OK,
but let's drop the rest.
--
Michael
--
*Matt Suiche*
Calendly: https://www.calendly.com/msuiche
Signal: +1-415-466-5067
*Bloomberg Odd Lots - *Cyberwar in the Age of AI: *Spotify
<https://open.spotify.com/episode/08KYvhIBPmqxO37C1mmaph?si=6b5b363d86cc452f>
| Apple
Podcasts
<https://podcasts.apple.com/us/podcast/legendary-hacker-matt-suiche-on-cyberwar-in-the-age-of-ai/id1056200096?i=1000754809995>*