Re: [PATCH] tree-walk: disallow overflowing modes

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Jan 21 2023, René Scharfe wrote:

> When parsing tree entries, reject mode values that don't fit into an
> unsigned int.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  tree-walk.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/tree-walk.c b/tree-walk.c
> index 74f4d710e8..5e7bc38600 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -17,6 +17,8 @@ static const char *get_mode(const char *str, unsigned int *modep)
>  	while ((c = *str++) != ' ') {
>  		if (c < '0' || c > '7')
>  			return NULL;
> +		if ((mode << 3) >> 3 != mode)
> +			return NULL;
>  		mode = (mode << 3) + (c - '0');
>  	}
>  	*modep = mode;

There was a discussion about this on git-security last August, in a
report that turned out to be uninteresting for the security aspects.

I'll just quote my own reply here out of context
(<220811.86mtcbqd5x.gmgdl@evledraar.gmail.com> for those with access).

For context the other issue in the "two issues" below is the one I'm
working towards fixing in
https://lore.kernel.org/git/cover-0.4-00000000000-20221118T113442Z-avarab@gmail.com/,
the other is this file mode overflow.

As noted at the end below I'm conflicted between whether we should "fix"
this in some way, or just intentionally leave it alone, because while
it's entirely accidental, this is the one part of git's object format
I'm aware of that we could sneak in some extension in the future,
without entirely breaking backwards compatibility.

BEGIN QUOTE

There's really two issues being raised here, how we validate the "mode"
in tree entries, and how we sometimes misreport object type X as type Y.

I replied on-list just now noting that the "mode" thing is something I
was working towards fixing a while ago, but am happy to see a more
isolated fix for:
https://lore.kernel.org/git/220811.86r11nqfi2.gmgdl@evledraar.gmail.com/

I have local patches for the misreporting of object types, it's not
*that* hard to get right. Basically we just need to be more careful
about how we populate the "struct object". The most common case is when
we parse tags that say on their envelope that we refer to a type X, but
it's really a type Y.

In that case we haven't .parsed=1 the object, but we note the wrong type
down. My local patches are basically just deferring that, so we don't
trust such type claims, and take our *actual* parsing of the object over
a previous reference in the object store.

I don't think that leaves any difficult edge cases related to tree
parsing, as Xavier notes we'd have modes claiming that an X is Y, but we
can resolve it using the same principle.

But regarding the "mode parsing" I think
https://lore.kernel.org/git/YvQdR3sDqDMCIjIE@coredump.intra.peff.net/ is
taking us in the wrong direction, but wasn't comfortable saying so on
list, because this is "exploitable" in the following way:

	$ perl -wE 'say unpack "B*", "hello world"'
	0110100001100101011011000110110001101111001000000111011101101111011100100110110001100100

Which, combined with Jeff's series on-list you can try e.g.:
	
	diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
	index ac4099ca893..d435dfd64c5 100755
	--- a/t/t5504-fetch-receive-strict.sh
	+++ b/t/t5504-fetch-receive-strict.sh
	@@ -357,16 +357,16 @@ test_expect_success 'badFilemode is not a strict error' '
	 	tree=$(
	 		cd badmode.git &&
	 		blob=$(echo blob | git hash-object -w --stdin | hex2oct) &&
	-		printf "123456 foo\0${blob}" |
	+		printf "106640110100001100101011011000110110001101111001000000111011101101111011100100110110001100100664 foo\0${blob}" |
	 		git hash-object -t tree --stdin -w --literally
	 	) &&
	 
	 	rm -rf dst.git &&
	 	git init --bare dst.git &&
	 	git -C dst.git config transfer.fsckObjects true &&
	-
	-	git -C badmode.git push ../dst.git $tree:refs/tags/tree 2>err &&
	-	grep "$tree: badFilemode" err
	+	git -C badmode.git push ../dst.git $tree:refs/tags/tree &&
	+	git -C badmode.git fsck &&
	+	git -C dst.git fsck
	 '
	 
	 test_done
	diff --git a/tree-walk.c b/tree-walk.c
	index 74f4d710e8f..2fb9f2e6cbe 100644
	--- a/tree-walk.c
	+++ b/tree-walk.c
	@@ -15,6 +15,7 @@ static const char *get_mode(const char *str, unsigned int *modep)
	 		return NULL;
	 
	 	while ((c = *str++) != ' ') {
	+		fprintf(stderr, "%c\n", c);
	 		if (c < '0' || c > '7')
	 			return NULL;
	 		mode = (mode << 3) + (c - '0');

Which gets you all tests passing, i.e. the particulars of the mode check
are such that we'll accept a very long mode (but if you play with it
you'll find it's not infinite, we'll run into other buffers elsewhere
pretty soon).

This is all assuming you're on a system whose "unsigned int" is 64 bit.

This part of it is also something I discovered in the beginning of 2021
(and there's some old off-list thread between myself & Elijah on the
topic I could dig up), but didn't report here at the time.

So, we could just "fix" it, and re the "wrong direction" above
downgrading the "bad mode" check to a mere warning is going a bit too
far given the above. We have some odd modes in the wild, but we don't
have such "overflowing modes".

On the other hand this edge case is also a golden opportunity we're not
likely to ever have again. We can't really change the git object format
at this point without *major* headaches, but in this case we have the
ability to encode arbitrary data into tree entries (e.g file metadata)
as long as the writer makes sure they overflow back to the valid
filemode :)

As hacky as that is I think it would be regrettable to forever close the
door on that to fix a hypothetical security bug, hypothetical because no
version of git.git has an issue with it. But it is a *potential* edge
case in overflowing any other tree parsing code out there in the wild
(which might be otherwise guarded by a stricter fsck check of ours).

END QUOTE