patchsetup: allow cwd=.git w/ bareRepository=explicit

> From: Kyle Lippincott <spectral@google.com>
>
> The safe.bareRepository setting can be set to 'explicit' to disallow
> implicit uses of bare repositories, preventing an attack [1] where an
> artificial and malicious bare repository is embedded in another git
> repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd
> when executing commands, and this is blocked when
> safe.bareRepository=explicit. Blocking is unnecessary, as git already
> prevents nested .git directories.

>     I'm assuming this isn't a significant concern, for two reasons:
>     
>      * most filesystems/OSes in use today (by number of users) are at least
>        case-preserving, so users/tools will have had to type out .GIT
>        instead of getting it from readdir/wherever.
>      * this is primarily a "quality of life" change to the feature, and if
>        we get it wrong we still fail closed.

>
> "Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Kyle Lippincott <spectral@google.com>
> >
> > The safe.bareRepository setting can be set to 'explicit' to disallow
> > implicit uses of bare repositories, preventing an attack [1] where an
> > artificial and malicious bare repository is embedded in another git
> > repository. Unfortunately, some tooling uses myrepo/.git/ as the cwd
> > when executing commands, and this is blocked when
> > safe.bareRepository=explicit. Blocking is unnecessary, as git already
> > prevents nested .git directories.
>
> In other words, if the directory $D that is the top level of the
> working tree of a non-bare repository, you should be able to chdir
> to "$D/.git" and run your git command without explicitly saying that
> you are inside $GIT_DIR (e.g. "git --git-dir=$(pwd) cmd")?

>
> It makes very good sense.
>
> I briefly wondered if this would give us a great usability
> improvement especially for hook scripts, but they are given GIT_DIR
> when called already so that is not a big upside, I guess.
>
> > Teach git to not reject uses of git inside of the .git directory: check
> > if cwd is .git (or a subdirectory of it) and allow it even if
> > safe.bareRepository=explicit.
>
>
> >     My primary concern with this patch is that I'm unsure if we need to
> >     worry about case-insensitive filesystems (ex: cwd=my_repo/.GIT instead
> >     of my_repo/.git, it might not trigger this logic and end up allowed).
>
> You are additionally allowing ".git" so even if somebody has ".GIT"
> that won't be allowed by this change, no?

>
> >     I'm assuming this isn't a significant concern, for two reasons:
> >
> >      * most filesystems/OSes in use today (by number of users) are at least
> >        case-preserving, so users/tools will have had to type out .GIT
> >        instead of getting it from readdir/wherever.
> >      * this is primarily a "quality of life" change to the feature, and if
> >        we get it wrong we still fail closed.
>
> Yup.
>
> If we really cared (which I doubt), we could resort to checking with
> is_ntfs_dotgit() and is_hfs_dotgit(), but that would work in the
> direction of loosening the check even further, which I do not know
> is needed.

>
> > -                     if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
> > +                     if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
> > +                         !ends_with_path_components(dir->buf, ".git"))
> >                               return GIT_DIR_DISALLOWED_BARE;
>
> Thanks.

> diff --git a/setup.c b/setup.c
> index b38702718fb..b095e284979 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1371,7 +1371,8 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,
>  
>  		if (is_git_directory(dir->buf)) {
>  			trace2_data_string("setup", NULL, "implicit-bare-repository", dir->buf);
> -			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT)
> +			if (get_allowed_bare_repo() == ALLOWED_BARE_REPO_EXPLICIT &&
> +			    !ends_with_path_components(dir->buf, ".git"))
>  				return GIT_DIR_DISALLOWED_BARE;
>  			if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
>  				return GIT_DIR_INVALID_OWNERSHIP;

> 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.

>
> 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

>     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@pobox.com>
> ---
>  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/"))

> +               goto out;
> +
> +       /*
> +        * Does gitdir that points at the ".git" file at the root of
> +        * the secondary worktree roundtrip here?
> +        */

> +       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
>

>> +       /*
>> +        * 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/)`?

>
> Kyle Lippincott <spectral@google.com> writes:
>
> >>     $ cat ../seconary/.git
> >
> > Nit: typo, should be `secondary` (missing the `d`)
>
> Good eyes.  Thanks.
>
> >> +       /*
> >> +        * 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?
>
> I am not certain offhand, but if they need to tolerate different
> separators, they can send in patches.
>
> >> +               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/)`?
>
> No loss of security.

>
> These "keep result the status we want to return if we want to return
> immediately here, and always jump to the out label instead of
> returning" patterns is mere a disciplined way to make it easier to
> write code that does not leak.  The very first one may not have to
> do the "goto out" and instead immediately return, but by writing
> this way, I do not need to be always looking out to shoot down
> patches that adds new check and/or allocations before this
> condition and early "out".
>
> > Or maybe we even combine the existing ends_with(.git) check with this
>
> At the mechanical level, perhaps, but I'd want logically separate
> things treated as distinct cases.  One is about being inside
> $GIT_DIR of the primary worktrees (where more than majority of users
> will encounter) and the new one is about being inside $GIT_DIR of
> secondaries.

>> > 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/)`?
>>
>> No loss of security.
>
> Then should we just do that?

> + /* Assumption: `path` points to the root of a $GIT_DIR. */
>  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;
> +
> +       /*
> +        * Also allow $GIT_DIRs in secondary worktrees.
> +        * These do not end in .git, but are still considered safe because
> +        * of the .git component in the path.
> +        */
> +       if (strstr(path, "/.git/worktrees/"))
> +               return 1;
> +
> +       return 0;
>  }

