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

From: Carlo Arenas <carenas@gmail.com>
Date: Apr 27, 2022, 20:59 UTC
Message-ID: <CAPUEsph9tUkm+JsvU6z0Wt7mtRcTCH1vbvYTEdWjNyXzdoU=vw@mail.gmail.com>
In-Reply-To: <xmqqzgk6l5ly.fsf@gitster.g>

On Wed, Apr 27, 2022 at 11:54 AM Junio C Hamano <gitster@pobox.com> wrote:

> The "sudo sh to get a shell, chdir to /var/tmp/foo to do something"
> use case does care---it needs to make sure that whereever it goes is
> not part of a hostile repository.  So "if SUDO_UID is there, anything
> goes" is not a good protection for such a use case.

FWIW that was never part of the proposal, indeed making git aware of SUDO_ID when it is running as only as root was part of the design to avoid other users probably allowing repositories they don't control by having an evil SUDO_ID.

as per the root user, I was expecting that he could be trusted more and that wouldn't accidentally get an evil SUDO_ID on their session because it is something that gets generated from their original account and they should be aware of it and even might need to tweak it (ex: by un setting it if it gets in the way).

To make the `sudo make install` use case possible I think it make sense to trust SUDO_ID by default, but I would be open to add a configuration setting that would be required to enable it (it indeed might become useful for root users that might want to disable it as an alternative to unset SUDO_ID).

Show 6 quoted lines

> Possible?  Yes.  Practical?  I dunno.  Especially if we do not
> deliberately truncate by reading into int16_t but use "int" (32-bit
> almost everywhere we care about) or "long".  Now how likely is uid_t
> narrower than long?  If it is not likely, then we probably are OK to
> use long and not worry about the truncation at all.  Or use strtoll()
> to make it even less likely.

strtoll (or my suggested alternative of strtoimax) suffer from portability issues which are handled later in the git-compat-util.h file, so while I think it is worth doing, I would prefer doing so later as part of moving this code into a proper compat module.

for this iteration, the suggestion by Phillip to use strtoul makes more sense (specially after confirmation by Randall that it works in NON-STOP), it is more portable and doesn't require later in the file git-compat magic to work, it allows for the full range of 32bit that might be needed in 32bit *NIX, it is should fit find even in 32bit environments with a 32bit unsigned uid_t (which is fairly common), it doesn't sign extend if applied into a wider uid_t and would be likely able to be assigned even without a cast and without the risk of truncation for most uid_t types. Lastly, since now this code is focused only in sudo compatibility, and sudo casts their uid to an unsigned int and prints it into that variable with "%u" it is the right "API".

For comparison, I would also think it is better to let the compiler warn if needed so at least the users would know their uid_t type is narrower than what we expect and sudo sets, and might work as a weather balloon to find those systems.

>
> So, in short, I think it is reasonable if we did:
>
>  - do the extra "if SUDO_UID exists, then..." only when we are root.

already in since the RFC, see discussion above for an option to tighten it further if really needed, but even in that case I would rather do it in the next iteration.

>  - use strtol to ensure SUDO_UID is in good shape (all digits) and
>    read into long; use errno=0 trick to notice overflows.  err on
>    the safe side and do not allow if the value cannot be read
>    correctly.

already implemented as part of v2, except it is using strtoul/unsigned long. also in the line of "err on the safe side" I was also discarding any values that would overflow an uid_t (which is assumed to be unsigned), which affect the next bullet point.

>  - do the comparison between st.st_uid and strtol() result by
>    casting both to (long).  Without this, st.st_uid has a value that
>    needs wider than long, we cannot allow the owner of such a
>    repository in (and I somehow feel that is unlikely to happen).

do you still want to cast both sides even if discarding values that would overflow an uid_t?

FWIW, as Phillip reported, using long (or another signed type) would restrict the valid ids in 32-bit *NIX to half of what the expected uint32_t range most of them should have.

>  - or omit the third one --- tough luck for those whose UID is
>    beyond what a long can represent (and I somehow feel that this is
>    probably OK).

That was what I proposed in the current RFC (at least in 32bit *NIX) but I have to admit that i'd seen a lot more issues with uid larger than INT_MAX that I expected when doing google for it, so that is what I thought it would be safer to use the full 32-bit and an unsigned type instead.

> Another thing I'd suggest doing is to have a helper function to do
> the "we do a bit more than geteuid() to come up with the allowed
> owner of repositories we encounter", and call that function instead
> of geteuid().

I did this originally, but discarded it quickly because it was a little odd and easy to reuse, for something that I was hoping wouldn't stay in git-compat for too long.

The use of the current helper function covers that use case but is by design harder to reuse while keeping the overall logic of the 2 combined functions more visible.

Once we move this code into a proper compat module, I would be happy to add a geteuid_or_sudo() function but that would be probably after we also fix doas.

Carlo