Re: [PATCH v2 06/12] reftable/block: fix OOB write with bogus inflated log size

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jul 03, 2026 at 11:23:37AM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> > index f4e926ce3a..088162483e 100644
> > --- a/t/unit-tests/u-reftable-block.c
> > +++ b/t/unit-tests/u-reftable-block.c
> > @@ -465,3 +465,35 @@ void test_reftable_block__iterator(void)
> >  	reftable_block_release(&block);
> >  	reftable_buf_release(&data);
> >  }
> > +
> > +void test_reftable_block__corrupt_log_block_size(void)
> > +{
> > +	struct reftable_block_source source = { 0 };
> > +	struct reftable_record rec = {
> > +		.type = REFTABLE_BLOCK_TYPE_LOG,
> > +		.u.log = {
> > +			.refname = (char *) "refs/heads/main",
> > +			.update_index = 1,
> > +			.value_type = REFTABLE_LOG_UPDATE,
> > +		},
> > +	};
> > +	struct reftable_block block = { 0 };
> > +	struct reftable_buf data = REFTABLE_BUF_INIT;
> > +
> > +	cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_LOG, &rec, 1);
> > +
> > +	/*
> > +	 * Log blocks store their inflated size as a big-endian 24-bit integer
> > +	 * right after the one-byte block type. Rewrite it to claim a size that
> > +	 * is smaller than the block header.
> > +	 */
> > +	reftable_put_be24((uint8_t *) data.buf + 1, 1);
> 
> Can I suggest to make this test a bit more strict:
> 
> 	/*
> 	 * Log blocks store their inflated size as a big-endian 24-bit integer
> 	 * right after the one-byte block type.
> 	 * First sanity check if the expected value is actually there,
> 	 * then rewrite it to claim a size that is smaller than the block header.
> 	 */
> 	void *p = (uint8_t *) data.buf + 1;
> 	uint32_t block_size = reftable_get_be24(p);
> 	cl_assert_equal_i(block_size, 82);
> 	reftable_put_be24(p, 5);
> 
> So first do a sanity check to see if the current block_size is what we
> expect, this helps us ensure we are actually reading the correct point
> in the buffers. Or does this depend on the architecture?
> 
> Then write a size that's just below what the expected value is. By using
> a value that's just too small, maybe we can avoid off-by-one errors?

That would cause a failure, but it wouldn't catch the out-of-bounds
write that the current test catches. This is because the offset would
still point into the buffer, even though it would point at the wrong
location. The requirement here is that the block size must be smaller
than the header size itself, so it must be between 1 and 3.

So in this case I think it doesn't quite help us to assert the written
block size.

Patrick