From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 27 Apr 2022 12:30:50 GMT
Subject: Re: [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged
Message-ID: <fcd26e80-daee-2d66-cbdc-b004c478357d@gmail.com>
In-Reply-To: <6522673b-494a-951c-be5e-3ca01577c12b@gmail.com>

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.

 > [...]
>> +
>>   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.

Best Wishes

Phillip

>> +    }
>> +    return st.st_uid == euid;
>>   }
>>   #define is_path_owned_by_current_user is_path_owned_by_current_uid
>