From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 27 Apr 2022 16:31:07 GMT
Subject: Re: [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged
Message-ID: <57217c15-c6d1-32f1-a481-0003ba66e342@gmail.com>
In-Reply-To: <CAPUEsphEymVE1HHeDZE+Fh50fr7DJSpM_YFNC-v=m9hFhgz-UA@mail.gmail.com>

On 27/04/2022 16:38, Carlo Arenas wrote:
> On Wed, Apr 27, 2022 at 2:33 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 27/04/2022 01:05, Carlo Marcelo Arenas Belón wrote:
> [...]
> FWIW, I still think that using atoi with a check to skip "" is
> probably as safe as doing all this extra checking as no one has shown
> yet a system where sizeof(uid_t) > sizeof(uint32_t), but agree with
> Junio that using long instead avoids issues with the systems where
> sizeof(uid_t) > sizeof(int) and unless sizeof(int) == sizeof(long)
> (ex: 32-bit Linux) which is then covered by the cast.

if sizeof(uid_t) < sizeof(long) then the cast will truncate the value 
returned by strtol() which means we are trusting that SUDO_UID is a 
valid uid otherwise it will be truncated.

>>> +{
>>> +     const char *real_uid = getenv(env);
>>> +
>>> +     if (real_uid && *real_uid) {
>>> +             char *error;
>>> +             long extracted_id = strtol(real_uid, &error, 10);
>>> +             if (!*error && LONG_MIN < extracted_id &&
>>> +                             extracted_id < LONG_MAX) {
>>
>> strtol() returns a long so the last two checks are redundant.
> 
> The last two checks were to check for underflow or overflow and to
> make sure that the bogus values this function returns in case of those
> errors are not taken into consideration.

Sorry I misread the code - this is checking if the value is valid, not 
if there is an error. On platforms where uid_t and long are the same 
size (e.g. where int and long are both 32 bits) the "< LONG_MAX" check 
is rejecting a valid uid. The fact that we are doing these checks makes 
me think we should care about the possible truncation above.

Best Wishes

Phillip

>> The standard is silent on what happens to error when the value is out of
>> range.
> 
> Which is why instead I was avoiding LONG_{MIN,MAX} which are
> described[1] as the bogus values that will be used in that case
> Agree with you that we could also add a check for >=0 as uid_t is
> usually unsigned, but it is not warranted to be so, and that is I was
> aiming for the wider possible range so we don't have to worry that
> much and let it be settled to a valid value through the cast.
> 
> Carlo
> 
> [1] https://pubs.opengroup.org/onlinepubs/007904875/functions/strtol.html