From 1fbb8d7ecb7bd78ac55d226b6b073372a5ea2c2d Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 10 Jan 2025 12:26:17 +0100 Subject: [PATCH 1/2] builtin/blame: fix out-of-bounds read with excessive `--abbrev` In 6411a0a896 (builtin/blame: fix type of `length` variable when emitting object ID, 2024-12-06) we have fixed the type of the `length` variable. In order to avoid a cast from `size_t` to `int` in the call to printf(3p) with the "%.*s" formatter we have converted the code to instead use fwrite(3p), which accepts the length as a `size_t`. It was reported though that this makes us read over the end of the OID array when the provided `--abbrev=` length exceeds the length of the object ID. This is because fwrite(3p) of course doesn't stop when it sees a NUL byte, whereas printf(3p) does. Fix the bug by reverting back to printf(3p) and culling the provided length to `GIT_MAX_HEXSZ` to keep it from overflowing when cast to an `int`. Reported-by: Johannes Schindelin Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/blame.c | 3 ++- t/t8002-blame.sh | 8 ++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/builtin/blame.c b/builtin/blame.c index 867032e4c1..d7630ac89c 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -505,7 +505,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int length--; putchar('?'); } - fwrite(hex, 1, length, stdout); + + printf("%.*s", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex); if (opt & OUTPUT_ANNOTATE_COMPAT) { const char *name; if (opt & OUTPUT_SHOW_EMAIL) diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh index 0147de304b..b3f8b63d2e 100755 --- a/t/t8002-blame.sh +++ b/t/t8002-blame.sh @@ -126,6 +126,14 @@ test_expect_success '--no-abbrev works like --abbrev with full length' ' check_abbrev $hexsz --no-abbrev ' +test_expect_success 'blame --abbrev gets truncated' ' + check_abbrev $hexsz --abbrev=9000 HEAD +' + +test_expect_success 'blame --abbrev gets truncated with boundary commit' ' + check_abbrev $hexsz --abbrev=9000 ^HEAD +' + test_expect_success '--exclude-promisor-objects does not BUG-crash' ' test_must_fail git blame --exclude-promisor-objects one ' From e7fb2ca94556e6aadfc3038afaa1c8cc3525258c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 10 Jan 2025 12:26:18 +0100 Subject: [PATCH 2/2] builtin/blame: fix out-of-bounds write with blank boundary commits When passing the `-b` flag to git-blame(1), then any blamed boundary commits which were marked as uninteresting will not get their actual commit ID printed, but will instead be replaced by a couple of spaces. The flag can lead to an out-of-bounds write as though when combined with `--abbrev=` when the abbreviation length is longer than `GIT_MAX_HEXSZ` as we simply use memset(3p) on that array with the user-provided length directly. The result is most likely that we segfault. An obvious fix would be to cull `length` to `GIT_MAX_HEXSZ` many bytes. But when the underlying object ID is SHA1, and if the abbreviated length exceeds the SHA1 length, it would cause us to print more bytes than desired, and the result would be misaligned. Instead, fix the bug by computing the length via strlen(3p). This makes us write as many bytes as the formatted object ID requires and thus effectively limits the length of what we may end up printing to the length of its hash. If `--abbrev=` asks us to abbreviate to something shorter than the full length of the underlying hash function it would be handled by the call to printf(3p) correctly. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/blame.c | 6 +++--- t/t8002-blame.sh | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index d7630ac89c..7555c445ab 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -489,9 +489,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int fputs(color, stdout); if (suspect->commit->object.flags & UNINTERESTING) { - if (blank_boundary) - memset(hex, ' ', length); - else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) { + if (blank_boundary) { + memset(hex, ' ', strlen(hex)); + } else if (!(opt & OUTPUT_ANNOTATE_COMPAT)) { length--; putchar('^'); } diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh index b3f8b63d2e..1ad039e123 100755 --- a/t/t8002-blame.sh +++ b/t/t8002-blame.sh @@ -134,6 +134,24 @@ test_expect_success 'blame --abbrev gets truncated with boundary commit' ' check_abbrev $hexsz --abbrev=9000 ^HEAD ' +test_expect_success 'blame --abbrev -b truncates the blank boundary' ' + # Note that `--abbrev=` always gets incremented by 1, which is why we + # expect 11 leading spaces and not 10. + cat >expect <<-EOF && + $(printf "%0.s " $(test_seq 11)) ( 2005-04-07 15:45:13 -0700 1) abbrev + EOF + git blame -b --abbrev=10 ^HEAD -- abbrev.t >actual && + test_cmp expect actual +' + +test_expect_success 'blame with excessive --abbrev and -b culls to hash length' ' + cat >expect <<-EOF && + $(printf "%0.s " $(test_seq $hexsz)) ( 2005-04-07 15:45:13 -0700 1) abbrev + EOF + git blame -b --abbrev=9000 ^HEAD -- abbrev.t >actual && + test_cmp expect actual +' + test_expect_success '--exclude-promisor-objects does not BUG-crash' ' test_must_fail git blame --exclude-promisor-objects one '