On Mon, Jul 11, 2022 at 02:26:46PM +0200, Matthias van de Meent wrote: > Thanks for reviewing.
I think that v6 is over-engineered because there should be no need to
add a check in xlogreader.c as long as the origin of the problem is
blocked, no? And the origin here is when the record is assembled. At
least this is the cleanest solution for HEAD, but not in the
back-branches if we'd care about doing something with records already
generated, and I am not sure that we need to care about other things
than HEAD, TBH. So it seems to me that there is no need to create a
XLogRecMaxLength which is close to a duplicate of
DecodeXLogRecordRequiredSpace().
@@ -519,7 +549,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
XLogRecPtr *fpw_lsn, int *num_fpi, bool *topxid_included)
{
XLogRecData *rdt;
- uint32 total_len = 0;
+ uint64 total_len = 0;
This has no need to change.
My suggestion from upthread was close to what you proposed, but I had
in mind something simpler, as of:
+ /*
+ * Ensure that xlogreader.c can read the record.
+ */
+ if (unlikely(!AllocSizeIsValid(DecodeXLogRecordRequiredSpace(total_len))))
+ elog(ERROR, "too much WAL data");
This would be the amount of data allocated by the WAL reader when it
is possible to allocate an oversized record, related to the business
of the circular buffer depending on if the read is blocking or not.
Among the two problems to solve at hand, the parts where the APIs are
changed and made more robust with unsigned types and where block data
is not overflowed with its 16-byte limit are committable, so I'd like
to do that first (still need to check its performance with some micro
benchmark on XLogRegisterBufData()). The second part to block the
creation of the assembled record is simpler, now
DecodeXLogRecordRequiredSpace() would make the path a bit hotter,
though we could inline it in the worst case?
--
Michael
From 2031f15fe90c94deb21d4e41b717ada9a8bdb520 Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postg...@gmail.com>
Date: Mon, 20 Jun 2022 10:21:22 +0200
Subject: [PATCH v7] 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 1GB.
---
src/include/access/xloginsert.h | 4 ++--
src/backend/access/transam/xloginsert.c | 30 ++++++++++++++++++++-----
2 files changed, 27 insertions(+), 7 deletions(-)
diff --git a/src/include/access/xloginsert.h b/src/include/access/xloginsert.h
index c04f77b173..aed4643d1c 100644
--- a/src/include/access/xloginsert.h
+++ b/src/include/access/xloginsert.h
@@ -43,12 +43,12 @@ extern void XLogBeginInsert(void);
extern void XLogSetRecordFlags(uint8 flags);
extern XLogRecPtr XLogInsert(RmgrId rmid, uint8 info);
extern void XLogEnsureRecordSpace(int max_block_id, int ndatas);
-extern void XLogRegisterData(char *data, int len);
+extern void XLogRegisterData(char *data, uint32 len);
extern void XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags);
extern void XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator,
ForkNumber forknum, BlockNumber blknum, char *page,
uint8 flags);
-extern void XLogRegisterBufData(uint8 block_id, char *data, int len);
+extern void XLogRegisterBufData(uint8 block_id, char *data, uint32 len);
extern void XLogResetInsertion(void);
extern bool XLogCheckBufferNeedsBackup(Buffer buffer);
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index f3c29fa909..3788429b70 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -348,13 +348,13 @@ XLogRegisterBlock(uint8 block_id, RelFileLocator *rlocator, ForkNumber forknum,
* XLogRecGetData().
*/
void
-XLogRegisterData(char *data, int len)
+XLogRegisterData(char *data, uint32 len)
{
XLogRecData *rdata;
Assert(begininsert_called);
- if (num_rdatas >= max_rdatas)
+ if (unlikely(num_rdatas >= max_rdatas))
elog(ERROR, "too much WAL data");
rdata = &rdatas[num_rdatas++];
@@ -386,7 +386,7 @@ XLogRegisterData(char *data, int len)
* limited)
*/
void
-XLogRegisterBufData(uint8 block_id, char *data, int len)
+XLogRegisterBufData(uint8 block_id, char *data, uint32 len)
{
registered_buffer *regbuf;
XLogRecData *rdata;
@@ -399,8 +399,16 @@ XLogRegisterBufData(uint8 block_id, char *data, int len)
elog(ERROR, "no block with id %d registered with WAL insertion",
block_id);
- if (num_rdatas >= max_rdatas)
+ /*
+ * Check against max_rdatas and ensure we do not 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 (unlikely(num_rdatas >= max_rdatas) ||
+ unlikely(regbuf->rdata_len + len > UINT16_MAX))
elog(ERROR, "too much WAL data");
+
rdata = &rdatas[num_rdatas++];
rdata->data = data;
@@ -756,12 +764,18 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
if (needs_data)
{
+ /*
+ * When copying to XLogRecordBlockHeader, the length is narrowed
+ * to an uint16. We double-check that that is still correct.
+ */
+ Assert(regbuf->rdata_len <= UINT16_MAX);
+
/*
* Link the caller-supplied rdata chain for this buffer to the
* overall list.
*/
bkpb.fork_flags |= BKPBLOCK_HAS_DATA;
- bkpb.data_length = regbuf->rdata_len;
+ bkpb.data_length = (uint16) regbuf->rdata_len;
total_len += regbuf->rdata_len;
rdt_datas_last->next = regbuf->rdata_head;
@@ -858,6 +872,12 @@ 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 xlogreader.c can read the record.
+ */
+ if (unlikely(!AllocSizeIsValid(DecodeXLogRecordRequiredSpace(total_len))))
+ elog(ERROR, "too much WAL data");
+
/*
* 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
--
2.36.1
signature.asc
Description: PGP signature
