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

From: Kyle Lippincott <[email protected]>

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.

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

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.

Quoted 9 lines · Kyle Lippincott via GitGitGadget

From: Kyle Lippincott <[email protected]>

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.

Quoted 3 lines

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.

Quoted 3 lines

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?

Quoted 8 lines

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.

setup.c static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, · line 1376

Quoted 5 lines

setup.c +2 -1

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

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.

setup.c +0 -0

index b38702718fb..b095e284979 100644

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 <[email protected]>

---
 setup.c                         | 52 ++++++++++++++++++++++++++++++++-
 t/t0035-safe-bare-repository.sh |  8 ++++-
 2 files changed, 58 insertions(+), 2 deletions(-)

setup.c +6 -0

index 3081be4970..68860dcd18 100644
@@ -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;

setup.c +29 -1

+       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;
 }
 
 /*

t/t0035-safe-bare-repository.sh +7 -2

index 8048856379..62cdfcefc1 100755
@@ -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

Quoted 1 line

    $ cat ../seconary/.git

Nit: typo, should be secondary (missing the d)

setup.c static const char *allowed_bare_repo_to_string( · line 1249

Quoted 8 lines

setup.c +8 -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?

setup.c static const char *allowed_bare_repo_to_string( · line 1255

Quoted 6 lines

setup.c +6 -0

+               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

Quoted 3 lines · Kyle Lippincott

Quoted 1 line

    $ cat ../seconary/.git

Nit: typo, should be secondary (missing the d)

Good eyes. Thanks.

Quoted 11 lines

setup.c static const char *allowed_bare_repo_to_string( · line 1249

Quoted 8 lines

setup.c +8 -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?

I am not certain offhand, but if they need to tolerate different separators, they can send in patches.

Quoted 7 lines

Quoted 6 lines

setup.c +6 -0

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

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 4 lines

Quoted 2 lines

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

setup.c +4 -1

-       return ends_with_path_components(path, ".git");
+       /* $GIT_DIR immediately below the primary working tree */
+       if (ends_with_path_components(path, ".git"))
+               return 1;
+

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.

Quoted 4 lines

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 <[email protected]>

Helped-by Kyle Meyer <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 setup.c                         | 28 +++++++++++++++++++++++++++-
 t/t0035-safe-bare-repository.sh | 26 ++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 5 deletions(-)

Quoted 2 lines

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

Quoted 1 line

- A submodule worktree (whose name is $hame) has its $GIT_DIR

nit: '$name', not '$hame'
...

Quoted 7 lines

Helped-by Kyle Lippincott <[email protected]>

Helped-by Kyle Meyer <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 setup.c                         | 28 +++++++++++++++++++++++++++-
 t/t0035-safe-bare-repository.sh | 26 ++++++++++++++++++++++----
 2 files changed, 49 insertions(+), 5 deletions(-)

Looks good, thanks!