Re: [PATCH v2] setup: tighten ownership checks post CVE-2022-24765

From: Derrick Stolee <derrickstolee@github.com>
Date: May 5, 2022, 13:58 UTC
Message-ID: <281017cb-0078-0635-8a3b-fed430b5439a@github.com>
In-Reply-To: <0f9aaada-8e72-38b9-e58a-9b26ed7bc780@github.com>

On 5/5/2022 9:14 AM, Derrick Stolee wrote:

Show 6 quoted lines

> On 5/4/2022 8:50 PM, Carlo Marcelo Arenas Belón wrote:
>> -static int ensure_valid_ownership(const char *path)
>> +static int ensure_valid_ownership(const char *worktree, const char *gitdir)
>>  {
>> -	struct safe_directory_data data = { .path = path };
>> +	struct safe_directory_data data = { .path = worktree };

This also seems a bit backwards to me. I think bare repos will have a NULL worktree, but all repos will have a gitdir. I think what we really want is this:

	.path = worktree ? worktree : gitdir

And that might affect the callers of this method allowing the worktree to be NULL.

Show 6 quoted lines

> I think you should remove check_path and instead do the following:
> 
> 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
> 	    is_path_owned_by_current_user(worktree) &&
> 	    (!gitdir || is_path_owned_by_current_user(gitdir)))
>  		return 1;

But that changes my logic here to instead be

 	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
	    is_path_owned_by_current_user(gitdir) &&
	    (!worktree|| is_path_owned_by_current_user(worktree)))
  		return 1;

Thanks, -Stolee