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

[Carlo Arenas <carenas@gmail.com>]

On Fri, May 6, 2022 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Carlo Arenas <carenas@gmail.com> writes:
>
> > On Fri, May 6, 2022 at 1:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Carlo Arenas <carenas@gmail.com> writes:
> >>
> >> > which is also why we can't use it, any possibly bogus or suspicious
> >> > value we get from SUDO_UID MUST be ignored.
> >>
> >> I do not think I agree.  If we have a strange value in SUDO_UID, it
> >> would be much better and safer to err on the safe side.
> >
> > ignoring it is the safe side; for example if we replace the current
> > function with the proposed one then some user lucky enough to have
> > access to the latest linux supercomputer that has been patched to have
> > a 64-bit uid_t (because who makes 32-bit supercomputers nowadays)
> > would get root[1] access by simply faking his SUDO_UID to be UINT_MAX
> > + 1.
>
> Since we do not pay attention to SUDO_UID unless euid is root,
> anybody who can attack by faking SUDO_UID to affect what Git does
> can already become root on the box.

Normally yes, but they just might be too clever and found instead a way to
fool sudo into generating a bogus SUDO_UID with the hopes that whoever
implemented this forgot to check for bogus values and used atoi() and
therefore grants them uid = 0 for whatever nefarious objective they
might have.

While we can't protect against a buggy sudo, we can't do the next best
thing and that is to NOT let a would-be attacker still get their 5 min
of fame by reporting a vulnerability in git, because they just made it
crash through sudo.

Especially when the impact of just ignoring them and going our merry
way is that (with the current code) we will use the euid of the user
that started sudo and "do the right thing" (tm).

> In any case, if we notice that SUDO_UID is not a valid number and
> die(), we deny the access anyway, so there is no need to write more
> code to ignore.

not sure which additional code you are referring to here, but
currently the only code to ignore is embedded in the one that is
needed to read the value, so it is not additional, except maybe for
the (uid_t)-1 check that was discussed before[1] and that is what
protects us from overflows when sizeof(long) > sizeof(uid_t) &&
UID_T_MIN >= 0, which are likely most of the systems we are expected
to run on.

if you are referring to the whole function that could be replaced by
using instead git_env_ulong() as dscho suggested, additional code
would be needed anyway to make sure that we truncate it appropriately
as what we really need is a git_env_uint32() which I am really glad to
add if you would rather go that way, but is definitely going to
require more code than what we have right now.

Note also that we will need to add a flag to it, so it doesn't try to
be helpful and allow SUDO_UID=0M to sneak in.

Carlo

[1] https://lore.kernel.org/git/CAPUEsphFb-=BcV-mxS5RZpJQ8UVq23ni0Lo8tQ4J3TP04B4KQg@mail.gmail.com/