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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Apr 27, 2022, 16:14 UTC
Message-ID: <b0a80f47-f441-fc6e-3695-58ea83dea2b8@gmail.com>
In-Reply-To: <CAPUEspiHWTTAvNyqqLzcC854UccH=bkPLPBzPaPfzFKn_yt4aA@mail.gmail.com>

On 27/04/2022 16:58, Carlo Arenas wrote:

Show 5 quoted lines

> On Wed, Apr 27, 2022 at 5:30 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>> You are ignoring any errors when parsing the environment variable - that
>> is not a good idea in a security check.
> 
> which errors are you concerned about?, if anything in this code

I was confused by the fact that the helper function returns a success/failure value which we ignore. However euid is not overwritten if strtol fails so it is safe I think.

Show 7 quoted lines

> worries me from a security point of view is the fact that we are
> relying in getenv not being racy (as mentioned in the original RFC),
> but there are no errors set there AFAIK.
> 
> not ignoring errno in strtol is an option, but as mentioned before I
> decided instead to reject bogus values and therefore not the clobber a
> previous errno,

strtol() will set errno if there is a range error ignoring it does not change that. In any case is_path_owned_by_current_uid() already clobbers errno if stat() fails.

Best Wishes

Phillip

> since I was using strtol as a wider version of atoi.

> Carlo