On 5/4/2022 8:50 PM, Carlo Marcelo Arenas Belón wrote:
> 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.
> [...] 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...
> -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
