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

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

Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:

> +		/*
> +		 * env_id could underflow/overflow in the previous call
> +		 * and if it will still fit in a long it will not report
> +		 * it as error with ERANGE, instead silently using an
> +		 * equivalent positive number that might be bogus.
> +		 * if uid_t is narrower than long, it might not fit,
> +		 * hence why we  need to check it against the maximum
> +		 * possible uid_t value before accepting it.
> +		 */
> +		if (!*endptr && !errno && env_id <= (uid_t)-1)
> +			*id = env_id;

Thanks for very clearly spelling out why you care.  It makes it much
easier to explain why I disagree with the line of reasoning ;-)

This code may be exercised by a potential attacker, but we know that
the codepath is entered only when euid==ROOT_UID.  The attacker may
or may not have used 'sudo', and we cannot trust the value of
SUDO_UID at all.  But that is OK.  If the attacker already is root
on the box, they do not have to use "git" or exercise this new code
in order to attack anybody on the box already.  This requires us to
exclude social engineering attack to tell a victim to run "sudo",
set SUDO_UID to a specific value, and run something, but at last I
have been excluding that from the beginning.  There are easier
things you can tell the potential victim to cause harm while being
root.

Now the whole point of adding this new code to _weaken_ the existing
check is to help legitimate users who are authorised to become root
via "sudo" on the box.  Making it easier for them to use "git" while
tentatively gaining root priviledge so that they can do "make
install" in a repository they own.

We know that this code is meant to be exercised after a potential
victim gained euid==ROOT_UID via 'sudo', and SUDO_UID is exported by
the command for the original user.  If uid_t is narrower than ulong
(e.g. 16-bit uid_t vs 64-bit ulong), and if it is unsigned, the only
effect the extra check is doing is to exclude the unfortunate user
with uid==65535 from using "sudo git describe".

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, and a user with maximum
allowed uid_t value is equally not that likely, so I do not care too
deeply either way---and in such a case, I do prefer a simpler code.