RE: [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged
On April 27, 2022 8:31 AM, Phillip Wood wrote:
Show 43 quoted lines
>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.
Show 18 quoted lines
> > [...]
>>> +
>>> 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.rsbecker@nexbridge.com <rsbecker@nexbridge.com>