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

[Junio C Hamano <gitster@pobox.com>]

Carlo Arenas <carenas@gmail.com> writes:

> not that there are 2 different scenarios which we might seem to be
> flip flopping about :
>
> 1) victim gets attacked by tricking them into using the wrong SUDO_UID
>
> unlikely to be useful as you pointed out, and totally useless if we
> still restrict this code to run only as root

I think we established that we are not interested in this long ago.

> 2) attacker using this code to elevate privileges themselves
>
> only useful if this code is not run as root.

An attacker who is not root would not make the "what does SUDO_UID
say about the user we came from?", and "git" will not be setuid
binary, so I agree the code is not useful for such an attack,
either.

> My concern again, is not with this code AS-IS but how it might be used
> in the future.


I'd rather not such a "concern" to block us for too long.


>> 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.
>
> bogus only if we are still only running this code as root, of course.

And this code is only run to learn what SUDO_UID says, which is what
we check when we notice geteuid() says we are root.

>> 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
>
> Apologies for not documenting it until now, but I had
> -Wtautological-constant-out-of-range-compare in mind instead, but your
> are correct either one would work and the point was that (without
> having to add even an "static assert") we were able to find them and
> warn them that they need to patch.
>
>>         env_id <= (uid_t) -1
>
> If that was not enough, that simple code ALSO disabled the code in
> that case to make sure they MUST patch locally if they need to make it
> work, or come back to us to figure out a portable way to accommodate
> them in the future, with all the information about their system we
> currently lack.

Without a comment to say that, nobody would be able to figure out
that we are waiting for them to speak up.

	/* the code does not work on a system with signed uid */
	intmax_t tmp = (uid_t) -1;
	if (tmp < 0)
		BUG("report to git@vger that you have a signed uid_t");

or better yet, a compile-time equivalent, perhaps.

>> 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?
>
> It doesn't need to be a real user (specially if someone independently
> decides to remove the restriction that keeps this code available only
> to root).

When they break, they can keep both halves.  Is it our concern?