patch, 11 partsreftable: harden against corrupted tables
48 messages between Jun 24, 2026 and Jul 8, 2026, from Patrick Steinhardt, Christian Couder, Junio C Hamano, Toon Claes.
Plain Markdown or JSON for tools and agents. Diffs are folded; open one to read it.
Hi,
this patch series addresses a bunch of errors that may happen when trying to read corrupted tables. These errors include out-of-bounds writes, out-of-bounds reads and the ability to hit abort(3p) calls.
The out-of-bounds write was originally reported by awo on the security mailing list. As we never transfer reftables over the protocol it would require local disk access to create such corrupted reftables, so there isn't really an easy way to exploit these.
In any case, I took that chance and wrote a fuzzer for parsing the tables, which surfaced a bunch of issues. At the end of this series though the fuzzer can now run for an extended amount of time (2hrs+) without surfacing any new issues.
Thanks!
Patrick
---
Patrick Steinhardt (11):
meson: support building fuzzers with libFuzzer
oss-fuzz: add fuzzer for parsing reftables
reftable/basics: fix OOB read on binary search of empty range
reftable/record: don't abort when decoding invalid ref value type
reftable/block: fix OOB write with bogus inflated log size
reftable/block: fix OOB read with bogus block size
reftable/block: fix OOB read with bogus restart count
reftable/block: fix use of uninitialized memory when binsearch fails
reftable/block: fix OOB read with bogus restart offset
reftable/table: fix NULL pointer access when seeking to bogus offsets
reftable/table: fix OOB read on truncated table Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
meson.build | 15 +++
meson_options.txt | 2 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 ++++++++++++++
oss-fuzz/meson.build | 2 +
reftable/basics.c | 3 +
reftable/block.c | 39 +++++++-
reftable/record.c | 6 +-
reftable/table.c | 7 ++
t/unit-tests/u-reftable-basics.c | 11 +++
t/unit-tests/u-reftable-block.c | 186 ++++++++++++++++++++++++++++++++++++
t/unit-tests/u-reftable-record.c | 24 +++++
t/unit-tests/u-reftable-table.c | 91 ++++++++++++++++++
15 files changed, 456 insertions(+), 7 deletions(-)
--- base-commit: ab776a62a78576513ee121424adb19597fbb7613 change-id: 20260623-pks-reftable-hardening-f54de69fea63
[PATCH 01/11] meson: support building fuzzers with libFuzzer
To support fuzzing via libFuzzer one has to pass a couple of compiler options:
- It is mandatory to enable the "fuzzer-no-link" sanitizer for
coverage feedback. - It is recommended to enable at least one more sanitizer to catch
issues, like the "address" sanitizer. - The fuzzing executables need to be linked with "-fsanitize=fuzzer"
to wire up libFuzzer itself.The first two items can already be achieved via the "-Db_sanitize=" option. But the last item cannot easily be achieved, as we can only configure global link arguments.
Introduce a new "-Dfuzzers_link_args=" build option to plug this gap. Add documentation so that users know how to set up libFuzzer.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
meson.build | 15 +++++++++++++++
meson_options.txt | 2 ++
oss-fuzz/meson.build | 1 +
3 files changed, 18 insertions(+)
Show changes to 3 files +18 −0
meson.build, meson_options.txt, oss-fuzz/meson.build
diff --git a/meson.build b/meson.build
index 3247697f74..9df6fbb0a5 100644
--- a/meson.build
+++ b/meson.build
@@ -161,6 +161,21 @@
# These machine files can be passed to `meson setup` via the `--native-file`
# option.
#
+# Fuzzing
+# =======
+#
+# Meson supports building the fuzzing targets by setting `-Dfuzzers=true`. By
+# default, the targets will be built without libFuzzer and thus won't be usable
+# for fuzzing. You have to configure a couple of options to properly wire up
+# libFuzzer:
+#
+# $ meson setup build-fuzzers \
+# -Db_sanitize=address,fuzzer-no-link \
+# -Dfuzzers=true \
+# -Dfuzzers_link_args=-fsanitize=fuzzer
+# $ meson compile -C build-fuzzers
+# $ ./build-fuzzers/oss-fuzz/fuzz-config <args>
+#
# Cross compilation
# =================
#
diff --git a/meson_options.txt b/meson_options.txt
index d936ada098..dc88f130d7 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -131,3 +131,5 @@ option('test_utf8_locale', type: 'string',
description: 'Name of a UTF-8 locale used for testing.')
option('fuzzers', type: 'boolean', value: false,
description: 'Enable building fuzzers.')
+option('fuzzers_link_args', type: 'array', value: [],
+ description: 'Linker arguments used to link fuzzers. Use -fsanitize=fuzzer for fuzzing.')
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 878afd8426..10bcac2f6d 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -16,5 +16,6 @@ foreach fuzz_program : fuzz_programs
fuzz_program,
],
dependencies: [libgit_commonmain],
+ link_args: get_option('fuzzers_link_args'),
)
endforeach
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 02/11] oss-fuzz: add fuzzer for parsing reftables
Add a new fuzzer that exercises our parsing of reftables. Fallout from this fuzzer will be fixed over subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 +++++++++++++++++++++++++++++++++++++
oss-fuzz/meson.build | 1 +
5 files changed, 78 insertions(+)
Show changes to 5 files +78 −0
Makefile, ci/run-build-and-minimal-fuzzers.sh, oss-fuzz/.gitignore, oss-fuzz/fuzz-reftable.c, oss-fuzz/meson.build
diff --git a/Makefile b/Makefile
index 1cec251f43..89d3edd5ea 100644
--- a/Makefile
+++ b/Makefile
@@ -2599,6 +2599,7 @@ FUZZ_OBJS += oss-fuzz/fuzz-date.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
FUZZ_OBJS += oss-fuzz/fuzz-parse-attr-line.o
+FUZZ_OBJS += oss-fuzz/fuzz-reftable.o
FUZZ_OBJS += oss-fuzz/fuzz-url-decode-mem.o
.PHONY: fuzz-objs
fuzz-objs: $(FUZZ_OBJS)
diff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh
index e7b97952e7..37b24b092d 100755
--- a/ci/run-build-and-minimal-fuzzers.sh
+++ b/ci/run-build-and-minimal-fuzzers.sh
@@ -21,6 +21,7 @@ date
pack-headers
pack-idx
parse-attr-line
+reftable
url-decode-mem
"
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index f2d74de457..dc7a127a62 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -5,4 +5,5 @@ fuzz-date
fuzz-pack-headers
fuzz-pack-idx
fuzz-parse-attr-line
+fuzz-reftable
fuzz-url-decode-mem
diff --git a/oss-fuzz/fuzz-reftable.c b/oss-fuzz/fuzz-reftable.c
new file mode 100644
index 0000000000..c46eac2c6b
--- /dev/null
+++ b/oss-fuzz/fuzz-reftable.c
@@ -0,0 +1,74 @@
+#include "git-compat-util.h"
+#include "reftable/basics.h"
+#include "reftable/blocksource.h"
+#include "reftable/reftable-blocksource.h"
+#include "reftable/reftable-error.h"
+#include "reftable/reftable-iterator.h"
+#include "reftable/reftable-record.h"
+#include "reftable/reftable-table.h"
+#include "reftable/reftable-writer.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ struct reftable_block_source source = { 0 };
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ struct reftable_table *table = NULL;
+ int err;
+
+ if (reftable_buf_add(&buf, (const char *)data, size) < 0)
+ goto out;
+ block_source_from_buf(&source, &buf);
+
+ err = reftable_table_new(&table, &source, "fuzz-input");
+ if (err < 0)
+ goto out;
+
+ /*
+ * Exercise the ref, log and raw block iterators so that we cover as
+ * much of the parsing code as possible.
+ */
+ {
+ struct reftable_ref_record ref = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_ref_iterator(table, &it);
+ if (!reftable_iterator_seek_ref(&it, ""))
+ while (!reftable_iterator_next_ref(&it, &ref))
+ ;
+
+ reftable_ref_record_release(&ref);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_log_iterator(table, &it);
+ if (!reftable_iterator_seek_log(&it, ""))
+ while (!reftable_iterator_next_log(&it, &log))
+ ;
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_table_iterator it = { 0 };
+ const struct reftable_block *block;
+
+ if (!reftable_table_iterator_init(&it, table))
+ while (!reftable_table_iterator_next(&it, &block))
+ ;
+
+ reftable_table_iterator_release(&it);
+ }
+
+out:
+ if (table)
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+ return 0;
+}
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 10bcac2f6d..5a3854256b 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -6,6 +6,7 @@ fuzz_programs = [
'fuzz-pack-headers.c',
'fuzz-pack-idx.c',
'fuzz-parse-attr-line.c',
+ 'fuzz-reftable.c',
'fuzz-url-decode-mem.c',
]
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 03/11] reftable/basics: fix OOB read on binary search of empty range
`binsearch()` performs a binary search over a range of `sz` elements by repeatedly calling the comparison function with indices into that range. When the range is empty though, there is no valid index to call the comparison function with. We still end up executing the comparison function though with an index of 0, which of course will cause an out-of-bounds read.
Return early when the range is empty.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/basics.c | 3 +++
t/unit-tests/u-reftable-basics.c | 11 +++++++++++
2 files changed, 14 insertions(+)
Show changes to 2 files +14 −0
reftable/basics.c, t/unit-tests/u-reftable-basics.c
diff --git a/reftable/basics.c b/reftable/basics.c
index e969927b61..f0442a46cf 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -152,6 +152,9 @@ size_t binsearch(size_t sz, int (*f)(size_t k, void *args), void *args)
size_t lo = 0;
size_t hi = sz;
+ if (!sz)
+ return 0;
+
/* Invariants:
*
* (hi == sz) || f(hi) == true
diff --git a/t/unit-tests/u-reftable-basics.c b/t/unit-tests/u-reftable-basics.c
index 73566ed0eb..c5d83b6714 100644
--- a/t/unit-tests/u-reftable-basics.c
+++ b/t/unit-tests/u-reftable-basics.c
@@ -60,6 +60,17 @@ void test_reftable_basics__binsearch(void)
}
}
+static int unreachable_lesseq(size_t i UNUSED, void *args UNUSED)
+{
+ cl_fail("comparison function called for empty range");
+ return 0;
+}
+
+void test_reftable_basics__binsearch_empty(void)
+{
+ cl_assert_equal_i(binsearch(0, &unreachable_lesseq, NULL), 0);
+}
+
void test_reftable_basics__names_length(void)
{
const char *a[] = { "a", "b", NULL };
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 04/11] reftable/record: don't abort when decoding invalid ref value type
When decoding a ref record we read its value type from the block. In case the type itself is invalid we call `abort()`. This is rather heavy-handed though: the data we're reading is untrusted, so we should treat the issue as a normal and not as a programming error.
Fix this by handling the error gracefully. Note that this also requires us to set the value type later, as otherwise we might store an invalid type in the record.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 6 +++---
t/unit-tests/u-reftable-record.c | 24 ++++++++++++++++++++++++
2 files changed, 27 insertions(+), 3 deletions(-)
Show changes to 2 files +27 −3
reftable/record.c, t/unit-tests/u-reftable-record.c
diff --git a/reftable/record.c b/reftable/record.c
index fcd387ba5d..1fce441930 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -388,7 +388,6 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
r->refname[key.len] = 0;
r->update_index = update_index;
- r->value_type = val_type;
switch (val_type) {
case REFTABLE_REF_VAL1:
if (in.len < hash_size) {
@@ -426,9 +425,10 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
case REFTABLE_REF_DELETION:
break;
default:
- abort();
- break;
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
}
+ r->value_type = val_type;
return start.len - in.len;
diff --git a/t/unit-tests/u-reftable-record.c b/t/unit-tests/u-reftable-record.c
index 1bf2e170dc..9c95083ef4 100644
--- a/t/unit-tests/u-reftable-record.c
+++ b/t/unit-tests/u-reftable-record.c
@@ -11,6 +11,7 @@
#include "reftable/basics.h"
#include "reftable/constants.h"
#include "reftable/record.h"
+#include "reftable/reftable-error.h"
static void t_copy(struct reftable_record *rec)
{
@@ -202,6 +203,29 @@ void test_reftable_record__ref_record_roundtrip(void)
reftable_buf_release(&scratch);
}
+void test_reftable_record__ref_record_decode_invalid_value_type(void)
+{
+ struct reftable_buf scratch = REFTABLE_BUF_INIT;
+ struct reftable_record out = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ };
+ struct reftable_buf key = REFTABLE_BUF_INIT;
+ uint8_t buffer[1024] = { 0 };
+ struct string_view dest = {
+ .buf = buffer,
+ .len = sizeof(buffer),
+ };
+
+ cl_must_pass(reftable_buf_addstr(&key, "refs/heads/master"));
+ cl_assert_equal_i(reftable_record_decode(&out, key, REFTABLE_NR_REF_VALUETYPES,
+ dest, REFTABLE_HASH_SIZE_SHA1, &scratch),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_record_release(&out);
+ reftable_buf_release(&key);
+ reftable_buf_release(&scratch);
+}
+
void test_reftable_record__log_record_comparison(void)
{
struct reftable_record in[3] = {
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 05/11] reftable/block: fix OOB write with bogus inflated log size
The "log" reftable block stores reflog information. This information is compressed using zlib. The inflated size is stored in the block header so that callers can easily learn ahead of time how large of a buffer they have to allocate to inflate the data in a single pass. So to reconstruct the full inflated block we:
- Copy over the header as-is, as it's not deflated.
- Append the inflated data to the buffer.
The inflated block size stored in the header also includes the length of the header itself. So to figure out the bytes that should be inflated by zlib we need to subtract the header size, which is trusted data, from the block size, which is untrusted data derived from the block header.
While we do verify that we were able to inflate all data as expected, we don't verify ahead of time that the encoded block length is larger than the header length. This can lead to an underflow, which makes zlib assume that it can write more data into the target buffer than we have allocated. The result is an out-of-bounds write:
==1422297==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c1ff6de5231 at pc 0x55555579a628 bp 0x7fffffff4f10 sp 0x7fffffff46d0
WRITE of size 4 at 0x7c1ff6de5231 thread T0
#0 0x55555579a627 in __asan_memcpy (./build/t/unit-tests+0x246627)
#1 0x55555598b093 in reftable_block_init ./build/../reftable/block.c:277:3
#2 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20
#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 0x55555584af4a 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) 0x7c1ff6de5231 is located 0 bytes after 1-byte region [0x7c1ff6de5230,0x7c1ff6de5231)
allocated by thread T0 here:
#0 0x55555579db1b in realloc.part.0 asan_malloc_linux.cpp.o
#1 0x5555559868d7 in reftable_realloc ./build/../reftable/basics.c:36:9
#2 0x55555598a98f in reftable_alloc_grow ./build/../reftable/basics.h:229:10
#3 0x55555598ae58 in reftable_block_init ./build/../reftable/block.c:269:3
#4 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20
#5 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3
#6 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3
#7 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4
#8 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11
#9 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8
#10 0x55555584af4a in main ./build/../common-main.c:9:11
#11 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#12 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#13 0x555555694c24 in _start (./build/t/unit-tests+0x140c24) SUMMARY: AddressSanitizer: heap-buffer-overflow (./build/t/unit-tests+0x246627) in __asan_memcpy
Shadow bytes around the buggy address:
0x7c1ff6de4f80: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5000: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5080: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5100: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5180: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd
=>0x7c1ff6de5200: fa fa 04 fa fa fa[01]fa fa fa fa fa fa fa fa fa
0x7c1ff6de5280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cbFix the bug by adding a sanity check and add a unit test.
Reported-by: oxsignal <awo@kakao.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 44 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 53 insertions(+)
Show changes to 2 files +53 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 920b3f4486..b86cb9ec5a 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -260,6 +260,15 @@ int reftable_block_init(struct reftable_block *block,
goto done;
}
+ /*
+ * Verify that the block size covers at least the table header, block
+ * header and the 2 byte restart counter.
+ */
+ if (block_size < header_size + 4 + 2) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
if (block_type == REFTABLE_BLOCK_TYPE_LOG) {
uint32_t block_header_skip = 4 + header_size;
uLong dst_len = block_size - block_header_skip;
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index f4bded7d26..40274af5c0 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -456,3 +456,47 @@ void test_reftable_block__iterator(void)
block_writer_release(&writer);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_log_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_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;
+
+ 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_LOG,
+ (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);
+
+ /*
+ * 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);
+
+ 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);
+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 06/11] 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>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 45 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 54 insertions(+)
Show changes to 2 files +54 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index b86cb9ec5a..4d6b11c2e7 100644
--- 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 40274af5c0..1f35aed91a 100644
--- 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);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 07/11] reftable/block: fix OOB read with bogus restart count
The restart count is stored in the last two bytes of a block. We use it without verification to compute the offset of the restart table. With a bogus restart count that is large enough this computation underflows, and the subsequent reads via the restart table access out-of-bounds memory:
==129439==ERROR: AddressSanitizer: SEGV on unknown address 0x7d90f6dcd0ad (pc 0x55555598ce89 bp 0x7fffffff4ed0 sp 0x7fffffff4e80 T0)
==129439==The signal is caused by a READ memory access.
#0 0x55555598ce89 in reftable_get_be24 ./git/build/../reftable/basics.h:125:9
#1 0x55555598eabf in block_restart_offset ./git/build/../reftable/block.c:407:9
#2 0x55555598e5d5 in restart_needle_less ./git/build/../reftable/block.c:431:17
#3 0x5555559887e2 in binsearch ./git/build/../reftable/basics.c:165:13
#4 0x55555598dfec in block_iter_seek_key ./git/build/../reftable/block.c:529:6
#5 0x555555814517 in test_reftable_block__corrupt_restart_count ./git/build/../t/unit-tests/u-reftable-block.c:593:15
#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 0x55555584c12a 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) ==129439==Register values:
rax = 0x00007d90f6dcd0ad rbx = 0x00007fffffff4f20 rcx = 0xf2f2f2f8f2f2f2f8 rdx = 0x0000000000000000
rdi = 0x00007d90f6dcd0ad rsi = 0x0000000000007fff rbp = 0x00007fffffff4ed0 rsp = 0x00007fffffff4e80
r8 = 0x0000000000000000 r9 = 0x0000000000000000 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff58e8 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x00005555560550b0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/basics.h:125:9 in reftable_get_be24
Verify that the restart table actually fits into the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 4 ++++
t/unit-tests/u-reftable-block.c | 46 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 50 insertions(+)
Show changes to 2 files +50 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 4d6b11c2e7..4d285aefd7 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -351,6 +351,10 @@ int reftable_block_init(struct reftable_block *block,
restart_count = reftable_get_be16(block->block_data.data + block_size - 2);
restart_off = block_size - 2 - 3 * restart_count;
+ if (restart_off < header_size + 4 || restart_off > block_size - 2) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
block->block_type = block_type;
block->hash_size = hash_size;
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index 1f35aed91a..ba410a0885 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -545,3 +545,49 @@ void test_reftable_block__corrupt_block_size(void)
block_writer_release(&writer);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_count(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;
+ int block_size;
+
+ 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));
+ block_size = block_writer_finish(&writer);
+ cl_assert(block_size > 0);
+
+ /*
+ * Corrupt the restart count to claim a bogus number of restart points.
+ * Note that this would only cause us to perform an out-of-bounds
+ * access when seeking into the block, but we want to refuse such a
+ * block outright.
+ */
+ reftable_put_be16((uint8_t *) data.buf + block_size - 2, 0xffff);
+
+ 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);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 08/11] reftable/block: fix use of uninitialized memory when binsearch fails
When doing the binary search through our restart offsets we may hit an error in case `restart_needle_less()` fails to decode the record at the given offset. While we correctly detect this case and error out, it will cause us to call `reftable_record_release()` on the yet-uninitialized record.
Fix this by initializing the record earlier.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to reftable/block.c +4 −4
diff --git a/reftable/block.c b/reftable/block.c
index 4d285aefd7..89efce8751 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -517,6 +517,10 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
int err = 0;
size_t i;
+ err = reftable_record_init(&rec, reftable_block_type(it->block));
+ if (err < 0)
+ goto done;
+
/*
* Perform a binary search over the block's restart points, which
* avoids doing a linear scan over the whole block. Like this, we
@@ -558,10 +562,6 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
else
it->next_off = it->block->header_off + 4;
- err = reftable_record_init(&rec, reftable_block_type(it->block));
- if (err < 0)
- goto done;
-
/*
* We're looking for the last entry less than the wanted key so that
* the next call to `block_reader_next()` would yield the wanted
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 09/11] 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>
---
reftable/block.c | 9 ++++++++
t/unit-tests/u-reftable-block.c | 51 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 60 insertions(+)
Show changes to 2 files +60 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 89efce8751..1fa81405d2 100644
--- 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 ba410a0885..77a054d082 100644
--- 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);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 10/11] reftable/table: fix NULL pointer access when seeking to bogus offsets
When seeking an iterator to an arbitrary offset we may return a positive value in case the offset points beyond the block. This makes sense when iterating through multiple blocks of the same section, as the positive value indicates to us that we're at the end of the table.
But when the offset originates from a section or index offset it is supposed to point at a valid block, so an out-of-bounds value means that the table is corrupt. Treating it as a normal end-of-iteration causes us to silently report an empty section instead of surfacing the corruption, and we are left with a partially-initialized block. This may later on cause a NULL pointer exception:
==1486841==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55555598e02c bp 0x7fffffff4eb0 sp 0x7fffffff4e70 T0)
==1486841==The signal is caused by a READ memory access.
==1486841==Hint: address points to the zero page.
#0 0x55555598e02c in reftable_block_type ./git/build/../reftable/block.c:392:9
#1 0x55555598ee6e in block_iter_seek_key ./git/build/../reftable/block.c:536:35
#2 0x5555559ae553 in table_iter_seek_linear ./git/build/../reftable/table.c:344:8
#3 0x5555559adbff in table_iter_seek ./git/build/../reftable/table.c:450:9
#4 0x5555559ada9c in table_iter_seek_void ./git/build/../reftable/table.c:460:9
#5 0x555555992872 in reftable_iterator_seek_log_at ./git/build/../reftable/iter.c:281:9
#6 0x555555992953 in reftable_iterator_seek_log ./git/build/../reftable/iter.c:287:9
#7 0x55555583aa78 in test_reftable_table__seek_invalid_log_offset ./git/build/../t/unit-tests/u-reftable-table.c:257:20
#8 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3
#9 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3
#10 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4
#11 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11
#12 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8
#13 0x55555584cffa in main ./git/build/../common-main.c:9:11
#14 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#15 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#16 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24) ==1486841==Register values:
rax = 0x0000000000000000 rbx = 0x00007fffffff4ec0 rcx = 0x0000000000000000 rdx = 0x00007cfff6e2bd58
rdi = 0x00007cfff6e2bd58 rsi = 0x00007bfff5da1020 rbp = 0x00007fffffff4eb0 rsp = 0x00007fffffff4e70
r8 = 0x0000000000000000 r9 = 0x0000000000000002 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff5908 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x0000555556056e90
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/block.c:392:9 in reftable_block_type
==1486841==ABORTING
Fix this by returning a proper error in `table_iter_seek_to()` when the offset ranges beyond the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 2 ++
t/unit-tests/u-reftable-table.c | 63 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
Show changes to 2 files +65 −0
reftable/table.c, t/unit-tests/u-reftable-table.c
diff --git a/reftable/table.c b/reftable/table.c
index 56362df0ed..f4bc86a29d 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -242,6 +242,8 @@ static int table_iter_seek_to(struct table_iter *ti, uint64_t off, uint8_t typ)
int err;
err = table_init_block(ti->table, &ti->block, off, typ);
+ if (err > 0)
+ return REFTABLE_FORMAT_ERROR;
if (err != 0)
return err;
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index 14fae8b199..c7dca45e70 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -1,8 +1,11 @@
#include "unit-test.h"
#include "lib-reftable.h"
+#include "reftable/basics.h"
+#include "reftable/block.h"
#include "reftable/blocksource.h"
#include "reftable/constants.h"
#include "reftable/iter.h"
+#include "reftable/reftable-error.h"
#include "reftable/table.h"
#include "strbuf.h"
@@ -199,3 +202,63 @@ void test_reftable_table__block_iterator(void)
reftable_buf_release(&buf);
reftable_free(records);
}
+
+void test_reftable_table__seek_invalid_log_offset(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_log_record logs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .update_index = 1,
+ .value_type = REFTABLE_LOG_UPDATE,
+ .value.update = {
+ .name = (char *) "user",
+ .email = (char *) "user@example.com",
+ .message = (char *) "message\n",
+ },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ size_t fsize = footer_size(1);
+ uint8_t *footer;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs),
+ logs, ARRAY_SIZE(logs), NULL);
+
+ /*
+ * Corrupt the log section offset stored in the footer so that it points
+ * past the end of the table. The footer is checksummed, so we also have
+ * to recompute and rewrite the CRC.
+ */
+ footer = (uint8_t *) buf.buf + buf.len - fsize;
+ reftable_put_be64(footer + header_size(1) + 24, UINT64_MAX);
+ reftable_put_be32(footer + fsize - 4, crc32(0, footer, fsize - 4));
+
+ block_source_from_buf(&source, &buf);
+ cl_must_pass(reftable_table_new(&table, &source, "name"));
+
+ /*
+ * Seeking the log iterator must not crash even though the log section
+ * offset is bogus. As the offset points past the end of the table we
+ * know that the table is corrupt, so the seek must report a format
+ * error instead of pretending that the section is empty.
+ */
+ reftable_table_init_log_iterator(table, &it);
+ cl_assert_equal_i(reftable_iterator_seek_log(&it, ""),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
[PATCH 11/11] reftable/table: fix OOB read on truncated table
When opening a table we compute the size of its data section by subtracting the footer size from the file size. We do not verify that the file is actually large enough to contain both the header and the footer though. With a truncated table the subtraction can thus underflow, causing us to read the footer out of bounds:
SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/pks/Development/git/build/t/unit-tests+0x2479a4) in __asan_memcpy
Shadow bytes around the buggy address:
0x7ccff6e0de80: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x7ccff6e0df00: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
0x7ccff6e0df80: fa fa fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x7ccff6e0e000: fd fd fd fd fa fa fa fa fa fa fa fa fd fd fd fd
0x7ccff6e0e080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
=>0x7ccff6e0e100: fa fa fa fa fa[fa]00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e180: 00 00 00 00 00 00 00 04 fa fa fa fa fa fa fa fa
0x7ccff6e0e200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e280: 00 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==1500371==ABORTINGVerify that the file is large enough to contain both the header and the footer before computing the table size.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 5 +++++
t/unit-tests/u-reftable-table.c | 28 ++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
Show changes to 2 files +33 −0
reftable/table.c, t/unit-tests/u-reftable-table.c
diff --git a/reftable/table.c b/reftable/table.c
index f4bc86a29d..b4d3f9e211 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -562,6 +562,11 @@ int reftable_table_new(struct reftable_table **out,
goto done;
}
+ if (file_size < header_size(t->version) + footer_size(t->version)) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
t->size = file_size - footer_size(t->version);
t->source = *source;
t->name = reftable_strdup(name);
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index c7dca45e70..28b0ef5258 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -262,3 +262,31 @@ void test_reftable_table__seek_invalid_log_offset(void)
reftable_table_decref(table);
reftable_buf_release(&buf);
}
+
+void test_reftable_table__new_with_truncated_table(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs), NULL, 0, NULL);
+
+ /*
+ * Truncate the table so that it is large enough to read the header, but
+ * too small to also contain the footer.
+ */
+ buf.len = footer_size(1) - 1;
+ block_source_from_buf(&source, &buf);
+
+ cl_assert_equal_i(reftable_table_new(&table, &source, "name"),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_buf_release(&buf);
+}
--
2.55.0.rc1.745.g43192e7977.dirty
Re: [PATCH 05/11] reftable/block: fix OOB write with bogus inflated log size
On Wed, Jun 24, 2026 at 10:24 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 35 quoted lines
> diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> index f4bded7d26..40274af5c0 100644
> --- a/t/unit-tests/u-reftable-block.c
> +++ b/t/unit-tests/u-reftable-block.c
> @@ -456,3 +456,47 @@ void test_reftable_block__iterator(void)
> block_writer_release(&writer);
> reftable_buf_release(&data);
> }
> +
> +void test_reftable_block__corrupt_log_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_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;
> +
> + 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_LOG,
> + (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);It looks like some of the block writing code above could be simplified using an helper function like:
int cl_reftable_write_block(struct reftable_buf *buf, uint8_t block_type,
size_t block_size, uint32_t header_off,
struct reftable_record *recs, size_t nrecs)
{
struct block_writer writer = {
.last_key = REFTABLE_BUF_INIT,
};
int block_end; REFTABLE_CALLOC_ARRAY(buf->buf, block_size);
cl_assert(buf->buf != NULL);
buf->len = block_size; cl_must_pass(block_writer_init(&writer, block_type, (uint8_t *) buf->buf,
block_size, header_off,
hash_size(REFTABLE_HASH_SHA1)));
for (size_t i = 0; i < nrecs; i++)
cl_must_pass(block_writer_add(&writer, &recs[i])); block_end = block_writer_finish(&writer);
cl_assert(block_end > 0); block_writer_release(&writer);
return block_end;
}
This function could be introduced by a preparatory commit in t/unit-tests/lib-reftable.{c,h}. It would be kind of similar to the existing cl_reftable_write_to_buf() helper in those files.
It looks like it could already simplify existing tests like:
- test_reftable_block__log_read_write
- test_reftable_block__obj_read_write
- test_reftable_block__ref_read_write
- test_reftable_block__iterator
and it could simplify the new tests introduced by other patches in this series:
- 06/11 reftable/block: fix OOB read with bogus block size
- 07/11 reftable/block: fix OOB read with bogus restart count
- 09/11 reftable/block: fix OOB read with bogus restart offset
Show 16 quoted lines
> + /*
> + * 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);
> +
> + 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);
> + block_writer_release(&writer);
> + reftable_buf_release(&data);
> +}
Otherwise the series looks great to me.
Thanks.
Re: [PATCH 05/11] reftable/block: fix OOB write with bogus inflated log size
On Fri, Jun 26, 2026 at 09:48:36AM +0200, Christian Couder wrote:
Show 84 quoted lines
> On Wed, Jun 24, 2026 at 10:24 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> > diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> > index f4bded7d26..40274af5c0 100644
> > --- a/t/unit-tests/u-reftable-block.c
> > +++ b/t/unit-tests/u-reftable-block.c
> > @@ -456,3 +456,47 @@ void test_reftable_block__iterator(void)
> > block_writer_release(&writer);
> > reftable_buf_release(&data);
> > }
> > +
> > +void test_reftable_block__corrupt_log_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_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;
> > +
> > + 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_LOG,
> > + (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);
>
> It looks like some of the block writing code above could be simplified
> using an helper function like:
>
> int cl_reftable_write_block(struct reftable_buf *buf, uint8_t block_type,
> size_t block_size, uint32_t header_off,
> struct reftable_record *recs, size_t nrecs)
> {
> struct block_writer writer = {
> .last_key = REFTABLE_BUF_INIT,
> };
> int block_end;
>
> REFTABLE_CALLOC_ARRAY(buf->buf, block_size);
> cl_assert(buf->buf != NULL);
> buf->len = block_size;
>
> cl_must_pass(block_writer_init(&writer, block_type, (uint8_t *) buf->buf,
> block_size, header_off,
> hash_size(REFTABLE_HASH_SHA1)));
> for (size_t i = 0; i < nrecs; i++)
> cl_must_pass(block_writer_add(&writer, &recs[i]));
>
> block_end = block_writer_finish(&writer);
> cl_assert(block_end > 0);
>
> block_writer_release(&writer);
>
> return block_end;
> }
>
> This function could be introduced by a preparatory commit in
> t/unit-tests/lib-reftable.{c,h}. It would be kind of similar to the
> existing cl_reftable_write_to_buf() helper in those files.
>
> It looks like it could already simplify existing tests like:
>
> - test_reftable_block__log_read_write
> - test_reftable_block__obj_read_write
> - test_reftable_block__ref_read_write
> - test_reftable_block__iterator
>
> and it could simplify the new tests introduced by other patches in this series:
>
> - 06/11 reftable/block: fix OOB read with bogus block size
> - 07/11 reftable/block: fix OOB read with bogus restart count
> - 09/11 reftable/block: fix OOB read with bogus restart offsetGood point, will do. Thanks!
Patrick
[PATCH v2 00/12] reftable: harden against corrupted tables
Hi,
this patch series addresses a bunch of errors that may happen when trying to read corrupted tables. These errors include out-of-bounds writes, out-of-bounds reads and the ability to hit abort(3p) calls.
The out-of-bounds write was originally reported by awo on the security mailing list. As we never transfer reftables over the protocol it would require local disk access to create such corrupted reftables, so there isn't really an easy way to exploit these.
In any case, I took that chance and wrote a fuzzer for parsing the tables, which surfaced a bunch of issues. At the end of this series though the fuzzer can now run for an extended amount of time (2hrs+) without surfacing any new issues.
Changes in v2:
- Introduce a test helper that writes a reftable block.
- Link to v1: https://patch.msgid.link/20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (12):
meson: support building fuzzers with libFuzzer
oss-fuzz: add fuzzer for parsing reftables
reftable/basics: fix OOB read on binary search of empty range
reftable/record: don't abort when decoding invalid ref value type
t/unit-tests: introduce test helper to write reftable blocks
reftable/block: fix OOB write with bogus inflated log size
reftable/block: fix OOB read with bogus block size
reftable/block: fix OOB read with bogus restart count
reftable/block: fix use of uninitialized memory when binsearch fails
reftable/block: fix OOB read with bogus restart offset
reftable/table: fix NULL pointer access when seeking to bogus offsets
reftable/table: fix OOB read on truncated table Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
meson.build | 15 +++
meson_options.txt | 2 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 +++++++++++++++
oss-fuzz/meson.build | 2 +
reftable/basics.c | 3 +
reftable/block.c | 39 +++++++-
reftable/record.c | 6 +-
reftable/table.c | 7 ++
t/unit-tests/u-reftable-basics.c | 11 +++
t/unit-tests/u-reftable-block.c | 184 ++++++++++++++++++++++++++++++++----
t/unit-tests/u-reftable-record.c | 24 +++++
t/unit-tests/u-reftable-table.c | 91 ++++++++++++++++++
15 files changed, 435 insertions(+), 26 deletions(-)
Range-diff versus v1:
1: 82275a5448 = 1: 5bb58da117 meson: support building fuzzers with libFuzzer
2: 8b234b5dc6 = 2: 8d11b15082 oss-fuzz: add fuzzer for parsing reftables
3: f265bcf6f4 = 3: 21186da3f1 reftable/basics: fix OOB read on binary search of empty range
4: a56c6cb50c = 4: 3c327bacc2 reftable/record: don't abort when decoding invalid ref value type
-: ---------- > 5: 4125c76a97 t/unit-tests: introduce test helper to write reftable blocks
5: 9074372e30 ! 6: e923c23518 reftable/block: fix OOB write with bogus inflated log size
@@ reftable/block.c: int reftable_block_init(struct reftable_block *block,
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__iterator(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_log_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_LOG,
+ .u.log = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__iterator(void)
+ },
+ };
+ struct reftable_block block = { 0 };
-+ struct reftable_buf data;
-+
-+ data.len = 1024;
-+ REFTABLE_CALLOC_ARRAY(data.buf, data.len);
-+ cl_assert(data.buf != NULL);
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+
-+ cl_must_pass(block_writer_init(&writer, REFTABLE_BLOCK_TYPE_LOG,
-+ (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);
++ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_LOG, &rec, 1);
+
+ /*
+ * Log blocks store their inflated size as a big-endian 24-bit integer
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__iterator(void)
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
6: 6877f58485 ! 7: 16c2904a96 reftable/block: fix OOB read with bogus block size
@@ reftable/block.c: int reftable_block_init(struct reftable_block *block,
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_size(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
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 = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_siz
+ },
+ };
+ struct reftable_block block = { 0 };
-+ struct reftable_buf data;
-+
-+ data.len = 1024;
-+ REFTABLE_CALLOC_ARRAY(data.buf, data.len);
-+ cl_assert(data.buf != NULL);
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+
-+ 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);
++ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * The block size is stored as a big-endian 24-bit integer right after
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_siz
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
7: 3c022a4f97 ! 8: 872eca67bb reftable/block: fix OOB read with bogus restart count
@@ reftable/block.c: int reftable_block_init(struct reftable_block *block,
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_block_size(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_count(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 = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_block_size(vo
+ },
+ };
+ struct reftable_block block = { 0 };
-+ struct reftable_buf data;
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+ int block_size;
+
-+ 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));
-+ block_size = block_writer_finish(&writer);
-+ cl_assert(block_size > 0);
++ block_size = cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * Corrupt the restart count to claim a bogus number of restart points.
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_block_size(vo
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
8: af5697b85b = 9: c82d51c163 reftable/block: fix use of uninitialized memory when binsearch fails
9: e9d4eca613 ! 10: 16e1087a66 reftable/block: fix OOB read with bogus restart offset
@@ reftable/block.c: static int restart_needle_less(size_t idx, void *_args)
## t/unit-tests/u-reftable-block.c ##
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_restart_count(void)
- block_writer_release(&writer);
+ reftable_block_release(&block);
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 = {
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_restart_count
+ 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);
++ struct reftable_buf data = REFTABLE_BUF_INIT;
+
-+ 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);
++ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ block_source_from_buf(&source, &data);
+ cl_must_pass(reftable_block_init(&block, &source, 0, 0, data.len,
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_restart_count
+ reftable_buf_release(&want);
+ block_iter_close(&it);
+ reftable_block_release(&block);
-+ block_writer_release(&writer);
+ reftable_buf_release(&data);
+}
10: 4bb729aeb0 = 11: 63dd98f908 reftable/table: fix NULL pointer access when seeking to bogus offsets
11: e3bca6af6e = 12: 32696a01bc reftable/table: fix OOB read on truncated table--- base-commit: ab776a62a78576513ee121424adb19597fbb7613 change-id: 20260623-pks-reftable-hardening-f54de69fea63
[PATCH v2 01/12] meson: support building fuzzers with libFuzzer
To support fuzzing via libFuzzer one has to pass a couple of compiler options:
- It is mandatory to enable the "fuzzer-no-link" sanitizer for
coverage feedback. - It is recommended to enable at least one more sanitizer to catch
issues, like the "address" sanitizer. - The fuzzing executables need to be linked with "-fsanitize=fuzzer"
to wire up libFuzzer itself.The first two items can already be achieved via the "-Db_sanitize=" option. But the last item cannot easily be achieved, as we can only configure global link arguments.
Introduce a new "-Dfuzzers_link_args=" build option to plug this gap. Add documentation so that users know how to set up libFuzzer.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
meson.build | 15 +++++++++++++++
meson_options.txt | 2 ++
oss-fuzz/meson.build | 1 +
3 files changed, 18 insertions(+)
Show changes to 3 files +18 −0
meson.build, meson_options.txt, oss-fuzz/meson.build
diff --git a/meson.build b/meson.build
index 3247697f74..9df6fbb0a5 100644
--- a/meson.build
+++ b/meson.build
@@ -161,6 +161,21 @@
# These machine files can be passed to `meson setup` via the `--native-file`
# option.
#
+# Fuzzing
+# =======
+#
+# Meson supports building the fuzzing targets by setting `-Dfuzzers=true`. By
+# default, the targets will be built without libFuzzer and thus won't be usable
+# for fuzzing. You have to configure a couple of options to properly wire up
+# libFuzzer:
+#
+# $ meson setup build-fuzzers \
+# -Db_sanitize=address,fuzzer-no-link \
+# -Dfuzzers=true \
+# -Dfuzzers_link_args=-fsanitize=fuzzer
+# $ meson compile -C build-fuzzers
+# $ ./build-fuzzers/oss-fuzz/fuzz-config <args>
+#
# Cross compilation
# =================
#
diff --git a/meson_options.txt b/meson_options.txt
index d936ada098..dc88f130d7 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -131,3 +131,5 @@ option('test_utf8_locale', type: 'string',
description: 'Name of a UTF-8 locale used for testing.')
option('fuzzers', type: 'boolean', value: false,
description: 'Enable building fuzzers.')
+option('fuzzers_link_args', type: 'array', value: [],
+ description: 'Linker arguments used to link fuzzers. Use -fsanitize=fuzzer for fuzzing.')
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 878afd8426..10bcac2f6d 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -16,5 +16,6 @@ foreach fuzz_program : fuzz_programs
fuzz_program,
],
dependencies: [libgit_commonmain],
+ link_args: get_option('fuzzers_link_args'),
)
endforeach
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 02/12] oss-fuzz: add fuzzer for parsing reftables
Add a new fuzzer that exercises our parsing of reftables. Fallout from this fuzzer will be fixed over subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 +++++++++++++++++++++++++++++++++++++
oss-fuzz/meson.build | 1 +
5 files changed, 78 insertions(+)
Show changes to 5 files +78 −0
Makefile, ci/run-build-and-minimal-fuzzers.sh, oss-fuzz/.gitignore, oss-fuzz/fuzz-reftable.c, oss-fuzz/meson.build
diff --git a/Makefile b/Makefile
index 1cec251f43..89d3edd5ea 100644
--- a/Makefile
+++ b/Makefile
@@ -2599,6 +2599,7 @@ FUZZ_OBJS += oss-fuzz/fuzz-date.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
FUZZ_OBJS += oss-fuzz/fuzz-parse-attr-line.o
+FUZZ_OBJS += oss-fuzz/fuzz-reftable.o
FUZZ_OBJS += oss-fuzz/fuzz-url-decode-mem.o
.PHONY: fuzz-objs
fuzz-objs: $(FUZZ_OBJS)
diff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh
index e7b97952e7..37b24b092d 100755
--- a/ci/run-build-and-minimal-fuzzers.sh
+++ b/ci/run-build-and-minimal-fuzzers.sh
@@ -21,6 +21,7 @@ date
pack-headers
pack-idx
parse-attr-line
+reftable
url-decode-mem
"
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index f2d74de457..dc7a127a62 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -5,4 +5,5 @@ fuzz-date
fuzz-pack-headers
fuzz-pack-idx
fuzz-parse-attr-line
+fuzz-reftable
fuzz-url-decode-mem
diff --git a/oss-fuzz/fuzz-reftable.c b/oss-fuzz/fuzz-reftable.c
new file mode 100644
index 0000000000..c46eac2c6b
--- /dev/null
+++ b/oss-fuzz/fuzz-reftable.c
@@ -0,0 +1,74 @@
+#include "git-compat-util.h"
+#include "reftable/basics.h"
+#include "reftable/blocksource.h"
+#include "reftable/reftable-blocksource.h"
+#include "reftable/reftable-error.h"
+#include "reftable/reftable-iterator.h"
+#include "reftable/reftable-record.h"
+#include "reftable/reftable-table.h"
+#include "reftable/reftable-writer.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ struct reftable_block_source source = { 0 };
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ struct reftable_table *table = NULL;
+ int err;
+
+ if (reftable_buf_add(&buf, (const char *)data, size) < 0)
+ goto out;
+ block_source_from_buf(&source, &buf);
+
+ err = reftable_table_new(&table, &source, "fuzz-input");
+ if (err < 0)
+ goto out;
+
+ /*
+ * Exercise the ref, log and raw block iterators so that we cover as
+ * much of the parsing code as possible.
+ */
+ {
+ struct reftable_ref_record ref = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_ref_iterator(table, &it);
+ if (!reftable_iterator_seek_ref(&it, ""))
+ while (!reftable_iterator_next_ref(&it, &ref))
+ ;
+
+ reftable_ref_record_release(&ref);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_log_iterator(table, &it);
+ if (!reftable_iterator_seek_log(&it, ""))
+ while (!reftable_iterator_next_log(&it, &log))
+ ;
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_table_iterator it = { 0 };
+ const struct reftable_block *block;
+
+ if (!reftable_table_iterator_init(&it, table))
+ while (!reftable_table_iterator_next(&it, &block))
+ ;
+
+ reftable_table_iterator_release(&it);
+ }
+
+out:
+ if (table)
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+ return 0;
+}
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 10bcac2f6d..5a3854256b 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -6,6 +6,7 @@ fuzz_programs = [
'fuzz-pack-headers.c',
'fuzz-pack-idx.c',
'fuzz-parse-attr-line.c',
+ 'fuzz-reftable.c',
'fuzz-url-decode-mem.c',
]
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 03/12] reftable/basics: fix OOB read on binary search of empty range
`binsearch()` performs a binary search over a range of `sz` elements by repeatedly calling the comparison function with indices into that range. When the range is empty though, there is no valid index to call the comparison function with. We still end up executing the comparison function though with an index of 0, which of course will cause an out-of-bounds read.
Return early when the range is empty.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/basics.c | 3 +++
t/unit-tests/u-reftable-basics.c | 11 +++++++++++
2 files changed, 14 insertions(+)
Show changes to 2 files +14 −0
reftable/basics.c, t/unit-tests/u-reftable-basics.c
diff --git a/reftable/basics.c b/reftable/basics.c
index e969927b61..f0442a46cf 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -152,6 +152,9 @@ size_t binsearch(size_t sz, int (*f)(size_t k, void *args), void *args)
size_t lo = 0;
size_t hi = sz;
+ if (!sz)
+ return 0;
+
/* Invariants:
*
* (hi == sz) || f(hi) == true
diff --git a/t/unit-tests/u-reftable-basics.c b/t/unit-tests/u-reftable-basics.c
index 73566ed0eb..c5d83b6714 100644
--- a/t/unit-tests/u-reftable-basics.c
+++ b/t/unit-tests/u-reftable-basics.c
@@ -60,6 +60,17 @@ void test_reftable_basics__binsearch(void)
}
}
+static int unreachable_lesseq(size_t i UNUSED, void *args UNUSED)
+{
+ cl_fail("comparison function called for empty range");
+ return 0;
+}
+
+void test_reftable_basics__binsearch_empty(void)
+{
+ cl_assert_equal_i(binsearch(0, &unreachable_lesseq, NULL), 0);
+}
+
void test_reftable_basics__names_length(void)
{
const char *a[] = { "a", "b", NULL };
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 04/12] reftable/record: don't abort when decoding invalid ref value type
When decoding a ref record we read its value type from the block. In case the type itself is invalid we call `abort()`. This is rather heavy-handed though: the data we're reading is untrusted, so we should treat the issue as a normal and not as a programming error.
Fix this by handling the error gracefully. Note that this also requires us to set the value type later, as otherwise we might store an invalid type in the record.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 6 +++---
t/unit-tests/u-reftable-record.c | 24 ++++++++++++++++++++++++
2 files changed, 27 insertions(+), 3 deletions(-)
Show changes to 2 files +27 −3
reftable/record.c, t/unit-tests/u-reftable-record.c
diff --git a/reftable/record.c b/reftable/record.c
index fcd387ba5d..1fce441930 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -388,7 +388,6 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
r->refname[key.len] = 0;
r->update_index = update_index;
- r->value_type = val_type;
switch (val_type) {
case REFTABLE_REF_VAL1:
if (in.len < hash_size) {
@@ -426,9 +425,10 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
case REFTABLE_REF_DELETION:
break;
default:
- abort();
- break;
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
}
+ r->value_type = val_type;
return start.len - in.len;
diff --git a/t/unit-tests/u-reftable-record.c b/t/unit-tests/u-reftable-record.c
index 1bf2e170dc..9c95083ef4 100644
--- a/t/unit-tests/u-reftable-record.c
+++ b/t/unit-tests/u-reftable-record.c
@@ -11,6 +11,7 @@
#include "reftable/basics.h"
#include "reftable/constants.h"
#include "reftable/record.h"
+#include "reftable/reftable-error.h"
static void t_copy(struct reftable_record *rec)
{
@@ -202,6 +203,29 @@ void test_reftable_record__ref_record_roundtrip(void)
reftable_buf_release(&scratch);
}
+void test_reftable_record__ref_record_decode_invalid_value_type(void)
+{
+ struct reftable_buf scratch = REFTABLE_BUF_INIT;
+ struct reftable_record out = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ };
+ struct reftable_buf key = REFTABLE_BUF_INIT;
+ uint8_t buffer[1024] = { 0 };
+ struct string_view dest = {
+ .buf = buffer,
+ .len = sizeof(buffer),
+ };
+
+ cl_must_pass(reftable_buf_addstr(&key, "refs/heads/master"));
+ cl_assert_equal_i(reftable_record_decode(&out, key, REFTABLE_NR_REF_VALUETYPES,
+ dest, REFTABLE_HASH_SIZE_SHA1, &scratch),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_record_release(&out);
+ reftable_buf_release(&key);
+ reftable_buf_release(&scratch);
+}
+
void test_reftable_record__log_record_comparison(void)
{
struct reftable_record in[3] = {
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 05/12] t/unit-tests: introduce test helper to write reftable blocks
Introduce a new test helper that allows us to write reftable blocks. This helper will be used by subsequent commits.
Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/unit-tests/u-reftable-block.c | 47 ++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 19 deletions(-)
Show changes to t/unit-tests/u-reftable-block.c +28 −19
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index f4bded7d26..f4e926ce3a 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -14,6 +14,31 @@ license that can be found in the LICENSE file or at
#include "reftable/reftable-error.h"
#include "strbuf.h"
+static int cl_reftable_write_block(struct reftable_buf *buf,
+ uint8_t block_type,
+ struct reftable_record *recs,
+ size_t nrecs)
+{
+ struct block_writer writer = {
+ .last_key = REFTABLE_BUF_INIT,
+ };
+ uint8_t block[1024];
+ int block_end;
+
+ cl_must_pass(block_writer_init(&writer, block_type, block, 1024,
+ 0, hash_size(REFTABLE_HASH_SHA1)));
+ for (size_t i = 0; i < nrecs; i++)
+ cl_must_pass(block_writer_add(&writer, &recs[i]));
+
+ block_end = block_writer_finish(&writer);
+ cl_assert(block_end > 0);
+
+ cl_must_pass(reftable_buf_add(buf, block, block_end));
+
+ block_writer_release(&writer);
+ return block_end;
+}
+
void test_reftable_block__read_write(void)
{
const int header_off = 21; /* random */
@@ -381,25 +406,13 @@ void test_reftable_block__ref_read_write(void)
void test_reftable_block__iterator(void)
{
struct reftable_block_source source = { 0 };
- struct block_writer writer = {
- .last_key = REFTABLE_BUF_INIT,
- };
struct reftable_record expected_refs[20];
struct reftable_ref_record ref = { 0 };
struct reftable_iterator it = { 0 };
struct reftable_block block = { 0 };
- struct reftable_buf data;
+ struct reftable_buf data = REFTABLE_BUF_INIT;
int err;
- data.len = 1024;
- REFTABLE_CALLOC_ARRAY(data.buf, data.len);
- cl_assert(data.buf != NULL);
-
- err = block_writer_init(&writer, REFTABLE_BLOCK_TYPE_REF,
- (uint8_t *) data.buf, data.len,
- 0, hash_size(REFTABLE_HASH_SHA1));
- cl_assert(!err);
-
for (size_t i = 0; i < ARRAY_SIZE(expected_refs); i++) {
expected_refs[i] = (struct reftable_record) {
.type = REFTABLE_BLOCK_TYPE_REF,
@@ -409,13 +422,10 @@ void test_reftable_block__iterator(void)
},
};
memset(expected_refs[i].u.ref.value.val1, i, REFTABLE_HASH_SIZE_SHA1);
-
- err = block_writer_add(&writer, &expected_refs[i]);
- cl_assert_equal_i(err, 0);
}
- err = block_writer_finish(&writer);
- cl_assert(err > 0);
+ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF,
+ expected_refs, ARRAY_SIZE(expected_refs));
block_source_from_buf(&source, &data);
reftable_block_init(&block, &source, 0, 0, data.len,
@@ -453,6 +463,5 @@ void test_reftable_block__iterator(void)
reftable_ref_record_release(&ref);
reftable_iterator_destroy(&it);
reftable_block_release(&block);
- block_writer_release(&writer);
reftable_buf_release(&data);
}
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 06/12] reftable/block: fix OOB write with bogus inflated log size
The "log" reftable block stores reflog information. This information is compressed using zlib. The inflated size is stored in the block header so that callers can easily learn ahead of time how large of a buffer they have to allocate to inflate the data in a single pass. So to reconstruct the full inflated block we:
- Copy over the header as-is, as it's not deflated.
- Append the inflated data to the buffer.
The inflated block size stored in the header also includes the length of the header itself. So to figure out the bytes that should be inflated by zlib we need to subtract the header size, which is trusted data, from the block size, which is untrusted data derived from the block header.
While we do verify that we were able to inflate all data as expected, we don't verify ahead of time that the encoded block length is larger than the header length. This can lead to an underflow, which makes zlib assume that it can write more data into the target buffer than we have allocated. The result is an out-of-bounds write:
==1422297==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c1ff6de5231 at pc 0x55555579a628 bp 0x7fffffff4f10 sp 0x7fffffff46d0
WRITE of size 4 at 0x7c1ff6de5231 thread T0
#0 0x55555579a627 in __asan_memcpy (./build/t/unit-tests+0x246627)
#1 0x55555598b093 in reftable_block_init ./build/../reftable/block.c:277:3
#2 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20
#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 0x55555584af4a 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) 0x7c1ff6de5231 is located 0 bytes after 1-byte region [0x7c1ff6de5230,0x7c1ff6de5231)
allocated by thread T0 here:
#0 0x55555579db1b in realloc.part.0 asan_malloc_linux.cpp.o
#1 0x5555559868d7 in reftable_realloc ./build/../reftable/basics.c:36:9
#2 0x55555598a98f in reftable_alloc_grow ./build/../reftable/basics.h:229:10
#3 0x55555598ae58 in reftable_block_init ./build/../reftable/block.c:269:3
#4 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20
#5 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3
#6 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3
#7 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4
#8 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11
#9 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8
#10 0x55555584af4a in main ./build/../common-main.c:9:11
#11 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#12 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#13 0x555555694c24 in _start (./build/t/unit-tests+0x140c24) SUMMARY: AddressSanitizer: heap-buffer-overflow (./build/t/unit-tests+0x246627) in __asan_memcpy
Shadow bytes around the buggy address:
0x7c1ff6de4f80: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5000: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5080: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5100: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5180: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd
=>0x7c1ff6de5200: fa fa 04 fa fa fa[01]fa fa fa fa fa fa fa fa fa
0x7c1ff6de5280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cbFix the bug by adding a sanity check and add a unit test.
Reported-by: oxsignal <awo@kakao.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+)
Show changes to 2 files +41 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 920b3f4486..b86cb9ec5a 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -260,6 +260,15 @@ int reftable_block_init(struct reftable_block *block,
goto done;
}
+ /*
+ * Verify that the block size covers at least the table header, block
+ * header and the 2 byte restart counter.
+ */
+ if (block_size < header_size + 4 + 2) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
if (block_type == REFTABLE_BLOCK_TYPE_LOG) {
uint32_t block_header_skip = 4 + header_size;
uLong dst_len = block_size - block_header_skip;
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);
+
+ 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);
+}
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 07/12] 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>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 42 insertions(+)
Show changes to 2 files +42 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index b86cb9ec5a..4d6b11c2e7 100644
--- 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 088162483e..43b9d5fb59 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -497,3 +497,36 @@ void test_reftable_block__corrupt_log_block_size(void)
reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_block_size(void)
+{
+ struct reftable_block_source source = { 0 };
+ 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 = REFTABLE_BUF_INIT;
+
+ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * 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);
+ reftable_buf_release(&data);
+}
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 08/12] reftable/block: fix OOB read with bogus restart count
The restart count is stored in the last two bytes of a block. We use it without verification to compute the offset of the restart table. With a bogus restart count that is large enough this computation underflows, and the subsequent reads via the restart table access out-of-bounds memory:
==129439==ERROR: AddressSanitizer: SEGV on unknown address 0x7d90f6dcd0ad (pc 0x55555598ce89 bp 0x7fffffff4ed0 sp 0x7fffffff4e80 T0)
==129439==The signal is caused by a READ memory access.
#0 0x55555598ce89 in reftable_get_be24 ./git/build/../reftable/basics.h:125:9
#1 0x55555598eabf in block_restart_offset ./git/build/../reftable/block.c:407:9
#2 0x55555598e5d5 in restart_needle_less ./git/build/../reftable/block.c:431:17
#3 0x5555559887e2 in binsearch ./git/build/../reftable/basics.c:165:13
#4 0x55555598dfec in block_iter_seek_key ./git/build/../reftable/block.c:529:6
#5 0x555555814517 in test_reftable_block__corrupt_restart_count ./git/build/../t/unit-tests/u-reftable-block.c:593:15
#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 0x55555584c12a 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) ==129439==Register values:
rax = 0x00007d90f6dcd0ad rbx = 0x00007fffffff4f20 rcx = 0xf2f2f2f8f2f2f2f8 rdx = 0x0000000000000000
rdi = 0x00007d90f6dcd0ad rsi = 0x0000000000007fff rbp = 0x00007fffffff4ed0 rsp = 0x00007fffffff4e80
r8 = 0x0000000000000000 r9 = 0x0000000000000000 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff58e8 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x00005555560550b0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/basics.h:125:9 in reftable_get_be24
Verify that the restart table actually fits into the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 4 ++++
t/unit-tests/u-reftable-block.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+)
Show changes to 2 files +37 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 4d6b11c2e7..4d285aefd7 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -351,6 +351,10 @@ int reftable_block_init(struct reftable_block *block,
restart_count = reftable_get_be16(block->block_data.data + block_size - 2);
restart_off = block_size - 2 - 3 * restart_count;
+ if (restart_off < header_size + 4 || restart_off > block_size - 2) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
block->block_type = block_type;
block->hash_size = hash_size;
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index 43b9d5fb59..a0fbfb247f 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -530,3 +530,36 @@ void test_reftable_block__corrupt_block_size(void)
reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_count(void)
+{
+ struct reftable_block_source source = { 0 };
+ 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 = REFTABLE_BUF_INIT;
+ int block_size;
+
+ block_size = cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * Corrupt the restart count to claim a bogus number of restart points.
+ * Note that this would only cause us to perform an out-of-bounds
+ * access when seeking into the block, but we want to refuse such a
+ * block outright.
+ */
+ reftable_put_be16((uint8_t *) data.buf + block_size - 2, 0xffff);
+
+ 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);
+ reftable_buf_release(&data);
+}
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 09/12] reftable/block: fix use of uninitialized memory when binsearch fails
When doing the binary search through our restart offsets we may hit an error in case `restart_needle_less()` fails to decode the record at the given offset. While we correctly detect this case and error out, it will cause us to call `reftable_record_release()` on the yet-uninitialized record.
Fix this by initializing the record earlier.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to reftable/block.c +4 −4
diff --git a/reftable/block.c b/reftable/block.c
index 4d285aefd7..89efce8751 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -517,6 +517,10 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
int err = 0;
size_t i;
+ err = reftable_record_init(&rec, reftable_block_type(it->block));
+ if (err < 0)
+ goto done;
+
/*
* Perform a binary search over the block's restart points, which
* avoids doing a linear scan over the whole block. Like this, we
@@ -558,10 +562,6 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
else
it->next_off = it->block->header_off + 4;
- err = reftable_record_init(&rec, reftable_block_type(it->block));
- if (err < 0)
- goto done;
-
/*
* We're looking for the last entry less than the wanted key so that
* the next call to `block_reader_next()` would yield the wanted
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 10/12] 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>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
Show changes to 2 files +48 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 89efce8751..1fa81405d2 100644
--- 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 a0fbfb247f..b8bb9a23e4 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -563,3 +563,42 @@ void test_reftable_block__corrupt_restart_count(void)
reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_offset(void)
+{
+ struct reftable_block_source source = { 0 };
+ 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 = REFTABLE_BUF_INIT;
+
+ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ 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);
+ reftable_buf_release(&data);
+}
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 11/12] reftable/table: fix NULL pointer access when seeking to bogus offsets
When seeking an iterator to an arbitrary offset we may return a positive value in case the offset points beyond the block. This makes sense when iterating through multiple blocks of the same section, as the positive value indicates to us that we're at the end of the table.
But when the offset originates from a section or index offset it is supposed to point at a valid block, so an out-of-bounds value means that the table is corrupt. Treating it as a normal end-of-iteration causes us to silently report an empty section instead of surfacing the corruption, and we are left with a partially-initialized block. This may later on cause a NULL pointer exception:
==1486841==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55555598e02c bp 0x7fffffff4eb0 sp 0x7fffffff4e70 T0)
==1486841==The signal is caused by a READ memory access.
==1486841==Hint: address points to the zero page.
#0 0x55555598e02c in reftable_block_type ./git/build/../reftable/block.c:392:9
#1 0x55555598ee6e in block_iter_seek_key ./git/build/../reftable/block.c:536:35
#2 0x5555559ae553 in table_iter_seek_linear ./git/build/../reftable/table.c:344:8
#3 0x5555559adbff in table_iter_seek ./git/build/../reftable/table.c:450:9
#4 0x5555559ada9c in table_iter_seek_void ./git/build/../reftable/table.c:460:9
#5 0x555555992872 in reftable_iterator_seek_log_at ./git/build/../reftable/iter.c:281:9
#6 0x555555992953 in reftable_iterator_seek_log ./git/build/../reftable/iter.c:287:9
#7 0x55555583aa78 in test_reftable_table__seek_invalid_log_offset ./git/build/../t/unit-tests/u-reftable-table.c:257:20
#8 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3
#9 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3
#10 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4
#11 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11
#12 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8
#13 0x55555584cffa in main ./git/build/../common-main.c:9:11
#14 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#15 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#16 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24) ==1486841==Register values:
rax = 0x0000000000000000 rbx = 0x00007fffffff4ec0 rcx = 0x0000000000000000 rdx = 0x00007cfff6e2bd58
rdi = 0x00007cfff6e2bd58 rsi = 0x00007bfff5da1020 rbp = 0x00007fffffff4eb0 rsp = 0x00007fffffff4e70
r8 = 0x0000000000000000 r9 = 0x0000000000000002 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff5908 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x0000555556056e90
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/block.c:392:9 in reftable_block_type
==1486841==ABORTING
Fix this by returning a proper error in `table_iter_seek_to()` when the offset ranges beyond the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 2 ++
t/unit-tests/u-reftable-table.c | 63 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
Show changes to 2 files +65 −0
reftable/table.c, t/unit-tests/u-reftable-table.c
diff --git a/reftable/table.c b/reftable/table.c
index 56362df0ed..f4bc86a29d 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -242,6 +242,8 @@ static int table_iter_seek_to(struct table_iter *ti, uint64_t off, uint8_t typ)
int err;
err = table_init_block(ti->table, &ti->block, off, typ);
+ if (err > 0)
+ return REFTABLE_FORMAT_ERROR;
if (err != 0)
return err;
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index 14fae8b199..c7dca45e70 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -1,8 +1,11 @@
#include "unit-test.h"
#include "lib-reftable.h"
+#include "reftable/basics.h"
+#include "reftable/block.h"
#include "reftable/blocksource.h"
#include "reftable/constants.h"
#include "reftable/iter.h"
+#include "reftable/reftable-error.h"
#include "reftable/table.h"
#include "strbuf.h"
@@ -199,3 +202,63 @@ void test_reftable_table__block_iterator(void)
reftable_buf_release(&buf);
reftable_free(records);
}
+
+void test_reftable_table__seek_invalid_log_offset(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_log_record logs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .update_index = 1,
+ .value_type = REFTABLE_LOG_UPDATE,
+ .value.update = {
+ .name = (char *) "user",
+ .email = (char *) "user@example.com",
+ .message = (char *) "message\n",
+ },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ size_t fsize = footer_size(1);
+ uint8_t *footer;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs),
+ logs, ARRAY_SIZE(logs), NULL);
+
+ /*
+ * Corrupt the log section offset stored in the footer so that it points
+ * past the end of the table. The footer is checksummed, so we also have
+ * to recompute and rewrite the CRC.
+ */
+ footer = (uint8_t *) buf.buf + buf.len - fsize;
+ reftable_put_be64(footer + header_size(1) + 24, UINT64_MAX);
+ reftable_put_be32(footer + fsize - 4, crc32(0, footer, fsize - 4));
+
+ block_source_from_buf(&source, &buf);
+ cl_must_pass(reftable_table_new(&table, &source, "name"));
+
+ /*
+ * Seeking the log iterator must not crash even though the log section
+ * offset is bogus. As the offset points past the end of the table we
+ * know that the table is corrupt, so the seek must report a format
+ * error instead of pretending that the section is empty.
+ */
+ reftable_table_init_log_iterator(table, &it);
+ cl_assert_equal_i(reftable_iterator_seek_log(&it, ""),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+}
--
2.55.0.rc2.803.g1fd1e6609c.dirty
[PATCH v2 12/12] reftable/table: fix OOB read on truncated table
When opening a table we compute the size of its data section by subtracting the footer size from the file size. We do not verify that the file is actually large enough to contain both the header and the footer though. With a truncated table the subtraction can thus underflow, causing us to read the footer out of bounds:
SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/pks/Development/git/build/t/unit-tests+0x2479a4) in __asan_memcpy
Shadow bytes around the buggy address:
0x7ccff6e0de80: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x7ccff6e0df00: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
0x7ccff6e0df80: fa fa fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x7ccff6e0e000: fd fd fd fd fa fa fa fa fa fa fa fa fd fd fd fd
0x7ccff6e0e080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
=>0x7ccff6e0e100: fa fa fa fa fa[fa]00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e180: 00 00 00 00 00 00 00 04 fa fa fa fa fa fa fa fa
0x7ccff6e0e200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e280: 00 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==1500371==ABORTINGVerify that the file is large enough to contain both the header and the footer before computing the table size.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 5 +++++
t/unit-tests/u-reftable-table.c | 28 ++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
Show changes to 2 files +33 −0
reftable/table.c, t/unit-tests/u-reftable-table.c
diff --git a/reftable/table.c b/reftable/table.c
index f4bc86a29d..b4d3f9e211 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -562,6 +562,11 @@ int reftable_table_new(struct reftable_table **out,
goto done;
}
+ if (file_size < header_size(t->version) + footer_size(t->version)) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
t->size = file_size - footer_size(t->version);
t->source = *source;
t->name = reftable_strdup(name);
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index c7dca45e70..28b0ef5258 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -262,3 +262,31 @@ void test_reftable_table__seek_invalid_log_offset(void)
reftable_table_decref(table);
reftable_buf_release(&buf);
}
+
+void test_reftable_table__new_with_truncated_table(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs), NULL, 0, NULL);
+
+ /*
+ * Truncate the table so that it is large enough to read the header, but
+ * too small to also contain the footer.
+ */
+ buf.len = footer_size(1) - 1;
+ block_source_from_buf(&source, &buf);
+
+ cl_assert_equal_i(reftable_table_new(&table, &source, "name"),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_buf_release(&buf);
+}
--
2.55.0.rc2.803.g1fd1e6609c.dirty
Re: [PATCH v2 05/12] t/unit-tests: introduce test helper to write reftable blocks
Patrick Steinhardt <ps@pks.im> writes:
Show 8 quoted lines
> Introduce a new test helper that allows us to write reftable blocks.
> This helper will be used by subsequent commits.
>
> Suggested-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/unit-tests/u-reftable-block.c | 47 ++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 19 deletions(-)
Nicely reduces boilerplate/setup code. Thanks, both.
Re: [PATCH v2 05/12] t/unit-tests: introduce test helper to write reftable blocks
On Mon, Jun 29, 2026 at 11:02 AM Patrick Steinhardt <ps@pks.im> wrote:
Show 22 quoted lines
>
> Introduce a new test helper that allows us to write reftable blocks.
> This helper will be used by subsequent commits.
>
> Suggested-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> t/unit-tests/u-reftable-block.c | 47 ++++++++++++++++++++++++-----------------
> 1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
> index f4bded7d26..f4e926ce3a 100644
> --- a/t/unit-tests/u-reftable-block.c
> +++ b/t/unit-tests/u-reftable-block.c
> @@ -14,6 +14,31 @@ license that can be found in the LICENSE file or at
> #include "reftable/reftable-error.h"
> #include "strbuf.h"
>
> +static int cl_reftable_write_block(struct reftable_buf *buf,
> + uint8_t block_type,
> + struct reftable_record *recs,
> + size_t nrecs)
Yeah, I suggested:
int cl_reftable_write_block(struct reftable_buf *buf, uint8_t block_type,
size_t block_size, uint32_t header_off,
struct reftable_record *recs, size_t nrecs)which accepts `size_t block_size` and `uint32_t header_off` as arguments, so that more existing tests could be refactored using cl_reftable_write_block().
Your choice to not have these extra arguments is reasonable though, as they are not needed for the code that your series adds, and they make the implementation of cl_reftable_write_block() a bit more complex.
Also they can still be added in the future if we really want to clean up more existing tests.
This version of your series looks good to me now.
Thanks.
Re: [PATCH v2 05/12] t/unit-tests: introduce test helper to write reftable blocks
Christian Couder <christian.couder@gmail.com> writes:
Show 25 quoted lines
> On Mon, Jun 29, 2026 at 11:02 AM Patrick Steinhardt <ps@pks.im> wrote:
>> ...
>> +static int cl_reftable_write_block(struct reftable_buf *buf,
>> + uint8_t block_type,
>> + struct reftable_record *recs,
>> + size_t nrecs)
>
> Yeah, I suggested:
>
> int cl_reftable_write_block(struct reftable_buf *buf, uint8_t block_type,
> size_t block_size, uint32_t header_off,
> struct reftable_record *recs, size_t nrecs)
>
> which accepts `size_t block_size` and `uint32_t header_off` as
> arguments, so that more existing tests could be refactored using
> cl_reftable_write_block().
>
> Your choice to not have these extra arguments is reasonable though, as
> they are not needed for the code that your series adds, and they make
> the implementation of cl_reftable_write_block() a bit more complex.
>
> Also they can still be added in the future if we really want to clean
> up more existing tests.
>
> This version of your series looks good to me now.
Re: [PATCH v2 06/12] reftable/block: fix OOB write with bogus inflated log size
Patrick Steinhardt <ps@pks.im> writes:
Show 31 quoted lines
> 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?
Show 9 quoted lines
> +
> + 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
Re: [PATCH v2 07/12] reftable/block: fix OOB read with bogus block size
Patrick Steinhardt <ps@pks.im> writes:
Show 89 quoted lines
> 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>
> ---
> reftable/block.c | 9 +++++++++
> t/unit-tests/u-reftable-block.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+)
>
> diff --git a/reftable/block.c b/reftable/block.c
> index b86cb9ec5a..4d6b11c2e7 100644
> --- 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 088162483e..43b9d5fb59 100644
> --- a/t/unit-tests/u-reftable-block.c
> +++ b/t/unit-tests/u-reftable-block.c
> @@ -497,3 +497,36 @@ void test_reftable_block__corrupt_log_block_size(void)
> reftable_block_release(&block);
> reftable_buf_release(&data);
> }
> +
> +void test_reftable_block__corrupt_block_size(void)
> +{
> + struct reftable_block_source source = { 0 };
> + 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 = REFTABLE_BUF_INIT;
> +
> + cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
> +
> + /*
> + * 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);Same here, would it make sense to write a size that's `+1` too much?
uint8_t *p = (uint8_t *)data.buf + 1;
uint32_t block_size = reftable_get_be24(p);
cl_assert_equal_i(block_size, 47);
reftable_put_be24(p, block_size + 1);
Show 14 quoted lines
> +
> + 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);
> + reftable_buf_release(&data);
> +}
>
> --
> 2.55.0.rc2.803.g1fd1e6609c.dirty
>
>
--
Cheers,
Toon
Re: [PATCH v2 06/12] reftable/block: fix OOB write with bogus inflated log size
On Fri, Jul 03, 2026 at 11:23:37AM +0200, Toon Claes wrote:
Show 53 quoted lines
> 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
Re: [PATCH v2 07/12] reftable/block: fix OOB read with bogus block size
On Fri, Jul 03, 2026 at 11:28:21AM +0200, Toon Claes wrote:
Show 40 quoted lines
> 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 088162483e..43b9d5fb59 100644
> > --- a/t/unit-tests/u-reftable-block.c
> > +++ b/t/unit-tests/u-reftable-block.c
> > @@ -497,3 +497,36 @@ void test_reftable_block__corrupt_log_block_size(void)
> > reftable_block_release(&block);
> > reftable_buf_release(&data);
> > }
> > +
> > +void test_reftable_block__corrupt_block_size(void)
> > +{
> > + struct reftable_block_source source = { 0 };
> > + 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 = REFTABLE_BUF_INIT;
> > +
> > + cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
> > +
> > + /*
> > + * 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);
>
> Same here, would it make sense to write a size that's `+1` too much?
>
> uint8_t *p = (uint8_t *)data.buf + 1;
> uint32_t block_size = reftable_get_be24(p);
> cl_assert_equal_i(block_size, 47);
> reftable_put_be24(p, block_size + 1);In this case it'd work indeed though, and I agree that this is a sensible change.
Patrick
[PATCH v3 00/12] reftable: harden against corrupted tables
Hi,
this patch series addresses a bunch of errors that may happen when trying to read corrupted tables. These errors include out-of-bounds writes, out-of-bounds reads and the ability to hit abort(3p) calls.
The out-of-bounds write was originally reported by awo on the security mailing list. As we never transfer reftables over the protocol it would require local disk access to create such corrupted reftables, so there isn't really an easy way to exploit these.
In any case, I took that chance and wrote a fuzzer for parsing the tables, which surfaced a bunch of issues. At the end of this series though the fuzzer can now run for an extended amount of time (2hrs+) without surfacing any new issues.
Changes in v3:
- Make one of the unit tests a bit stricter.
- Link to v2: https://patch.msgid.link/20260629-pks-reftable-hardening-v2-0-b0228e7d908d@pks.im
Changes in v2:
- Introduce a test helper that writes a reftable block.
- Link to v1: https://patch.msgid.link/20260624-pks-reftable-hardening-v1-0-66e4ce87c6b9@pks.im
Thanks!
Patrick
---
Patrick Steinhardt (12):
meson: support building fuzzers with libFuzzer
oss-fuzz: add fuzzer for parsing reftables
reftable/basics: fix OOB read on binary search of empty range
reftable/record: don't abort when decoding invalid ref value type
t/unit-tests: introduce test helper to write reftable blocks
reftable/block: fix OOB write with bogus inflated log size
reftable/block: fix OOB read with bogus block size
reftable/block: fix OOB read with bogus restart count
reftable/block: fix use of uninitialized memory when binsearch fails
reftable/block: fix OOB read with bogus restart offset
reftable/table: fix NULL pointer access when seeking to bogus offsets
reftable/table: fix OOB read on truncated table Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
meson.build | 15 +++
meson_options.txt | 2 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 ++++++++++++++
oss-fuzz/meson.build | 2 +
reftable/basics.c | 3 +
reftable/block.c | 39 +++++++-
reftable/record.c | 6 +-
reftable/table.c | 7 ++
t/unit-tests/u-reftable-basics.c | 11 +++
t/unit-tests/u-reftable-block.c | 189 ++++++++++++++++++++++++++++++++----
t/unit-tests/u-reftable-record.c | 24 +++++
t/unit-tests/u-reftable-table.c | 91 +++++++++++++++++
15 files changed, 440 insertions(+), 26 deletions(-)
Range-diff versus v2:
1: cd28489ce9 = 1: 53673a5044 meson: support building fuzzers with libFuzzer
2: 2f304e5bef = 2: 06037f8cc6 oss-fuzz: add fuzzer for parsing reftables
3: d74a5b244d = 3: 21b4025e1d reftable/basics: fix OOB read on binary search of empty range
4: f710410b28 = 4: dfed39b876 reftable/record: don't abort when decoding invalid ref value type
5: 0f90c22ccf = 5: badbfdc070 t/unit-tests: introduce test helper to write reftable blocks
6: b69a8c40a0 = 6: b0d13e79c5 reftable/block: fix OOB write with bogus inflated log size
7: 19046dbb2e ! 7: 612df3c909 reftable/block: fix OOB read with bogus block size
@@ Commit message
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)
+ ==2274138==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c3ff6de2e3f at pc 0x55555598c6ea bp 0x7fffffff4ee0 sp 0x7fffffff4ed8
+ READ of size 1 at 0x7c3ff6de2e3f thread T0
+ #0 0x55555598c6e9 in reftable_get_be16 /home/pks/Development/git/build/../reftable/basics.h:119:20
+ #1 0x55555598c252 in reftable_block_init /home/pks/Development/git/build/../reftable/block.c:343:18
+ #2 0x555555813c70 in test_reftable_block__corrupt_block_size /home/pks/Development/git/build/../t/unit-tests/u-reftable-block.c:531:20
+ #3 0x5555557f684e in clar_run_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:335:3
+ #4 0x5555557f2e69 in clar_run_suite /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:431:3
+ #5 0x5555557f2882 in clar_test_run /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:636:4
+ #6 0x5555557f375f in clar_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:687:11
+ #7 0x5555557fa49d in cmd_main /home/pks/Development/git/build/../t/unit-tests/unit-test.c:62:8
+ #8 0x55555584b8aa in main /home/pks/Development/git/build/../common-main.c:9:11
+ #9 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b284) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
+ #10 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b337) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
+ #11 0x555555694c24 in _start (/home/pks/Development/git/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.
+ 0x7c3ff6de2e3f is located 0 bytes after 47-byte region [0x7c3ff6de2e10,0x7c3ff6de2e3f)
+ allocated by thread T0 here:
+ #0 0x55555579e95b in malloc (/home/pks/Development/git/build/t/unit-tests+0x24a95b)
+ #1 0x5555559871c2 in reftable_malloc /home/pks/Development/git/build/../reftable/basics.c:24:9
+ #2 0x5555559872e8 in reftable_calloc /home/pks/Development/git/build/../reftable/basics.c:54:6
+ #3 0x55555598f0d3 in reftable_buf_read_data /home/pks/Development/git/build/../reftable/blocksource.c:67:2
+ #4 0x55555598ea7e in block_source_read_data /home/pks/Development/git/build/../reftable/blocksource.c:41:19
+ #5 0x55555598c555 in read_block /home/pks/Development/git/build/../reftable/block.c:224:9
+ #6 0x55555598b69e in reftable_block_init /home/pks/Development/git/build/../reftable/block.c:258:9
+ #7 0x555555813c70 in test_reftable_block__corrupt_block_size /home/pks/Development/git/build/../t/unit-tests/u-reftable-block.c:531:20
+ #8 0x5555557f684e in clar_run_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:335:3
+ #9 0x5555557f2e69 in clar_run_suite /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:431:3
+ #10 0x5555557f2882 in clar_test_run /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:636:4
+ #11 0x5555557f375f in clar_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:687:11
+ #12 0x5555557fa49d in cmd_main /home/pks/Development/git/build/../t/unit-tests/unit-test.c:62:8
+ #13 0x55555584b8aa in main /home/pks/Development/git/build/../common-main.c:9:11
+ #14 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b284) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
+ #15 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b337) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
+ #16 0x555555694c24 in _start (/home/pks/Development/git/build/t/unit-tests+0x140c24)
+
+ SUMMARY: AddressSanitizer: heap-buffer-overflow /home/pks/Development/git/build/../reftable/basics.h:119:20 in reftable_get_be16
+ Shadow bytes around the buggy address:
+ 0x7c3ff6de2b80: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
+ 0x7c3ff6de2c00: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
+ 0x7c3ff6de2c80: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa
+ 0x7c3ff6de2d00: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa
+ 0x7c3ff6de2d80: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd fd fd
+ =>0x7c3ff6de2e00: fa fa 00 00 00 00 00[07]fa fa fa fa fa fa fa fa
+ 0x7c3ff6de2e80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
+ 0x7c3ff6de2f00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
+ 0x7c3ff6de2f80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
+ 0x7c3ff6de3000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
+ 0x7c3ff6de3080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
+ Shadow byte legend (one shadow byte represents 8 application bytes):
+ Addressable: 00
+ Partially addressable: 01 02 03 04 05 06 07
+ Heap left redzone: fa
+ Freed heap region: fd
+ Stack left redzone: f1
+ Stack mid redzone: f2
+ Stack right redzone: f3
+ Stack after return: f5
+ Stack use after scope: f8
+ Global redzone: f9
+ Global init order: f6
+ Poisoned by user: f7
+ Container overflow: fc
+ Array cookie: ac
+ Intra object redzone: bb
+ ASan internal: fe
+ Left alloca redzone: ca
+ Right alloca redzone: cb
Verify that the claimed block size fits into the block data before using
it.
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_siz
+ };
+ struct reftable_block block = { 0 };
+ struct reftable_buf data = REFTABLE_BUF_INIT;
++ uint32_t block_size;
++ unsigned char *p;
+
+ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
@@ t/unit-tests/u-reftable-block.c: void test_reftable_block__corrupt_log_block_siz
+ * 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);
++ p = (unsigned char *) data.buf + 1;
++ block_size = reftable_get_be24(p);
++ cl_assert_equal_i(block_size, 47);
++ reftable_put_be24(p, block_size + 1);
+
+ block_source_from_buf(&source, &data);
+ cl_assert_equal_i(reftable_block_init(&block, &source, 0, 0, data.len,
8: bc0e0e5f30 = 8: 6df5cc741a reftable/block: fix OOB read with bogus restart count
9: 668108e851 = 9: 74e7d35bd9 reftable/block: fix use of uninitialized memory when binsearch fails
10: bb3f76e084 = 10: 3264933eba reftable/block: fix OOB read with bogus restart offset
11: 6b7e61728b = 11: 638964395c reftable/table: fix NULL pointer access when seeking to bogus offsets
12: ea051207e1 = 12: 6745a7fca8 reftable/table: fix OOB read on truncated table--- base-commit: ab776a62a78576513ee121424adb19597fbb7613 change-id: 20260623-pks-reftable-hardening-f54de69fea63
[PATCH v3 01/12] meson: support building fuzzers with libFuzzer
To support fuzzing via libFuzzer one has to pass a couple of compiler options:
- It is mandatory to enable the "fuzzer-no-link" sanitizer for
coverage feedback. - It is recommended to enable at least one more sanitizer to catch
issues, like the "address" sanitizer. - The fuzzing executables need to be linked with "-fsanitize=fuzzer"
to wire up libFuzzer itself.The first two items can already be achieved via the "-Db_sanitize=" option. But the last item cannot easily be achieved, as we can only configure global link arguments.
Introduce a new "-Dfuzzers_link_args=" build option to plug this gap. Add documentation so that users know how to set up libFuzzer.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
meson.build | 15 +++++++++++++++
meson_options.txt | 2 ++
oss-fuzz/meson.build | 1 +
3 files changed, 18 insertions(+)
Show changes to 3 files +18 −0
meson.build, meson_options.txt, oss-fuzz/meson.build
diff --git a/meson.build b/meson.build
index 3247697f74..9df6fbb0a5 100644
--- a/meson.build
+++ b/meson.build
@@ -161,6 +161,21 @@
# These machine files can be passed to `meson setup` via the `--native-file`
# option.
#
+# Fuzzing
+# =======
+#
+# Meson supports building the fuzzing targets by setting `-Dfuzzers=true`. By
+# default, the targets will be built without libFuzzer and thus won't be usable
+# for fuzzing. You have to configure a couple of options to properly wire up
+# libFuzzer:
+#
+# $ meson setup build-fuzzers \
+# -Db_sanitize=address,fuzzer-no-link \
+# -Dfuzzers=true \
+# -Dfuzzers_link_args=-fsanitize=fuzzer
+# $ meson compile -C build-fuzzers
+# $ ./build-fuzzers/oss-fuzz/fuzz-config <args>
+#
# Cross compilation
# =================
#
diff --git a/meson_options.txt b/meson_options.txt
index d936ada098..dc88f130d7 100644
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -131,3 +131,5 @@ option('test_utf8_locale', type: 'string',
description: 'Name of a UTF-8 locale used for testing.')
option('fuzzers', type: 'boolean', value: false,
description: 'Enable building fuzzers.')
+option('fuzzers_link_args', type: 'array', value: [],
+ description: 'Linker arguments used to link fuzzers. Use -fsanitize=fuzzer for fuzzing.')
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 878afd8426..10bcac2f6d 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -16,5 +16,6 @@ foreach fuzz_program : fuzz_programs
fuzz_program,
],
dependencies: [libgit_commonmain],
+ link_args: get_option('fuzzers_link_args'),
)
endforeach
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 02/12] oss-fuzz: add fuzzer for parsing reftables
Add a new fuzzer that exercises our parsing of reftables. Fallout from this fuzzer will be fixed over subsequent commits.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Makefile | 1 +
ci/run-build-and-minimal-fuzzers.sh | 1 +
oss-fuzz/.gitignore | 1 +
oss-fuzz/fuzz-reftable.c | 74 +++++++++++++++++++++++++++++++++++++
oss-fuzz/meson.build | 1 +
5 files changed, 78 insertions(+)
Show changes to 5 files +78 −0
Makefile, ci/run-build-and-minimal-fuzzers.sh, oss-fuzz/.gitignore, oss-fuzz/fuzz-reftable.c, oss-fuzz/meson.build
diff --git a/Makefile b/Makefile
index 1cec251f43..89d3edd5ea 100644
--- a/Makefile
+++ b/Makefile
@@ -2599,6 +2599,7 @@ FUZZ_OBJS += oss-fuzz/fuzz-date.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-headers.o
FUZZ_OBJS += oss-fuzz/fuzz-pack-idx.o
FUZZ_OBJS += oss-fuzz/fuzz-parse-attr-line.o
+FUZZ_OBJS += oss-fuzz/fuzz-reftable.o
FUZZ_OBJS += oss-fuzz/fuzz-url-decode-mem.o
.PHONY: fuzz-objs
fuzz-objs: $(FUZZ_OBJS)
diff --git a/ci/run-build-and-minimal-fuzzers.sh b/ci/run-build-and-minimal-fuzzers.sh
index e7b97952e7..37b24b092d 100755
--- a/ci/run-build-and-minimal-fuzzers.sh
+++ b/ci/run-build-and-minimal-fuzzers.sh
@@ -21,6 +21,7 @@ date
pack-headers
pack-idx
parse-attr-line
+reftable
url-decode-mem
"
diff --git a/oss-fuzz/.gitignore b/oss-fuzz/.gitignore
index f2d74de457..dc7a127a62 100644
--- a/oss-fuzz/.gitignore
+++ b/oss-fuzz/.gitignore
@@ -5,4 +5,5 @@ fuzz-date
fuzz-pack-headers
fuzz-pack-idx
fuzz-parse-attr-line
+fuzz-reftable
fuzz-url-decode-mem
diff --git a/oss-fuzz/fuzz-reftable.c b/oss-fuzz/fuzz-reftable.c
new file mode 100644
index 0000000000..c46eac2c6b
--- /dev/null
+++ b/oss-fuzz/fuzz-reftable.c
@@ -0,0 +1,74 @@
+#include "git-compat-util.h"
+#include "reftable/basics.h"
+#include "reftable/blocksource.h"
+#include "reftable/reftable-blocksource.h"
+#include "reftable/reftable-error.h"
+#include "reftable/reftable-iterator.h"
+#include "reftable/reftable-record.h"
+#include "reftable/reftable-table.h"
+#include "reftable/reftable-writer.h"
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size);
+
+int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)
+{
+ struct reftable_block_source source = { 0 };
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ struct reftable_table *table = NULL;
+ int err;
+
+ if (reftable_buf_add(&buf, (const char *)data, size) < 0)
+ goto out;
+ block_source_from_buf(&source, &buf);
+
+ err = reftable_table_new(&table, &source, "fuzz-input");
+ if (err < 0)
+ goto out;
+
+ /*
+ * Exercise the ref, log and raw block iterators so that we cover as
+ * much of the parsing code as possible.
+ */
+ {
+ struct reftable_ref_record ref = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_ref_iterator(table, &it);
+ if (!reftable_iterator_seek_ref(&it, ""))
+ while (!reftable_iterator_next_ref(&it, &ref))
+ ;
+
+ reftable_ref_record_release(&ref);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+
+ reftable_table_init_log_iterator(table, &it);
+ if (!reftable_iterator_seek_log(&it, ""))
+ while (!reftable_iterator_next_log(&it, &log))
+ ;
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ }
+
+ {
+ struct reftable_table_iterator it = { 0 };
+ const struct reftable_block *block;
+
+ if (!reftable_table_iterator_init(&it, table))
+ while (!reftable_table_iterator_next(&it, &block))
+ ;
+
+ reftable_table_iterator_release(&it);
+ }
+
+out:
+ if (table)
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+ return 0;
+}
diff --git a/oss-fuzz/meson.build b/oss-fuzz/meson.build
index 10bcac2f6d..5a3854256b 100644
--- a/oss-fuzz/meson.build
+++ b/oss-fuzz/meson.build
@@ -6,6 +6,7 @@ fuzz_programs = [
'fuzz-pack-headers.c',
'fuzz-pack-idx.c',
'fuzz-parse-attr-line.c',
+ 'fuzz-reftable.c',
'fuzz-url-decode-mem.c',
]
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 03/12] reftable/basics: fix OOB read on binary search of empty range
`binsearch()` performs a binary search over a range of `sz` elements by repeatedly calling the comparison function with indices into that range. When the range is empty though, there is no valid index to call the comparison function with. We still end up executing the comparison function though with an index of 0, which of course will cause an out-of-bounds read.
Return early when the range is empty.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/basics.c | 3 +++
t/unit-tests/u-reftable-basics.c | 11 +++++++++++
2 files changed, 14 insertions(+)
Show changes to 2 files +14 −0
reftable/basics.c, t/unit-tests/u-reftable-basics.c
diff --git a/reftable/basics.c b/reftable/basics.c
index e969927b61..f0442a46cf 100644
--- a/reftable/basics.c
+++ b/reftable/basics.c
@@ -152,6 +152,9 @@ size_t binsearch(size_t sz, int (*f)(size_t k, void *args), void *args)
size_t lo = 0;
size_t hi = sz;
+ if (!sz)
+ return 0;
+
/* Invariants:
*
* (hi == sz) || f(hi) == true
diff --git a/t/unit-tests/u-reftable-basics.c b/t/unit-tests/u-reftable-basics.c
index 73566ed0eb..c5d83b6714 100644
--- a/t/unit-tests/u-reftable-basics.c
+++ b/t/unit-tests/u-reftable-basics.c
@@ -60,6 +60,17 @@ void test_reftable_basics__binsearch(void)
}
}
+static int unreachable_lesseq(size_t i UNUSED, void *args UNUSED)
+{
+ cl_fail("comparison function called for empty range");
+ return 0;
+}
+
+void test_reftable_basics__binsearch_empty(void)
+{
+ cl_assert_equal_i(binsearch(0, &unreachable_lesseq, NULL), 0);
+}
+
void test_reftable_basics__names_length(void)
{
const char *a[] = { "a", "b", NULL };
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 04/12] reftable/record: don't abort when decoding invalid ref value type
When decoding a ref record we read its value type from the block. In case the type itself is invalid we call `abort()`. This is rather heavy-handed though: the data we're reading is untrusted, so we should treat the issue as a normal and not as a programming error.
Fix this by handling the error gracefully. Note that this also requires us to set the value type later, as otherwise we might store an invalid type in the record.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/record.c | 6 +++---
t/unit-tests/u-reftable-record.c | 24 ++++++++++++++++++++++++
2 files changed, 27 insertions(+), 3 deletions(-)
Show changes to 2 files +27 −3
reftable/record.c, t/unit-tests/u-reftable-record.c
diff --git a/reftable/record.c b/reftable/record.c
index fcd387ba5d..1fce441930 100644
--- a/reftable/record.c
+++ b/reftable/record.c
@@ -388,7 +388,6 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
r->refname[key.len] = 0;
r->update_index = update_index;
- r->value_type = val_type;
switch (val_type) {
case REFTABLE_REF_VAL1:
if (in.len < hash_size) {
@@ -426,9 +425,10 @@ static int reftable_ref_record_decode(void *rec, struct reftable_buf key,
case REFTABLE_REF_DELETION:
break;
default:
- abort();
- break;
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
}
+ r->value_type = val_type;
return start.len - in.len;
diff --git a/t/unit-tests/u-reftable-record.c b/t/unit-tests/u-reftable-record.c
index 1bf2e170dc..9c95083ef4 100644
--- a/t/unit-tests/u-reftable-record.c
+++ b/t/unit-tests/u-reftable-record.c
@@ -11,6 +11,7 @@
#include "reftable/basics.h"
#include "reftable/constants.h"
#include "reftable/record.h"
+#include "reftable/reftable-error.h"
static void t_copy(struct reftable_record *rec)
{
@@ -202,6 +203,29 @@ void test_reftable_record__ref_record_roundtrip(void)
reftable_buf_release(&scratch);
}
+void test_reftable_record__ref_record_decode_invalid_value_type(void)
+{
+ struct reftable_buf scratch = REFTABLE_BUF_INIT;
+ struct reftable_record out = {
+ .type = REFTABLE_BLOCK_TYPE_REF,
+ };
+ struct reftable_buf key = REFTABLE_BUF_INIT;
+ uint8_t buffer[1024] = { 0 };
+ struct string_view dest = {
+ .buf = buffer,
+ .len = sizeof(buffer),
+ };
+
+ cl_must_pass(reftable_buf_addstr(&key, "refs/heads/master"));
+ cl_assert_equal_i(reftable_record_decode(&out, key, REFTABLE_NR_REF_VALUETYPES,
+ dest, REFTABLE_HASH_SIZE_SHA1, &scratch),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_record_release(&out);
+ reftable_buf_release(&key);
+ reftable_buf_release(&scratch);
+}
+
void test_reftable_record__log_record_comparison(void)
{
struct reftable_record in[3] = {
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 05/12] t/unit-tests: introduce test helper to write reftable blocks
Introduce a new test helper that allows us to write reftable blocks. This helper will be used by subsequent commits.
Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
t/unit-tests/u-reftable-block.c | 47 ++++++++++++++++++++++++-----------------
1 file changed, 28 insertions(+), 19 deletions(-)
Show changes to t/unit-tests/u-reftable-block.c +28 −19
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index f4bded7d26..f4e926ce3a 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -14,6 +14,31 @@ license that can be found in the LICENSE file or at
#include "reftable/reftable-error.h"
#include "strbuf.h"
+static int cl_reftable_write_block(struct reftable_buf *buf,
+ uint8_t block_type,
+ struct reftable_record *recs,
+ size_t nrecs)
+{
+ struct block_writer writer = {
+ .last_key = REFTABLE_BUF_INIT,
+ };
+ uint8_t block[1024];
+ int block_end;
+
+ cl_must_pass(block_writer_init(&writer, block_type, block, 1024,
+ 0, hash_size(REFTABLE_HASH_SHA1)));
+ for (size_t i = 0; i < nrecs; i++)
+ cl_must_pass(block_writer_add(&writer, &recs[i]));
+
+ block_end = block_writer_finish(&writer);
+ cl_assert(block_end > 0);
+
+ cl_must_pass(reftable_buf_add(buf, block, block_end));
+
+ block_writer_release(&writer);
+ return block_end;
+}
+
void test_reftable_block__read_write(void)
{
const int header_off = 21; /* random */
@@ -381,25 +406,13 @@ void test_reftable_block__ref_read_write(void)
void test_reftable_block__iterator(void)
{
struct reftable_block_source source = { 0 };
- struct block_writer writer = {
- .last_key = REFTABLE_BUF_INIT,
- };
struct reftable_record expected_refs[20];
struct reftable_ref_record ref = { 0 };
struct reftable_iterator it = { 0 };
struct reftable_block block = { 0 };
- struct reftable_buf data;
+ struct reftable_buf data = REFTABLE_BUF_INIT;
int err;
- data.len = 1024;
- REFTABLE_CALLOC_ARRAY(data.buf, data.len);
- cl_assert(data.buf != NULL);
-
- err = block_writer_init(&writer, REFTABLE_BLOCK_TYPE_REF,
- (uint8_t *) data.buf, data.len,
- 0, hash_size(REFTABLE_HASH_SHA1));
- cl_assert(!err);
-
for (size_t i = 0; i < ARRAY_SIZE(expected_refs); i++) {
expected_refs[i] = (struct reftable_record) {
.type = REFTABLE_BLOCK_TYPE_REF,
@@ -409,13 +422,10 @@ void test_reftable_block__iterator(void)
},
};
memset(expected_refs[i].u.ref.value.val1, i, REFTABLE_HASH_SIZE_SHA1);
-
- err = block_writer_add(&writer, &expected_refs[i]);
- cl_assert_equal_i(err, 0);
}
- err = block_writer_finish(&writer);
- cl_assert(err > 0);
+ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF,
+ expected_refs, ARRAY_SIZE(expected_refs));
block_source_from_buf(&source, &data);
reftable_block_init(&block, &source, 0, 0, data.len,
@@ -453,6 +463,5 @@ void test_reftable_block__iterator(void)
reftable_ref_record_release(&ref);
reftable_iterator_destroy(&it);
reftable_block_release(&block);
- block_writer_release(&writer);
reftable_buf_release(&data);
}
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 06/12] reftable/block: fix OOB write with bogus inflated log size
The "log" reftable block stores reflog information. This information is compressed using zlib. The inflated size is stored in the block header so that callers can easily learn ahead of time how large of a buffer they have to allocate to inflate the data in a single pass. So to reconstruct the full inflated block we:
- Copy over the header as-is, as it's not deflated.
- Append the inflated data to the buffer.
The inflated block size stored in the header also includes the length of the header itself. So to figure out the bytes that should be inflated by zlib we need to subtract the header size, which is trusted data, from the block size, which is untrusted data derived from the block header.
While we do verify that we were able to inflate all data as expected, we don't verify ahead of time that the encoded block length is larger than the header length. This can lead to an underflow, which makes zlib assume that it can write more data into the target buffer than we have allocated. The result is an out-of-bounds write:
==1422297==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c1ff6de5231 at pc 0x55555579a628 bp 0x7fffffff4f10 sp 0x7fffffff46d0
WRITE of size 4 at 0x7c1ff6de5231 thread T0
#0 0x55555579a627 in __asan_memcpy (./build/t/unit-tests+0x246627)
#1 0x55555598b093 in reftable_block_init ./build/../reftable/block.c:277:3
#2 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20
#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 0x55555584af4a 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) 0x7c1ff6de5231 is located 0 bytes after 1-byte region [0x7c1ff6de5230,0x7c1ff6de5231)
allocated by thread T0 here:
#0 0x55555579db1b in realloc.part.0 asan_malloc_linux.cpp.o
#1 0x5555559868d7 in reftable_realloc ./build/../reftable/basics.c:36:9
#2 0x55555598a98f in reftable_alloc_grow ./build/../reftable/basics.h:229:10
#3 0x55555598ae58 in reftable_block_init ./build/../reftable/block.c:269:3
#4 0x555555813701 in test_reftable_block__corrupt_log_block_size ./build/../t/unit-tests/u-reftable-block.c:495:20
#5 0x5555557f684e in clar_run_test ./build/../t/unit-tests/clar/clar.c:335:3
#6 0x5555557f2e69 in clar_run_suite ./build/../t/unit-tests/clar/clar.c:431:3
#7 0x5555557f2882 in clar_test_run ./build/../t/unit-tests/clar/clar.c:636:4
#8 0x5555557f375f in clar_test ./build/../t/unit-tests/clar/clar.c:687:11
#9 0x5555557fa49d in cmd_main ./build/../t/unit-tests/unit-test.c:62:8
#10 0x55555584af4a in main ./build/../common-main.c:9:11
#11 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#12 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#13 0x555555694c24 in _start (./build/t/unit-tests+0x140c24) SUMMARY: AddressSanitizer: heap-buffer-overflow (./build/t/unit-tests+0x246627) in __asan_memcpy
Shadow bytes around the buggy address:
0x7c1ff6de4f80: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5000: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5080: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5100: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x7c1ff6de5180: fa fa fd fd fa fa fd fd fa fa fd fa fa fa fd fd
=>0x7c1ff6de5200: fa fa 04 fa fa fa[01]fa fa fa fa fa fa fa fa fa
0x7c1ff6de5280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c1ff6de5480: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cbFix the bug by adding a sanity check and add a unit test.
Reported-by: oxsignal <awo@kakao.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 32 ++++++++++++++++++++++++++++++++
2 files changed, 41 insertions(+)
Show changes to 2 files +41 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 920b3f4486..b86cb9ec5a 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -260,6 +260,15 @@ int reftable_block_init(struct reftable_block *block,
goto done;
}
+ /*
+ * Verify that the block size covers at least the table header, block
+ * header and the 2 byte restart counter.
+ */
+ if (block_size < header_size + 4 + 2) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
if (block_type == REFTABLE_BLOCK_TYPE_LOG) {
uint32_t block_header_skip = 4 + header_size;
uLong dst_len = block_size - block_header_skip;
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);
+
+ 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);
+}
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 07/12] 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:
==2274138==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7c3ff6de2e3f at pc 0x55555598c6ea bp 0x7fffffff4ee0 sp 0x7fffffff4ed8
READ of size 1 at 0x7c3ff6de2e3f thread T0
#0 0x55555598c6e9 in reftable_get_be16 /home/pks/Development/git/build/../reftable/basics.h:119:20
#1 0x55555598c252 in reftable_block_init /home/pks/Development/git/build/../reftable/block.c:343:18
#2 0x555555813c70 in test_reftable_block__corrupt_block_size /home/pks/Development/git/build/../t/unit-tests/u-reftable-block.c:531:20
#3 0x5555557f684e in clar_run_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:335:3
#4 0x5555557f2e69 in clar_run_suite /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:431:3
#5 0x5555557f2882 in clar_test_run /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:636:4
#6 0x5555557f375f in clar_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:687:11
#7 0x5555557fa49d in cmd_main /home/pks/Development/git/build/../t/unit-tests/unit-test.c:62:8
#8 0x55555584b8aa in main /home/pks/Development/git/build/../common-main.c:9:11
#9 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b284) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
#10 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b337) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
#11 0x555555694c24 in _start (/home/pks/Development/git/build/t/unit-tests+0x140c24) 0x7c3ff6de2e3f is located 0 bytes after 47-byte region [0x7c3ff6de2e10,0x7c3ff6de2e3f)
allocated by thread T0 here:
#0 0x55555579e95b in malloc (/home/pks/Development/git/build/t/unit-tests+0x24a95b)
#1 0x5555559871c2 in reftable_malloc /home/pks/Development/git/build/../reftable/basics.c:24:9
#2 0x5555559872e8 in reftable_calloc /home/pks/Development/git/build/../reftable/basics.c:54:6
#3 0x55555598f0d3 in reftable_buf_read_data /home/pks/Development/git/build/../reftable/blocksource.c:67:2
#4 0x55555598ea7e in block_source_read_data /home/pks/Development/git/build/../reftable/blocksource.c:41:19
#5 0x55555598c555 in read_block /home/pks/Development/git/build/../reftable/block.c:224:9
#6 0x55555598b69e in reftable_block_init /home/pks/Development/git/build/../reftable/block.c:258:9
#7 0x555555813c70 in test_reftable_block__corrupt_block_size /home/pks/Development/git/build/../t/unit-tests/u-reftable-block.c:531:20
#8 0x5555557f684e in clar_run_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:335:3
#9 0x5555557f2e69 in clar_run_suite /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:431:3
#10 0x5555557f2882 in clar_test_run /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:636:4
#11 0x5555557f375f in clar_test /home/pks/Development/git/build/../t/unit-tests/clar/clar.c:687:11
#12 0x5555557fa49d in cmd_main /home/pks/Development/git/build/../t/unit-tests/unit-test.c:62:8
#13 0x55555584b8aa in main /home/pks/Development/git/build/../common-main.c:9:11
#14 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b284) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
#15 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/8kvxvr3pmsypxiypq4g8zy13glnfr7nx-glibc-2.42-67/lib/libc.so.6+0x2b337) (BuildId: 5a702452a01df1d7d50ce0663acec7be3c71fd4d)
#16 0x555555694c24 in _start (/home/pks/Development/git/build/t/unit-tests+0x140c24) SUMMARY: AddressSanitizer: heap-buffer-overflow /home/pks/Development/git/build/../reftable/basics.h:119:20 in reftable_get_be16
Shadow bytes around the buggy address:
0x7c3ff6de2b80: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x7c3ff6de2c00: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fa
0x7c3ff6de2c80: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa
0x7c3ff6de2d00: fa fa fd fd fd fd fd fd fa fa fd fd fd fd fd fa
0x7c3ff6de2d80: fa fa 00 00 00 00 00 00 fa fa fd fd fd fd fd fd
=>0x7c3ff6de2e00: fa fa 00 00 00 00 00[07]fa fa fa fa fa fa fa fa
0x7c3ff6de2e80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c3ff6de2f00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c3ff6de2f80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c3ff6de3000: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7c3ff6de3080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cbVerify that the claimed block size fits into the block data before using it.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 38 ++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+)
Show changes to 2 files +47 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index b86cb9ec5a..4d6b11c2e7 100644
--- 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 088162483e..6995eb8bf1 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -497,3 +497,41 @@ void test_reftable_block__corrupt_log_block_size(void)
reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_block_size(void)
+{
+ struct reftable_block_source source = { 0 };
+ 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 = REFTABLE_BUF_INIT;
+ uint32_t block_size;
+ unsigned char *p;
+
+ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * 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.
+ */
+ p = (unsigned char *) data.buf + 1;
+ block_size = reftable_get_be24(p);
+ cl_assert_equal_i(block_size, 47);
+ reftable_put_be24(p, block_size + 1);
+
+ 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);
+ reftable_buf_release(&data);
+}
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 08/12] reftable/block: fix OOB read with bogus restart count
The restart count is stored in the last two bytes of a block. We use it without verification to compute the offset of the restart table. With a bogus restart count that is large enough this computation underflows, and the subsequent reads via the restart table access out-of-bounds memory:
==129439==ERROR: AddressSanitizer: SEGV on unknown address 0x7d90f6dcd0ad (pc 0x55555598ce89 bp 0x7fffffff4ed0 sp 0x7fffffff4e80 T0)
==129439==The signal is caused by a READ memory access.
#0 0x55555598ce89 in reftable_get_be24 ./git/build/../reftable/basics.h:125:9
#1 0x55555598eabf in block_restart_offset ./git/build/../reftable/block.c:407:9
#2 0x55555598e5d5 in restart_needle_less ./git/build/../reftable/block.c:431:17
#3 0x5555559887e2 in binsearch ./git/build/../reftable/basics.c:165:13
#4 0x55555598dfec in block_iter_seek_key ./git/build/../reftable/block.c:529:6
#5 0x555555814517 in test_reftable_block__corrupt_restart_count ./git/build/../t/unit-tests/u-reftable-block.c:593:15
#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 0x55555584c12a 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) ==129439==Register values:
rax = 0x00007d90f6dcd0ad rbx = 0x00007fffffff4f20 rcx = 0xf2f2f2f8f2f2f2f8 rdx = 0x0000000000000000
rdi = 0x00007d90f6dcd0ad rsi = 0x0000000000007fff rbp = 0x00007fffffff4ed0 rsp = 0x00007fffffff4e80
r8 = 0x0000000000000000 r9 = 0x0000000000000000 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff58e8 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x00005555560550b0
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/basics.h:125:9 in reftable_get_be24
Verify that the restart table actually fits into the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 4 ++++
t/unit-tests/u-reftable-block.c | 33 +++++++++++++++++++++++++++++++++
2 files changed, 37 insertions(+)
Show changes to 2 files +37 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 4d6b11c2e7..4d285aefd7 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -351,6 +351,10 @@ int reftable_block_init(struct reftable_block *block,
restart_count = reftable_get_be16(block->block_data.data + block_size - 2);
restart_off = block_size - 2 - 3 * restart_count;
+ if (restart_off < header_size + 4 || restart_off > block_size - 2) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
block->block_type = block_type;
block->hash_size = hash_size;
diff --git a/t/unit-tests/u-reftable-block.c b/t/unit-tests/u-reftable-block.c
index 6995eb8bf1..d77048cdd0 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -535,3 +535,36 @@ void test_reftable_block__corrupt_block_size(void)
reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_count(void)
+{
+ struct reftable_block_source source = { 0 };
+ 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 = REFTABLE_BUF_INIT;
+ int block_size;
+
+ block_size = cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ /*
+ * Corrupt the restart count to claim a bogus number of restart points.
+ * Note that this would only cause us to perform an out-of-bounds
+ * access when seeking into the block, but we want to refuse such a
+ * block outright.
+ */
+ reftable_put_be16((uint8_t *) data.buf + block_size - 2, 0xffff);
+
+ 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);
+ reftable_buf_release(&data);
+}
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 09/12] reftable/block: fix use of uninitialized memory when binsearch fails
When doing the binary search through our restart offsets we may hit an error in case `restart_needle_less()` fails to decode the record at the given offset. While we correctly detect this case and error out, it will cause us to call `reftable_record_release()` on the yet-uninitialized record.
Fix this by initializing the record earlier.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/block.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to reftable/block.c +4 −4
diff --git a/reftable/block.c b/reftable/block.c
index 4d285aefd7..89efce8751 100644
--- a/reftable/block.c
+++ b/reftable/block.c
@@ -517,6 +517,10 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
int err = 0;
size_t i;
+ err = reftable_record_init(&rec, reftable_block_type(it->block));
+ if (err < 0)
+ goto done;
+
/*
* Perform a binary search over the block's restart points, which
* avoids doing a linear scan over the whole block. Like this, we
@@ -558,10 +562,6 @@ int block_iter_seek_key(struct block_iter *it, struct reftable_buf *want)
else
it->next_off = it->block->header_off + 4;
- err = reftable_record_init(&rec, reftable_block_type(it->block));
- if (err < 0)
- goto done;
-
/*
* We're looking for the last entry less than the wanted key so that
* the next call to `block_reader_next()` would yield the wanted
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 10/12] 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>
---
reftable/block.c | 9 +++++++++
t/unit-tests/u-reftable-block.c | 39 +++++++++++++++++++++++++++++++++++++++
2 files changed, 48 insertions(+)
Show changes to 2 files +48 −0
reftable/block.c, t/unit-tests/u-reftable-block.c
diff --git a/reftable/block.c b/reftable/block.c
index 89efce8751..1fa81405d2 100644
--- 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 d77048cdd0..99b155d4bf 100644
--- a/t/unit-tests/u-reftable-block.c
+++ b/t/unit-tests/u-reftable-block.c
@@ -568,3 +568,42 @@ void test_reftable_block__corrupt_restart_count(void)
reftable_block_release(&block);
reftable_buf_release(&data);
}
+
+void test_reftable_block__corrupt_restart_offset(void)
+{
+ struct reftable_block_source source = { 0 };
+ 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 = REFTABLE_BUF_INIT;
+
+ cl_reftable_write_block(&data, REFTABLE_BLOCK_TYPE_REF, &rec, 1);
+
+ 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);
+ reftable_buf_release(&data);
+}
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 11/12] reftable/table: fix NULL pointer access when seeking to bogus offsets
When seeking an iterator to an arbitrary offset we may return a positive value in case the offset points beyond the block. This makes sense when iterating through multiple blocks of the same section, as the positive value indicates to us that we're at the end of the table.
But when the offset originates from a section or index offset it is supposed to point at a valid block, so an out-of-bounds value means that the table is corrupt. Treating it as a normal end-of-iteration causes us to silently report an empty section instead of surfacing the corruption, and we are left with a partially-initialized block. This may later on cause a NULL pointer exception:
==1486841==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x55555598e02c bp 0x7fffffff4eb0 sp 0x7fffffff4e70 T0)
==1486841==The signal is caused by a READ memory access.
==1486841==Hint: address points to the zero page.
#0 0x55555598e02c in reftable_block_type ./git/build/../reftable/block.c:392:9
#1 0x55555598ee6e in block_iter_seek_key ./git/build/../reftable/block.c:536:35
#2 0x5555559ae553 in table_iter_seek_linear ./git/build/../reftable/table.c:344:8
#3 0x5555559adbff in table_iter_seek ./git/build/../reftable/table.c:450:9
#4 0x5555559ada9c in table_iter_seek_void ./git/build/../reftable/table.c:460:9
#5 0x555555992872 in reftable_iterator_seek_log_at ./git/build/../reftable/iter.c:281:9
#6 0x555555992953 in reftable_iterator_seek_log ./git/build/../reftable/iter.c:287:9
#7 0x55555583aa78 in test_reftable_table__seek_invalid_log_offset ./git/build/../t/unit-tests/u-reftable-table.c:257:20
#8 0x5555557f684e in clar_run_test ./git/build/../t/unit-tests/clar/clar.c:335:3
#9 0x5555557f2e69 in clar_run_suite ./git/build/../t/unit-tests/clar/clar.c:431:3
#10 0x5555557f2882 in clar_test_run ./git/build/../t/unit-tests/clar/clar.c:636:4
#11 0x5555557f375f in clar_test ./git/build/../t/unit-tests/clar/clar.c:687:11
#12 0x5555557fa49d in cmd_main ./git/build/../t/unit-tests/unit-test.c:62:8
#13 0x55555584cffa in main ./git/build/../common-main.c:9:11
#14 0x7ffff7a2b284 in __libc_start_call_main (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b284) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#15 0x7ffff7a2b337 in __libc_start_main@GLIBC_2.2.5 (/nix/store/57iz36553175g3178pvxjij8z5rcsd4n-glibc-2.42-61/lib/libc.so.6+0x2b337) (BuildId: 8ae0b698f2d4e727f569f64bb166e08ae30bd077)
#16 0x555555694c24 in _start (./git/build/t/unit-tests+0x140c24) ==1486841==Register values:
rax = 0x0000000000000000 rbx = 0x00007fffffff4ec0 rcx = 0x0000000000000000 rdx = 0x00007cfff6e2bd58
rdi = 0x00007cfff6e2bd58 rsi = 0x00007bfff5da1020 rbp = 0x00007fffffff4eb0 rsp = 0x00007fffffff4e70
r8 = 0x0000000000000000 r9 = 0x0000000000000002 r10 = 0x0000000000000000 r11 = 0x0000000000000017
r12 = 0x00007fffffff5908 r13 = 0x0000000000000001 r14 = 0x00007ffff7ffd000 r15 = 0x0000555556056e90
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV ./git/build/../reftable/block.c:392:9 in reftable_block_type
==1486841==ABORTING
Fix this by returning a proper error in `table_iter_seek_to()` when the offset ranges beyond the block.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 2 ++
t/unit-tests/u-reftable-table.c | 63 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 65 insertions(+)
Show changes to 2 files +65 −0
reftable/table.c, t/unit-tests/u-reftable-table.c
diff --git a/reftable/table.c b/reftable/table.c
index 56362df0ed..f4bc86a29d 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -242,6 +242,8 @@ static int table_iter_seek_to(struct table_iter *ti, uint64_t off, uint8_t typ)
int err;
err = table_init_block(ti->table, &ti->block, off, typ);
+ if (err > 0)
+ return REFTABLE_FORMAT_ERROR;
if (err != 0)
return err;
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index 14fae8b199..c7dca45e70 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -1,8 +1,11 @@
#include "unit-test.h"
#include "lib-reftable.h"
+#include "reftable/basics.h"
+#include "reftable/block.h"
#include "reftable/blocksource.h"
#include "reftable/constants.h"
#include "reftable/iter.h"
+#include "reftable/reftable-error.h"
#include "reftable/table.h"
#include "strbuf.h"
@@ -199,3 +202,63 @@ void test_reftable_table__block_iterator(void)
reftable_buf_release(&buf);
reftable_free(records);
}
+
+void test_reftable_table__seek_invalid_log_offset(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_log_record logs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .update_index = 1,
+ .value_type = REFTABLE_LOG_UPDATE,
+ .value.update = {
+ .name = (char *) "user",
+ .email = (char *) "user@example.com",
+ .message = (char *) "message\n",
+ },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_log_record log = { 0 };
+ struct reftable_iterator it = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+ size_t fsize = footer_size(1);
+ uint8_t *footer;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs),
+ logs, ARRAY_SIZE(logs), NULL);
+
+ /*
+ * Corrupt the log section offset stored in the footer so that it points
+ * past the end of the table. The footer is checksummed, so we also have
+ * to recompute and rewrite the CRC.
+ */
+ footer = (uint8_t *) buf.buf + buf.len - fsize;
+ reftable_put_be64(footer + header_size(1) + 24, UINT64_MAX);
+ reftable_put_be32(footer + fsize - 4, crc32(0, footer, fsize - 4));
+
+ block_source_from_buf(&source, &buf);
+ cl_must_pass(reftable_table_new(&table, &source, "name"));
+
+ /*
+ * Seeking the log iterator must not crash even though the log section
+ * offset is bogus. As the offset points past the end of the table we
+ * know that the table is corrupt, so the seek must report a format
+ * error instead of pretending that the section is empty.
+ */
+ reftable_table_init_log_iterator(table, &it);
+ cl_assert_equal_i(reftable_iterator_seek_log(&it, ""),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_log_record_release(&log);
+ reftable_iterator_destroy(&it);
+ reftable_table_decref(table);
+ reftable_buf_release(&buf);
+}
--
2.55.0.795.g602f6c329a.dirty
[PATCH v3 12/12] reftable/table: fix OOB read on truncated table
When opening a table we compute the size of its data section by subtracting the footer size from the file size. We do not verify that the file is actually large enough to contain both the header and the footer though. With a truncated table the subtraction can thus underflow, causing us to read the footer out of bounds:
SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/pks/Development/git/build/t/unit-tests+0x2479a4) in __asan_memcpy
Shadow bytes around the buggy address:
0x7ccff6e0de80: fa fa fa fa fa fa fa fa fd fd fd fd fd fd fd fd
0x7ccff6e0df00: fd fd fd fd fd fd fd fd fd fa fa fa fa fa fa fa
0x7ccff6e0df80: fa fa fd fd fd fd fd fd fd fd fd fd fd fd fd fd
0x7ccff6e0e000: fd fd fd fd fa fa fa fa fa fa fa fa fd fd fd fd
0x7ccff6e0e080: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa fa
=>0x7ccff6e0e100: fa fa fa fa fa[fa]00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e180: 00 00 00 00 00 00 00 04 fa fa fa fa fa fa fa fa
0x7ccff6e0e200: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
0x7ccff6e0e280: 00 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e300: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x7ccff6e0e380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==1500371==ABORTINGVerify that the file is large enough to contain both the header and the footer before computing the table size.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
reftable/table.c | 5 +++++
t/unit-tests/u-reftable-table.c | 28 ++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
Show changes to 2 files +33 −0
reftable/table.c, t/unit-tests/u-reftable-table.c
diff --git a/reftable/table.c b/reftable/table.c
index f4bc86a29d..b4d3f9e211 100644
--- a/reftable/table.c
+++ b/reftable/table.c
@@ -562,6 +562,11 @@ int reftable_table_new(struct reftable_table **out,
goto done;
}
+ if (file_size < header_size(t->version) + footer_size(t->version)) {
+ err = REFTABLE_FORMAT_ERROR;
+ goto done;
+ }
+
t->size = file_size - footer_size(t->version);
t->source = *source;
t->name = reftable_strdup(name);
diff --git a/t/unit-tests/u-reftable-table.c b/t/unit-tests/u-reftable-table.c
index c7dca45e70..28b0ef5258 100644
--- a/t/unit-tests/u-reftable-table.c
+++ b/t/unit-tests/u-reftable-table.c
@@ -262,3 +262,31 @@ void test_reftable_table__seek_invalid_log_offset(void)
reftable_table_decref(table);
reftable_buf_release(&buf);
}
+
+void test_reftable_table__new_with_truncated_table(void)
+{
+ struct reftable_ref_record refs[] = {
+ {
+ .refname = (char *) "refs/heads/main",
+ .value_type = REFTABLE_REF_VAL1,
+ .value.val1 = { 42 },
+ },
+ };
+ struct reftable_block_source source = { 0 };
+ struct reftable_table *table;
+ struct reftable_buf buf = REFTABLE_BUF_INIT;
+
+ cl_reftable_write_to_buf(&buf, refs, ARRAY_SIZE(refs), NULL, 0, NULL);
+
+ /*
+ * Truncate the table so that it is large enough to read the header, but
+ * too small to also contain the footer.
+ */
+ buf.len = footer_size(1) - 1;
+ block_source_from_buf(&source, &buf);
+
+ cl_assert_equal_i(reftable_table_new(&table, &source, "name"),
+ REFTABLE_FORMAT_ERROR);
+
+ reftable_buf_release(&buf);
+}
--
2.55.0.795.g602f6c329a.dirty
Re: [PATCH v3 00/12] reftable: harden against corrupted tables
Patrick Steinhardt <ps@pks.im> writes:
Show 15 quoted lines
> Hi,
>
> this patch series addresses a bunch of errors that may happen when
> trying to read corrupted tables. These errors include out-of-bounds
> writes, out-of-bounds reads and the ability to hit abort(3p) calls.
>
> The out-of-bounds write was originally reported by awo on the security
> mailing list. As we never transfer reftables over the protocol it would
> require local disk access to create such corrupted reftables, so there
> isn't really an easy way to exploit these.
>
> In any case, I took that chance and wrote a fuzzer for parsing the
> tables, which surfaced a bunch of issues. At the end of this series
> though the fuzzer can now run for an extended amount of time (2hrs+)
> without surfacing any new issues.
Great work on providing the fixes with their corresponding unit tests. I've manually verified if each test, and they all fail correctly without the code changes.
All looks good to me. The only thing I'm not sure about is whether it's useful to have the Asan output in the commit messages.
--
Cheers,
Toon