On 2022-Dec-02, Andres Freund wrote:
> Hi,
>
> On 2022-12-02 14:22:55 +0900, Michael Paquier wrote:
> > On Fri, Nov 04, 2022 at 09:52:39AM +0900, Ian Lawrence Barwick wrote:
> > > CommitFest 2022-11 is currently underway, so if you are interested
> > > in moving this patch forward, now would be a good time to update it.
> >
> > No replies after 4 weeks, so I have marked this entry as returned
> > with feedback. I am still wondering what would be the best thing to
> > do here..
>
> IMO this a bugfix, I don't think we can just close the entry, even if Matthias
> doesn't have time / energy to push it forward.
I have created one in the January commitfest,
https://commitfest.postgresql.org/41/
and rebased the patch on current master. (I have not reviewed this.)
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La gente vulgar sólo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"
>From 385ccf1b7d68923009c73db493dbe74be34e305b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvhe...@alvh.no-ip.org>
Date: Mon, 19 Dec 2022 12:26:52 +0100
Subject: [PATCH v9] Add protections in xlog record APIs against large numbers
and overflows.
Before this, it was possible for an extension to create malicious WAL records
that were too large to replay; or that would overflow the xl_tot_len field,
causing potential corruption in WAL record IO ops.
Emitting invalid records was also possible through pg_logical_emit_message(),
which allowed you to emit arbitrary amounts of data up to 2GB, much higher
than the replay limit of approximately 1GB.
This patch adds a limit to the size of an XLogRecord (1020MiB), checks
against overflows, and ensures that the reader infrastructure can read any
max-sized XLogRecords, such that the wal-generating backend will fail when
it attempts to insert unreadable records, instead of that insertion
succeeding but breaking any replication streams.
Author: Matthias van de Meent <boekewurm+postg...@gmail.com>
---
src/backend/access/transam/xloginsert.c | 59 ++++++++++++++++++++++---
src/include/access/xlogrecord.h | 13 ++++++
2 files changed, 65 insertions(+), 7 deletions(-)
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index f811523448..c25431f0e9 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -141,6 +141,17 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info,
bool *topxid_included);
static bool XLogCompressBackupBlock(char *page, uint16 hole_offset,
uint16 hole_length, char *dest, uint16 *dlen);
+static inline void XLogErrorDataLimitExceeded();
+
+/*
+ * Error due to exceeding the maximum size of a WAL record, or registering
+ * more datas than are being accounted for by the XLog infrastructure.
+ */
+static inline void
+XLogErrorDataLimitExceeded()
+{
+ elog(ERROR, "too much WAL data");
+}
/*
* Begin constructing a WAL record. This must be called before the
@@ -352,10 +363,25 @@ XLogRegisterData(char *data, uint32 len)
{
XLogRecData *rdata;
- Assert(begininsert_called);
+ Assert(begininsert_called && XLogRecordLengthIsValid(len));
+
+ /*
+ * Check against max_rdatas; and ensure we don't fill a record with
+ * more data than can be replayed. Records are allocated in one chunk
+ * with some overhead, so ensure XLogRecordLengthIsValid() for that
+ * size of record.
+ *
+ * Additionally, check that we don't accidentally overflow the
+ * intermediate sum value on 32-bit systems by ensuring that the
+ * sum of the two inputs is no less than one of the inputs.
+ */
+ if (num_rdatas >= max_rdatas ||
+#if SIZEOF_SIZE_T == 4
+ mainrdata_len + len < len ||
+#endif
+ !XLogRecordLengthIsValid((size_t) mainrdata_len + (size_t) len))
+ XLogErrorDataLimitExceeded();
- if (num_rdatas >= max_rdatas)
- elog(ERROR, "too much WAL data");
rdata = &rdatas[num_rdatas++];
rdata->data = data;
@@ -391,7 +417,7 @@ XLogRegisterBufData(uint8 block_id, char *data, uint32 len)
registered_buffer *regbuf;
XLogRecData *rdata;
- Assert(begininsert_called);
+ Assert(begininsert_called && len <= UINT16_MAX);
/* find the registered buffer struct */
regbuf = ®istered_buffers[block_id];
@@ -400,14 +426,14 @@ XLogRegisterBufData(uint8 block_id, char *data, uint32 len)
block_id);
/*
- * Check against max_rdatas and ensure we do not register more data per
+ * Check against max_rdatas; and ensure we don't register more data per
* buffer than can be handled by the physical data format; i.e. that
* regbuf->rdata_len does not grow beyond what
* XLogRecordBlockHeader->data_length can hold.
*/
if (num_rdatas >= max_rdatas ||
regbuf->rdata_len + len > UINT16_MAX)
- elog(ERROR, "too much WAL data");
+ XLogErrorDataLimitExceeded();
rdata = &rdatas[num_rdatas++];
@@ -527,6 +553,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecPtr *fpw_lsn, int *num_fpi, bool *topxid_included)
{
XLogRecData *rdt;
+ /*
+ * Overflow of total_len is not normally possible, considering that
+ * this value will be at most 33 RegBufDatas (at UINT16_MAX each)
+ * plus one MaxXLogRecordSize, which together are still an order of
+ * magnitude smaller than UINT32_MAX.
+ */
uint32 total_len = 0;
int block_id;
pg_crc32c rdata_crc;
@@ -535,6 +567,9 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecord *rechdr;
char *scratch = hdr_scratch;
+ /* ensure that any assembled record can be decoded */
+ Assert(AllocSizeIsValid(DecodeXLogRecordRequiredSpace(MaxXLogRecordSize)));
+
/*
* Note: this function can be called multiple times for the same record.
* All the modifications we do to the rdata chains below must handle that.
@@ -766,7 +801,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
{
/*
* When copying to XLogRecordBlockHeader, the length is narrowed
- * to an uint16. Double-check that it is still correct.
+ * to an uint16. We double-check that that is still correct.
*/
Assert(regbuf->rdata_len <= UINT16_MAX);
@@ -872,6 +907,16 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
for (rdt = hdr_rdt.next; rdt != NULL; rdt = rdt->next)
COMP_CRC32C(rdata_crc, rdt->data, rdt->len);
+ /*
+ * Ensure that the XLogRecord is not too large.
+ *
+ * XLogReader machinery is only able to handle records up to a certain
+ * size (ignoring machine resource limitations), so make sure we will
+ * not emit records larger than those sizes we advertise we support.
+ */
+ if (!XLogRecordLengthIsValid(total_len))
+ XLogErrorDataLimitExceeded();
+
/*
* Fill in the fields in the record header. Prev-link is filled in later,
* once we know where in the WAL the record will be inserted. The CRC does
diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h
index 835151ec92..065bc63a79 100644
--- a/src/include/access/xlogrecord.h
+++ b/src/include/access/xlogrecord.h
@@ -54,6 +54,19 @@ typedef struct XLogRecord
#define SizeOfXLogRecord (offsetof(XLogRecord, xl_crc) + sizeof(pg_crc32c))
+/*
+ * XLogReader needs to allocate all the data of an xlog record in a single
+ * chunk. This means that a single XLogRecord cannot exceed MaxAllocSize
+ * in length if we ignore any allocation overhead of the XLogReader.
+ *
+ * To accommodate some overhead, hhis MaxXLogRecordSize value allows for
+ * 4MB -1b of allocation overhead in anything that allocates xlog record
+ * data, which should be enough for effectively all purposes.
+ */
+#define MaxXLogRecordSize (1020 * 1024 * 1024)
+
+#define XLogRecordLengthIsValid(len) ((len) >= 0 && (len) < MaxXLogRecordSize)
+
/*
* The high 4 bits in xl_info may be used freely by rmgr. The
* XLR_SPECIAL_REL_UPDATE and XLR_CHECK_CONSISTENCY bits can be passed by
--
2.30.2
