From: Phillip Wood Date: Wed, 27 Apr 2022 16:14:27 GMT Subject: Re: [PATCH] git-compat-util: avoid failing dir ownership checks if running privileged Message-ID: In-Reply-To: On 27/04/2022 16:58, Carlo Arenas wrote: > On Wed, Apr 27, 2022 at 5:30 AM Phillip Wood 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. > 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