Re: [PATCH] Harden two-phase GID handling and fork-number validation in WAL replay/decode paths

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