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

[<rsbecker@nexbridge.com>]

On April 27, 2022 8:31 AM, Phillip Wood wrote:
>On 27/04/2022 10:33, Phillip Wood wrote:
>> Hi Carlo
>>
>> On 27/04/2022 01:05, Carlo Marcelo Arenas Belón wrote:
>
>> [...]
>>> +{
>>> +    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
>> standard is silent on what happens to error when the value is out of
>> range. The way to check that all the string was consumed without
>> underflow/overflow is
>>
>>      errno = 0;
>>      val = strtol(str, &endp, 10);
>>      if (errno || !*endp)
>
>Sorry that should be "if (errno || *endp)" to check for an error
>
> > [...]
> >     #if sizeof(uid_t) == sizeof(int)
> >         if (extracted_id > INT_MAX)
> >             error(...)
> >     #endif
>
>I think we should probably check if uid_t is a short integer as well as
>
> > +#ifdef __TANDEM
> > +#define ROOT_UID 65535
>
>suggests it may be an unsigned short on NonStop.
>
>Not knowing the size of uid_t or if it is signed or not (as far as I can tell posix just
>says it's an integer) makes the limit checks tricky - maybe we make euid a long (or
>unsigned long) here and it the function below rather than casting it to uid_t and
>possibly truncating it.

It depends on the compile. If LP64 is used - long file pointers, then it is a long (32-bit) or an int (also 32-bit). Not unsigned in either case, but specifying unsigned would be helpful. Nice catch.

> > [...]
>>> +
>>>   static inline int is_path_owned_by_current_uid(const char *path)
>>>   {
>>>       struct stat st;
>>> +    uid_t euid;
>>> +
>>>       if (lstat(path, &st))
>>>           return 0;
>>> -    return st.st_uid == geteuid();
>>> +
>>> +    euid = geteuid();
>>> +    if (euid == ROOT_UID) {
>>> +        /* we might have raised our privileges with sudo */
>>> +        extract_id_from_env("SUDO_UID", &euid);
>
>You are ignoring any errors when parsing the environment variable - that is not a
>good idea in a security check.