reftable/block: fix OOB read with bogus restart offset

Restart points encode records in a given block that do not use prefix
compression and that can thus immediately be seeked to. These offsets
are encoded in the restart table, where each offset needs to point at
one of the records of the block. We do not verify this though, so a
bogus restart offset may cause an out-of-bounds read:

  ==1472280==ERROR: AddressSanitizer: SEGV on unknown address 0x7d8ff7de5f7f (pc 0x55555599502b bp 0x7fffffff4df0 sp 0x7fffffff4d40 T0)
  ==1472280==The signal is caused by a READ memory access.
      #0 0x55555599502b in get_var_int ./git/build/../reftable/record.c:30:6
      #1 0x555555995c2a in reftable_decode_keylen ./git/build/../reftable/record.c:177:6
      #2 0x55555598e85c in restart_needle_less ./git/build/../reftable/block.c:455:6
      #3 0x55555598895f in binsearch ./git/build/../reftable/basics.c:175:9
      #4 0x55555598e189 in block_iter_seek_key ./git/build/../reftable/block.c:543:6
      #5 0x555555814aee in test_reftable_block__corrupt_restart_offset ./git/build/../t/unit-tests/u-reftable-block.c:636:20
      #6 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3
      #7 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3
      #8 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4
      #9 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11
      #10 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8
      #11 0x55555584c25a in main ./git/build/../common-main.c:9:11
      #12 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
      #13 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
      #14 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24)

  ==1472280==Register values:
  rax = 0x00007d8ff7de5f7f  rbx = 0x00007fffffff4e00  rcx = 0x00007d8ff7de5f80  rdx = 0x00007bfff5b6af60
  rdi = 0x00007bfff5b6af40  rsi = 0x00007bfff592dfa0  rbp = 0x00007fffffff4df0  rsp = 0x00007fffffff4d40
   r8 = 0x00000000ff00002b   r9 = 0x00007d8ff7de5f7f  r10 = 0x00000f7ffeb25bf0  r11 = 0xf3f30000f1f1f1f1
  r12 = 0x00007fffffff58f8  r13 = 0x0000000000000001  r14 = 0x00007ffff7ffd000  r15 = 0x0000555556055fd0
  AddressSanitizer can not provide additional info.
  SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/record.c:30:6 in get_var_int

Guard against such restart offsets and signal an error to the caller via
`args.error`.

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 89efce875149525c3a7e0bb161b689db44f452f3..1fa81405d2680fc7ad6ecea8e80dcb052fd14a1e 100644 (file)
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -440,6 +440,15 @@ static int restart_needle_less(size_t idx, void *_args)
        uint8_t extra;
        int n;
 
+       /*
+        * The restart offset must point to a record, which is stored before
+        * the restart table. Verify that this is the case.
+        */
+       if (off >= args->block->restart_off) {
+               args->error = 1;
+               return -1;
+       }
+
        /*
         * Records at restart points are stored without prefix compression, so
         * there is no need to fully decode the record key here. This removes

diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index ba410a0885230c747541d218738c02f2cb424bea..77a054d082c409f73cb509f90dd58ec3b47b8fa6 100644 (file)
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -591,3 +591,54 @@ void test_reftable_block__corrupt_restart_count(void)
        block_writer_release(&writer);
        reftable_buf_release(&data);
 }
+
+void test_reftable_block__corrupt_restart_offset(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 block_iter it = BLOCK_ITER_INIT;
+       struct reftable_buf want = REFTABLE_BUF_INIT;
+       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);
+
+       block_source_from_buf(&source, &data);
+       cl_must_pass(reftable_block_init(&block, &source, 0, 0, data.len,
+                                        REFTABLE_HASH_SIZE_SHA1, REFTABLE_BLOCK_TYPE_REF));
+
+       /*
+        * Corrupt the first restart offset, stored as a big-endian 24-bit
+        * integer at the start of the restart table, to point past the end of
+        * the records section. Seeking such a block must fail gracefully.
+        */
+       reftable_put_be24((uint8_t *) block.block_data.data + block.restart_off,
+                         0xffffff);
+
+       block_iter_init(&it, &block);
+       cl_must_pass(reftable_buf_addstr(&want, "refs/heads/main"));
+       cl_assert_equal_i(block_iter_seek_key(&it, &want), REFTABLE_FORMAT_ERROR);
+
+       reftable_buf_release(&want);
+       block_iter_close(&it);
+       reftable_block_release(&block);
+       block_writer_release(&writer);
+       reftable_buf_release(&data);
+}