> 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 <[email protected]> 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>*
