[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

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&gt;
| Apple
Podcasts
<https://podcasts.apple.com/us/podcast/legendary-hacker-matt-suiche-on-cyberwar-in-the-age-of-ai/id1056200096?i=1000754809995&gt;*