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

[Toon Claes <toon@iotcl.com>]

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?

> +
> +	block_source_from_buf(&source, &data);
> +	cl_assert_equal_i(reftable_block_init(&block, &source, 0, 0, data.len,
> +					      REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_LOG),
> +			  REFTABLE_FORMAT_ERROR);
> +
> +	reftable_block_release(&block);
> +	reftable_buf_release(&data);
> +}

-- 
Cheers,
Toon