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

From: René Scharfe <l.s.r@web.de>
Date: Jan 24, 2023, 18:53 UTC
Message-ID: <b4b48877-5b80-e96f-d09f-2fe275f42950@web.de>
In-Reply-To: <230123.86a629tzgc.gmgdl@evledraar.gmail.com>

Am 23.01.23 um 09:33 schrieb Ævar Arnfjörð Bjarmason:

Show 30 quoted lines

>
> 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).

Show 6 quoted lines

> 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 :)

Patch v1 cited above still keeps bits beyond S_IFMT (0xF000), so that's at 16 bits on a platform with 32-bit unsigned int for future object types or other metadata.

One bit would suffice to switch the path field into an URL and encode additional metadata there. We could do that even with the stricter patch v2 by using one of the bits between S_IFMT and normal permissions (0777) for that, e.g. the sticky bit.

That all said, the longer I think about mode overflow the less I understand why I sent these patches. It's basically harmless. Perhaps we just need a comment stating that, to contain the urge to "fix" this. Anyway, I'd like to retract my overflowing modes patches (unless someone else really, really wants one of them applied).

René