> 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.

>> 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'.
>
> Does the same reason also apply to .git/modules/$name ?

>
> Builds directly on top of 45bb9162 (setup: allow cwd=.git w/
> bareRepository=explicit, 2024-01-20).
>
> Instead of saying "primary worktree's $GIT_DIR is OK", "secondary
> worktree's $GIT_DIR is OK", and "submodule's $GIT_DIR is OK"
> separately, let's give them a name to call them collectively,
> "implicit bare repository" (for now, to reuse what an earlier commit
> used, which may not be an optimum name), as these share the same
> security guarantee and convenience benefit.
>
> The code got significantly simpler, and test moderately more
> complex, having to set up submodule tests.
>
> ------- >8 ------------- >8 ------------- >8 -------
> Setting the safe.bareRepository configuration variable to explicit
> stops git from using a bare repository, unless the repository is
> explicitly specified, either by the "--git-dir=<path>" command line
> option, or by exporting $GIT_DIR environment variable.  This may be
> a reasonable measure to safeguard users from accidentally straying
> into a bare repository in unexpected places, but often gets in the
> way of users who need valid accesses too the repository.

>
> Earlier, 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,
> 2024-01-20) loosened the rule such that being inside the ".git"
> directory of a non-bare repository does not really count as
> accessing a "bare" repository.  The reason why such a loosening is
> needed is because often hooks and third-party tools run from within
> $GIT_DIR while working with a non-bare repository.
>
> More importantly, the reason why this is safe is because a directory
> whose contents look like that of a "bare" repository cannot be a
> bare repository that came embedded within a checkout of a malicious
> project, as long as its directory name is ".git", because ".git" is
> not a name allowed for a directory in payload.
>
> There are at least two other cases where tools have to work in a
> bare-repository looking directory that is not an embedded bare
> repository, and accesses to them are still not allowed by the recent
> change.
>
>  - A secondary worktree (whose name is $name) has its $GIT_DIR
>    inside "worktrees/$name/" subdirectory of the $GIT_DIR of the
>    primary worktree of the same repository.
>
>  - A submodule worktree (whose name is $hame) has its $GIT_DIR

>    inside "modules/$name/" subdirectory of the $GIT_DIR of its
>    superproject.
>
> As long as the primary worktree or the superproject in these cases
> are not bare, the pathname of these "looks like bare but not really"
> directories will have "/.git/worktrees/" and "/.git/modules/" as a
> substring in its leading part, and we can take advantage of the same
> security guarantee allow git to work from these places.
>
> Extend the earlier "in a directory called '.git' we are OK" logic
> used for the primary worktree to also cover the secondary worktree's
> and non-embedded submodule's $GIT_DIR, by moving the logic to a
> helper function "is_implicit_bare_repo()".  We deliberately exclude
> secondary worktrees and submodules of a bare repository, as these
> are exactly what safe.bareRepository=explicit setting is designed to
> forbid accesses to without an explicit GIT_DIR/--git-dir=<path>
>
> Helped-by: Kyle Lippincott <spectral@google.com>
> Helped-by: Kyle Meyer <kyle@kyleam.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  setup.c                         | 28 +++++++++++++++++++++++++++-
>  t/t0035-safe-bare-repository.sh | 26 ++++++++++++++++++++++----
>  2 files changed, 49 insertions(+), 5 deletions(-)

>> into a bare repository in unexpected places, but often gets in the
>> way of users who need valid accesses too the repository.
>
> nit: 'to', not 'too'
> ...
>>  - A submodule worktree (whose name is $hame) has its $GIT_DIR
>
> nit: '$name', not '$hame'
> ...
>> Helped-by: Kyle Lippincott <spectral@google.com>
>> Helped-by: Kyle Meyer <kyle@kyleam.com>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  setup.c                         | 28 +++++++++++++++++++++++++++-
>>  t/t0035-safe-bare-repository.sh | 26 ++++++++++++++++++++++----
>>  2 files changed, 49 insertions(+), 5 deletions(-)
>
> Looks good, thanks!