Subject: Re: [RFC PATCH v4 2/3] git-compat-util: avoid failing dir ownership checks if running privileged

From: Junio C Hamano <[email-masked]>
Date: Mon, 09 May 2022 09:54:52 -0700

Carlo Marcelo Arenas Belón <[email-masked]> writes:

>> In exchange, the only attack scenario the code prevents is this,
>> IIUC.
>> 
>>  * You, the aspiring cracker, are a user not allowed to run "sudo" on
>>    the box, and you know your uid is 1000
>> 
>>  * You look for another user, a potential victim, whose uid is 1000
>>    modulo 65536 (if your uid_t is 16-bit) and who can run "sudo" on
>>    the box.
>> 
>>  * You prepare a malicious repository, invite that user there and
>>    ask them to run "sudo something" there.
>> 
>> I'd say such an attack vector is not likely,...

Sorry, I was totally wrong here.

It is not just "not likely", but such an attack, with a potential
victim not futzing with SUDO_UID environment themselves, would not
work at all.  The value of SUDO_UID and the original uid of the
potential victim by definition should fit in the uid_t type.  So if
you, the aspiring cracker, have UID 1000, nobody else on the system
has UID that is congruent modulo uid_t and wrap-around attack does
not exist.  As long as the type we use to read SUDO_UID string into
a variable is not narrower than uid_t, there.

Of course you can tell any user who runs "sudo" to set SUDO_UID to
1000 + 64k and cause wrap-around, but then you can tell them to set
SUDO_UID to 1000 without relying on wrap-around and have the same
effect.  So, let's stop worrying about this bogus scenario.

As to the "we can break compilation with -Wsign-compare on a system
with signed uid_t", I agree that is true if we have

	env_id <= (uid_t) -1

there.  But I am not sure if that is the most effective way to smoke
out platforms where this code has trouble working correctly.  Also,
I would think that a system with signed uid_t is a possibility, but
a user with a negative UID?

I do not think even nobody4 was negative ;-)