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

From: Derrick Stolee <derrickstolee@github.com>
Date: May 5, 2022, 13:14 UTC
Message-ID: <0f9aaada-8e72-38b9-e58a-9b26ed7bc780@github.com>
In-Reply-To: <20220505005009.27789-1-carenas@gmail.com>

On 5/4/2022 8:50 PM, Carlo Marcelo Arenas Belón wrote:

Show 10 quoted lines

> 8959555cee7 (setup_git_directory(): add an owner check for the top-level
> directory, 2022-03-02), adds a function to check for ownership of
> repositories using a directory that is representative of it (its workdir)
> and ways to add it to an exception list if needed, but that check breaks
> when the ownership of the workdir is not the same than the ownership of
> directory where the configuration and other relevant files reside.
> 
> An attacker could create a git repository in a directory that he has write
> access to but is owned by the victim, and therefore workaround the fix that
> was introduced with CVE-2022-24765 to attack them,

It's worth noting that this requires having the current user owning a directory, but allowing other users to write into it.

Show 5 quoted lines

> [...] like in the following
> scenario which could result in privilege escalation if root then runs a git
> command in that directory or any of its sub directories:
> 
>   $ git -C /tmp init

This /tmp example is an example of why that is actually common, and not just a "user misconfigured their machine" issue.

> To avoid that, extend the ensure_valid_ownership function to be able to
> check for ownership of both the worktree and the gitdir, and use that for
> non bare repositories.

You mention extending the check here, but...

Show 11 quoted lines

> -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 };
> +	const char *check_path;
> +
> +	if (gitdir)
> +		check_path = gitdir;
> +	else
> +		check_path = worktree;

...this makes it appear like you are choosing to check only _one_ of the directories, not _both_, which is what I would expect for a strengthening of our security checks.

>  	if (!git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0) &&
> -	    is_path_owned_by_current_user(path))
> +	    is_path_owned_by_current_user(check_path))
>  		return 1;

Indeed, we are checking only one of these directories for ownership.

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;

Thanks, -Stolee