On Fri, Mar 8, 2024 at 1:20 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> If you have /var/tmp/primary/ as a repository, and if you create a
> secondary worktree of it at /var/tmp/secondary/, the layout would
> look like this:
>
> $ cd /var/tmp/
> $ git init primary
> $ cd primary
> $ pwd
> /var/tmp/primary
> $ git worktree add ../secondary
> $ cat ../seconary/.git
Nit: typo, should be `secondary` (missing the `d`)
> gitdir: /var/tmp/primary/.git/worktrees/secondary
> $ ls /var/tmp/primary/.git/worktrees/secondary
> commondir gitdir HEAD index refs
> $ cat /var/tmp/primary/.git/worktrees/secondary/gitdir
> /var/tmp/secondary/.git
>
> When the configuration variable 'safe.bareRepository=explicit' is
> set to explicit, the change made by 45bb9162 (setup: allow cwd=.git
> w/ bareRepository=explicit, 2024-01-20) allows you to work in the
> /var/tmp/primary/.git directory (i.e., $GIT_DIR of the primary
> worktree). The idea is that if it is safe to work in the repository
> in its working tree, it should be equally safe to work in the
> ".git/" directory of that working tree, too.
>
> Now, for the same reason, let's allow command execution from within
> the $GIT_DIR directory of a secondary worktree. This is useful for
> tools working with secondary worktrees when the 'bareRepository'
> setting is set to 'explicit'.
>
> In the previous commit, we created a helper function to house the
> logic that checks if a directory that looks like a bare repository
> is actually a part of a non-bare repository. Extend the helper
> function to also check if the apparent bare-repository is a $GIT_DIR
> of a secondary worktree, by checking three things:
>
> * The path to the $GIT_DIR must be a subdirectory of
> ".git/worktrees/", which is the primary worktree [*].
>
> * Such $GIT_DIR must have file "gitdir", that records the path of
> the ".git" file that is at the root level of the secondary
> worktree.
>
> * That ".git" file in turn points back at the $GIT_DIR we are
> inspecting.
>
> The latter two points are merely for checking sanity. The security
> lies in the first requirement.
>
> Remember that a tree object with an entry whose pathname component
> is ".git" is forbidden at various levels (fsck, object transfer and
> checkout), so malicious projects cannot cause users to clone and
> checkout a crafted ".git" directory in a shell directory that
> pretends to be a working tree with that ".git" thing at its root
> level. That is where 45bb9162 (setup: allow cwd=.git w/
> bareRepository=explicit, 2024-01-20) draws its security guarantee
> from. And the solution for secondary worktrees in this commit draws
> its security guarantee from the same place.
>
> Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx>
> ---
> setup.c | 52 ++++++++++++++++++++++++++++++++-
> t/t0035-safe-bare-repository.sh | 8 ++++-
> 2 files changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 3081be4970..68860dcd18 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1231,9 +1231,59 @@ static const char *allowed_bare_repo_to_string(
> return NULL;
> }
>
> +static int is_git_dir_of_secondary_worktree(const char *path)
> +{
> + int result = 0; /* assume not */
> + struct strbuf gitfile_here = STRBUF_INIT;
> + struct strbuf gitfile_there = STRBUF_INIT;
> + const char *gitfile_contents;
> + int error_code = 0;
> +
> + /*
> + * We should be a subdirectory of /.git/worktrees inside
> + * the $GIT_DIR of the primary worktree.
> + *
> + * NEEDSWORK: some folks create secondary worktrees out of a
> + * bare repository; they don't count ;-), at least not yet.
> + */
> + if (!strstr(path, "/.git/worktrees/"))
Do we need to be concerned about path separators being different on
Windows? Or have they already been normalized here?
> + goto out;
> +
> + /*
> + * Does gitdir that points at the ".git" file at the root of
> + * the secondary worktree roundtrip here?
> + */
What loss of security do we have if we don't have as stringent of a
check? i.e. if we just did `return !!strstr(path, "/.git/worktrees/)`?
Or maybe we even combine the existing ends_with(.git) check with this
and do something like:
static int is_under_dotgit_dir(const char *path) {
char *dotgit = strstr(path, "/.git");
return dotgit && (dotgit[5] == '\0' || dotgit[5] == '/');
}
> + strbuf_addf(&gitfile_here, "%s/gitdir", path);
> + if (!file_exists(gitfile_here.buf))
> + goto out;
> + if (strbuf_read_file(&gitfile_there, gitfile_here.buf, 0) < 0)
> + goto out;
> + strbuf_trim_trailing_newline(&gitfile_there);
> +
> + gitfile_contents = read_gitfile_gently(gitfile_there.buf, &error_code);
> + if ((!gitfile_contents) || strcmp(gitfile_contents, path))
> + goto out;
> +
> + /* OK, we are happy */
> + result = 1;
> +
> +out:
> + strbuf_release(&gitfile_here);
> + strbuf_release(&gitfile_there);
> + return result;
> +}
> +
> static int is_repo_with_working_tree(const char *path)
> {
> - return ends_with_path_components(path, ".git");
> + /* $GIT_DIR immediately below the primary working tree */
> + if (ends_with_path_components(path, ".git"))
> + return 1;
> +
> + /* Are we in $GIT_DIR of a secondary worktree? */
> + if (is_git_dir_of_secondary_worktree(path))
> + return 1;
> +
> + return 0;
> }
>
> /*
> diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
> index 8048856379..62cdfcefc1 100755
> --- a/t/t0035-safe-bare-repository.sh
> +++ b/t/t0035-safe-bare-repository.sh
> @@ -31,7 +31,9 @@ expect_rejected () {
>
> test_expect_success 'setup bare repo in worktree' '
> git init outer-repo &&
> - git init --bare outer-repo/bare-repo
> + git init --bare outer-repo/bare-repo &&
> + git -C outer-repo worktree add ../outer-secondary &&
> + test_path_is_dir outer-secondary
> '
>
> test_expect_success 'safe.bareRepository unset' '
> @@ -86,4 +88,8 @@ test_expect_success 'no trace when "bare repository" is a subdir of .git' '
> expect_accepted_implicit -C outer-repo/.git/objects
> '
>
> +test_expect_success 'no trace in $GIT_DIR of secondary worktree' '
> + expect_accepted_implicit -C outer-repo/.git/worktrees/outer-secondary
> +'
> +
> test_done
> --
> 2.44.0-165-ge09f1254c5
>
