reftable/block: fix OOB read with bogus block size

The block size is read from the block header, which is untrusted data.
We use it without verification to access the restart count at the end of
the block as well as to compute the restart table offset. With a bogus
block size that exceeds the data we have actually read this can lead to
an out-of-bounds read:

  ==1458284==ERROR: AddressSanitizer: SEGV on unknown address 0x7d8ff7de4b7d (pc 0x55555598c339 bp 0x7fffffff4ef0 sp 0x7fffffff4eb0 T0)
  ==1458284==The signal is caused by a READ memory access.
      #0 0x55555598c339 in reftable_get_be16 ./build/../reftable/basics.h:118:9
      #1 0x55555598bee2 in reftable_block_init ./build/../reftable/block.c:344:18
      #2 0x555555813e0e in test_reftable_block__corrupt_block_size ./build/../t/unit-tests/u-reftable-block.c:540:8
      #3 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3
      #4 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3
      #5 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4
      #6 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11
      #7 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8
      #8 0x55555584b55a in main ./build/../common-main.c:9:11
      #9 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
      #10 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
      #11 0x555555694c24 in _start (./build/t/unit-tests+0x140c24)

  ==1458284==Register values:
  rax = 0x00007d8ff7de4b7d  rbx = 0x00007fffffff4f00  rcx = 0x0000000000000006  rdx = 0x0000000000000010
  rdi = 0x00007d8ff7de4b7d  rsi = 0x00007bfff5cf0420  rbp = 0x00007fffffff4ef0  rsp = 0x00007fffffff4eb0
   r8 = 0x00000f807eb960b8   r9 = 0x0000000000000001  r10 = 0x00007bfff5cf05e7  r11 = 0x000000000000000f
  r12 = 0x00007fffffff58f8  r13 = 0x0000000000000001  r14 = 0x0000555555ee8160  r15 = 0x0000000000000000
  AddressSanitizer can not provide additional info.

Verify that the claimed block size fits into the block data before using
it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/reftable/block.c b/reftable/block.c
index b86cb9ec5ad5bc9b032e06d320e1a4ac59db38e8..4d6b11c2e7e660d52f206845a84359f58946a3e8 100644 (file)
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -340,6 +340,15 @@ int reftable_block_init(struct reftable_block *block,
                full_block_size = block_size;
        }
 
+       /*
+        * Ensure that we have sufficient data available now to satisfy the
+        * claimed block size.
+        */
+       if (block_size > block->block_data.len) {
+               err = REFTABLE_FORMAT_ERROR;
+               goto done;
+       }
+
        restart_count = reftable_get_be16(block->block_data.data + block_size - 2);
        restart_off = block_size - 2 - 3 * restart_count;
 

diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index 40274af5c0ce67eabfe57fde9ebbbbaccb0fcbbe..1f35aed91ae5de0418ff432e2da21f17761ab099 100644 (file)
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -500,3 +500,48 @@ void test_reftable_block__corrupt_log_block_size(void)
        block_writer_release(&writer);
        reftable_buf_release(&data);
 }
+
+void test_reftable_block__corrupt_block_size(void)
+{
+       struct reftable_block_source source = { 0 };
+       struct block_writer writer = {
+               .last_key = REFTABLE_BUF_INIT,
+       };
+       struct reftable_record rec = {
+               .type = REFTABLE_BLOCK_TYPE_REF,
+               .u.ref = {
+                       .value_type = REFTABLE_REF_VAL1,
+                       .refname = (char *) "refs/heads/main",
+               },
+       };
+       struct reftable_block block = { 0 };
+       struct reftable_buf data;
+
+       data.len = 1024;
+       REFTABLE_CALLOC_ARRAY(data.buf, data.len);
+       cl_assert(data.buf != NULL);
+
+       cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_REF,
+                                      (uint8_t *) data.buf, data.len,
+                                      0, hash_size(REFTABLE_HASH_SHA1)));
+       cl_must_pass(block_writer_add(&writer, &rec));
+       cl_assert(block_writer_finish(&writer) > 0);
+
+       /*
+        * The block size is stored as a big-endian 24-bit integer right after
+        * the one-byte block type at the start of the block. Corrupt it to
+        * claim a size that is larger than the data we actually have. Reading
+        * the restart count and restart table relative to such a bogus block
+        * size must not access out-of-bounds memory.
+        */
+       reftable_put_be24((uint8_t *) data.buf + 1, 0xffffff);
+
+       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_REF),
+                         REFTABLE_FORMAT_ERROR);
+
+       reftable_block_release(&block);
+       block_writer_release(&writer);
+       reftable_buf_release(&data);
+}