Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Jan 9, 2025, 11:14 UTC
- Message-ID
- <Z3-vpLHvxoQCTjY1@pks.im>
- In-Reply-To
- <c439fcaf-11af-7862-9c3c-18dc0842b57d@gmx.de>
On Thu, Jan 09, 2025 at 11:49:43AM +0100, Johannes Schindelin wrote:
Show 16 quoted lines
> > diff --git a/builtin/blame.c b/builtin/blame.c > > index 867032e4c16878ffd56df8a73162b89ca4bd2694..ad91fe9e97f90625dd2708fbd44bf2dd24a337a6 100644 > > --- a/builtin/blame.c > > +++ b/builtin/blame.c > > @@ -475,6 +475,8 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int > > char ch; > > size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ? > > the_hash_algo->hexsz : (size_t) abbrev; > > + if (length > GIT_MAX_HEXSZ) > > + length = GIT_MAX_HEXSZ; > > This causes a subtle change of behavior because there are a couple of > conditional code blocks between this change and the `printf()` call > decrease `length`, i.e. specifying values larger than the maximal hex size > causes potentially-desirable, different behavior (and think about > https://www.hyrumslaw.com/).
Alternatively we can move this until after we have done the subtractions. Then we don't have to do weird gymnastics.
Show 25 quoted lines
> >
> > if (opt & OUTPUT_COLOR_LINE) {
> > if (cnt > 0) {
> > @@ -505,7 +507,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
> > length--;
> > putchar('?');
> > }
> > - fwrite(hex, 1, length, stdout);
> > + printf("%.*s", (int)length, 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 0147de304b4d104cc7f05ea1f8d68f1a07ceb80d..fcaba8c11f7ede084e069eefd292f337e8396cb4 100755
> > --- a/t/t8002-blame.sh
> > +++ b/t/t8002-blame.sh
> > @@ -126,6 +126,10 @@ 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 9000 --abbrev=$hexsz HEAD
>
> This is actually incorrect: it passes `--abbrev=$hexsz` instead of a value
> that needs to be truncated.Oh dear. The test did manage to catch the bug, but thinking more about it that was only because my initial fix was broken.
Show 20 quoted lines
> diff --git a/builtin/blame.c b/builtin/blame.c
> index ad91fe9e97f9..5b4976835066 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -475,8 +475,13 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
> char ch;
> size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?
> the_hash_algo->hexsz : (size_t) abbrev;
> - if (length > GIT_MAX_HEXSZ)
> - length = GIT_MAX_HEXSZ;
> +
> + /*
> + * Leave enough space for ^, * and ? indicators (boundary,
> + * unblamable, ignored).
> + */
> + if (length > GIT_MAX_HEXSZ + 3)
> + length = GIT_MAX_HEXSZ + 3;
>
> if (opt & OUTPUT_COLOR_LINE) {
> if (cnt > 0) {How about this instead?
diff --git a/builtin/blame.c b/builtin/blame.c
index ad91fe9e97..f92e487bed 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -475,8 +475,6 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
char ch;
size_t length = (opt & OUTPUT_LONG_OBJECT_NAME) ?
the_hash_algo->hexsz : (size_t) abbrev;
- if (length > GIT_MAX_HEXSZ)
- length = GIT_MAX_HEXSZ;
if (opt & OUTPUT_COLOR_LINE) {
if (cnt > 0) {
@@ -507,6 +505,9 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
length--;
putchar('?');
}
+
+ if (length > GIT_MAX_HEXSZ)
+ length = GIT_MAX_HEXSZ;
printf("%.*s", (int)length, hex);
if (opt & OUTPUT_ANNOTATE_COMPAT) {
const char *name;In that case there's no need to juggle with the magic indicators, which makes it a bit easier to reason about.
Show 11 quoted lines
> diff --git a/t/t8002-blame.sh b/t/t8002-blame.sh > index fcaba8c11f7e..71fa70a64679 100755 > --- a/t/t8002-blame.sh > +++ b/t/t8002-blame.sh > @@ -127,7 +127,7 @@ test_expect_success '--no-abbrev works like --abbrev with full length' ' > ' > > test_expect_success 'blame --abbrev gets truncated' ' > - check_abbrev 9000 --abbrev=$hexsz HEAD > + check_abbrev 9000 --abbrev=9000 HEAD.. > '
This should be `check_abbrev $hexsz --abbrev=9000`, shouldn't it?
Patrick