Re: [PATCH] builtin/blame: fix out-of-bounds read with excessive `--abbrev`

[Patrick Steinhardt <ps@pks.im>]

On Fri, Jan 10, 2025 at 10:27:23AM +0100, Johannes Schindelin wrote:
> Hi Patrick,
> 
> On Thu, 9 Jan 2025, Johannes Schindelin wrote:
> 
> > On Thu, 9 Jan 2025, Patrick Steinhardt wrote:
> >
> > > On Thu, Jan 09, 2025 at 11:49:43AM +0100, Johannes Schindelin wrote:
> > > > > 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.
> >
> > Or we can even avoid assiging a maximum altogether:
> >
> > 		if (length < GIT_MAX_HEXSZ)
> > 			printf("%.*s", (int)length, hex);
> > 		else
> > 			printf("%s", hex);
> >
> > Or be more consistent with Git's source code style which often prefers
> > ternaries, favoring succinctness over readability:
> >
> > 		printf("%.*s", (int)(length < GIT_MAX_HEXSZ ? length : GIT_MAX_HEXSZ), hex);
> 
> Coverity noticed a problem with this approach, looking at
> https://github.com/git/git/blob/v2.48.0-rc2/builtin/blame.c#L493:
> 
> 				memset(hex, ' ', length);
> 
> If the `GIT_MAX_HEXSZ` guard is moved after this statement, then we can
> easily overrun the `hex` buffer.

Oh. That's even an old-standing issue that wasn't caused by the
refactoring, right? Your proposed fix to set `length = GIT_MAX_HEXSZ +
3` to account for the old behaviour wouldn't fix it either, as we could
still end up overwriting two bytes.

I'll send a new version with another commit on top to fix this.

Patrick