setup: allow cwd=.git w/ bareRepository=explicit #1645
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. [1] https://github.com/justinsteven/advisories/blob/main/2022_git_buried_bare_repos_and_fsmonitor_various_abuses.md Signed-off-by: Kyle Lippincott <spectral@google.com>
|
/submit |
|
Submitted as pull.1645.git.1705709303098.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "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. |
|
This branch is now known as |
|
This patch series was integrated into seen via git@d4fc633. |
|
On the Git mailing list, Kyle Lippincott wrote (reply to this): On Sat, Jan 20, 2024 at 2:26 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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")?
Correct.
>
> 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?
My concern was what happens if someone on a case-insensitive
filesystem does `cd .GIT` and then attempts to use it. If the cwd path
isn't case-normalized at some point, we'll have a string like
/path/to/my-repo/.GIT from getcwd() and it won't be allowed by this
code, since this code is checking for `.git` in a case sensitive
fashion.
>
> > 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.
Agreed, it's not worth the additional complexity.
>
> > - 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. |
|
This patch series was integrated into seen via git@c47afa4. |
|
This patch series was integrated into seen via git@bbcca1c. |
|
This patch series was integrated into seen via git@e86e2b8. |
|
There was a status update in the "New Topics" section about the branch Loosen "disable repository discovery of a bare repository" check, triggered by setting safe.bareRepository configuration variable to 'explicit', to exclude the ".git/" directory inside a non-bare repository from the check. Will merge to 'next'. source: <pull.1645.git.1705709303098.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@7c30055. |
|
This patch series was integrated into seen via git@f84738a. |
|
This patch series was integrated into next via git@e77b796. |
|
This patch series was integrated into seen via git@5d6f52b. |
|
There was a status update in the "Cooking" section about the branch Loosen "disable repository discovery of a bare repository" check, triggered by setting safe.bareRepository configuration variable to 'explicit', to exclude the ".git/" directory inside a non-bare repository from the check. Will merge to 'master'. source: <pull.1645.git.1705709303098.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@1b84011. |
|
This patch series was integrated into seen via git@aff65bf. |
|
There was a status update in the "Cooking" section about the branch Loosen "disable repository discovery of a bare repository" check, triggered by setting safe.bareRepository configuration variable to 'explicit', to exclude the ".git/" directory inside a non-bare repository from the check. Will merge to 'master'. source: <pull.1645.git.1705709303098.gitgitgadget@gmail.com> |
|
This patch series was integrated into seen via git@a8bf3c0. |
|
This patch series was integrated into master via git@a8bf3c0. |
|
This patch series was integrated into next via git@a8bf3c0. |
|
Closed via a8bf3c0. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Kyle Lippincott via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
> 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;
I wish we had caught it much before we added DISALLOWED_BARE thing,
but I wonder how well would this escape-hatch interact with
secondary worktrees, where their git directory is not named ".git"
and not immediately below the root level of the working tree?
In a secondary worktree the root level of its working tree has a
file ".git", whose contents may look like
gitdir: /home/gitster/git.git/.git/worktrees/git.old
where
- /home/gitster/git.git/ is the primary worktree with the
repository.
- /home/gitster/git.git/.git/worktrees/git.old looks like a bare
repository.
- /home/gitster/git.git/.git/worktrees/git.old/gitdir gives a way
to discover the secondary worktree, whose contents just records
the path to the ".git" file, e.g., "/home/gitster/git.old/.git"
that had "gitdir: ..." in it.
So perhaps we can also use the presence of "gitdir" file, check the
contents of it tn ensure that ".git" file there takes us back to
this (not quite) bare repository we are looking at, and allow access
to it, or something?
Thoughts?
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Earlier 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,
2024-01-20) loosened safe.bareRepository=explicit in such a way that
working inside the ".git/" directory (or its subdirectories) of a
repository that is not bare can be done without an explicit GIT_DIR
or "git --git-dir=<path>". The code needed for its change was
almost trivial---when it looks like we encountered a bare
repository, if the last path component of the discovered "$GIT_DIR"
is ".git", then it cannot be anything but the $GIT_DIR of a non-bare
repository, the root of whose working tree is the parent directory
of that ".git" directory. This is because projects cannot create a
".git" directory in their working tree and cause clone/checkout to
extract them in the victim's working tree.
This almost works, until somebody starts using "git worktree add" to
create a secondary worktree. Their $GIT_DIR resides inside the
$GIT_DIR of the primary worktree of the same repository, at
$GIT_DIR/worktree/$name where $name is the name of the secondary
worktree, which is not ".git".
These two patches are to extend the "if you can work in its working
tree, you should be able to work in its $GIT_DIR" for secondary
worktrees.
Junio C Hamano (2):
setup: detect to be in $GIT_DIR with a new helper
setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
setup.c | 57 ++++++++++++++++++++++++++++++++-
t/t0035-safe-bare-repository.sh | 8 ++++-
2 files changed, 63 insertions(+), 2 deletions(-)
--
2.44.0-165-ge09f1254c5
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Earlier, 45bb9162 (setup: allow cwd=.git w/ bareRepository=explicit,
2024-01-20) loosened the "safe.bareRepository=explicit" to allow Git
operations inside ".git/" directory in the root level of a working
tree of a non-bare repository. It used the fact that the $GIT_DIR
you discover has ".git" as the last path component, if you descended
into ".git" of a non-bare repository.
Let's move the logic into a separate helper function. We can
enhance this to detect the case where we are inside $GIT_DIR of a
secondary worktree (where "ends with .git" trick does not work) in
the next commit.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
setup.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/setup.c b/setup.c
index a09b7b87ec..3081be4970 100644
--- a/setup.c
+++ b/setup.c
@@ -1231,6 +1231,11 @@ static const char *allowed_bare_repo_to_string(
return NULL;
}
+static int is_repo_with_working_tree(const char *path)
+{
+ return ends_with_path_components(path, ".git");
+}
+
/*
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
@@ -1360,7 +1365,7 @@ 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 &&
- !ends_with_path_components(dir->buf, ".git"))
+ !is_repo_with_working_tree(dir->buf))
return GIT_DIR_DISALLOWED_BARE;
if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
return GIT_DIR_INVALID_OWNERSHIP;
--
2.44.0-165-ge09f1254c5
|
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Junio C Hamano <gitster@pobox.com> writes:
> 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.
I wrote the "[*]" mark but forgot to add a footnote with an
additional information for it. Something like this was what I had
in mind to write there:
[Footnote]
* This does not help folks who create a new worktree out of a bare
repository, because in their set-up, there won't be "/.git/" in
front of "worktrees" directory. It is fundamentally impossible
to lift this limitation, as long as safe.bareRepository is
considered to be a meaningful security measure. The security of
both the loosening for a secondary worktree's GIT_DIR as well as
the loosening for the GIT_DIR of the primary worktree, hinge on
the fact that ".git/" directory is impossible to create as
payload to be cloned. |
|
On the Git mailing list, Kyle Lippincott wrote (reply to this): On Fri, Mar 8, 2024 at 1:20 PM Junio C Hamano <gitster@pobox.com> 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@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/"))
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
> |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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. |
|
On the Git mailing list, Kyle Lippincott wrote (reply to this): On Fri, Mar 8, 2024 at 3:32 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
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;
}
>
> 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. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Kyle Lippincott <spectral@google.com> writes:
>> > 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?
I do not see what you mean.
> + /* 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;
> }
Ah, no. I thought you were asking "goto out" vs "return", and my
answer was about these two. Whether you leave with "goto out" or
"return", it does not change the security posture. Direct return
will raise the risk of leaking resources after careless future
updates to the code.
I didn't get that you do not want to see the other two "sanity
check".
Losing these sanity checks may not lose "security" per-se, but it
may raise the risk of misidentification. A healthy GIT_DIR of a
secondary worktree should satisfy these two extra conditions. |
|
On the Git mailing list, Kyle Meyer wrote (reply to this): Junio C Hamano writes:
> 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 ?
> 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.
In the case of .git/modules/, the second point doesn't apply because
there's no gitdir file. But perhaps the core.worktree setting could be
used for the same purpose.
$ pwd
/path/to/super/.git/modules/sub
$ git config core.worktree
../../../sub |
|
User |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Kyle Meyer <kyle@kyleam.com> writes:
>> 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 ?
Perhaps. I do not actively work on submodules so unlike those who
are always thinking about improving the user experience around them,
I did not think of those ".git/modules/$name" things as something
similar to the ".git/worktrees/$name" things.
Often hooks (and probably third-party tools) run after chdir to be
in $GIT_DIR, so the problems they face when their /etc/gitconfig
forces them to use safe.bareRepository=explicit are probably very
similar either way. |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): 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(-)
diff --git a/setup.c b/setup.c
index a09b7b87ec..25d98ee6dd 100644
--- a/setup.c
+++ b/setup.c
@@ -1231,6 +1231,32 @@ static const char *allowed_bare_repo_to_string(
return NULL;
}
+static int is_implicit_bare_repo(const char *path)
+{
+ /*
+ * what we found is a ".git" directory at the root of
+ * the working tree.
+ */
+ if (ends_with_path_components(path, ".git"))
+ return 1;
+
+ /*
+ * we are inside $GIT_DIR of a secondary worktree of a
+ * non-bare repository.
+ */
+ if (strstr(path, "/.git/worktrees/"))
+ return 1;
+
+ /*
+ * we are inside $GIT_DIR of a worktree of a non-embedded
+ * submodule, whose superproject is not a bare repository.
+ */
+ if (strstr(path, "/.git/modules/"))
+ return 1;
+
+ return 0;
+}
+
/*
* We cannot decide in this function whether we are in the work tree or
* not, since the config can only be read _after_ this function was called.
@@ -1360,7 +1386,7 @@ 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 &&
- !ends_with_path_components(dir->buf, ".git"))
+ !is_implicit_bare_repo(dir->buf))
return GIT_DIR_DISALLOWED_BARE;
if (!ensure_valid_ownership(NULL, NULL, dir->buf, report))
return GIT_DIR_INVALID_OWNERSHIP;
diff --git a/t/t0035-safe-bare-repository.sh b/t/t0035-safe-bare-repository.sh
index 8048856379..d3cb2a1cb9 100755
--- a/t/t0035-safe-bare-repository.sh
+++ b/t/t0035-safe-bare-repository.sh
@@ -29,9 +29,20 @@ expect_rejected () {
grep -F "implicit-bare-repository:$pwd" "$pwd/trace.perf"
}
-test_expect_success 'setup bare repo in worktree' '
+test_expect_success 'setup an embedded bare repo, secondary worktree and submodule' '
git init outer-repo &&
- git init --bare outer-repo/bare-repo
+ git init --bare --initial-branch=main outer-repo/bare-repo &&
+ git -C outer-repo worktree add ../outer-secondary &&
+ test_path_is_dir outer-secondary &&
+ (
+ cd outer-repo &&
+ test_commit A &&
+ git push bare-repo +HEAD:refs/heads/main &&
+ git -c protocol.file.allow=always \
+ submodule add --name subn -- ./bare-repo subd
+ ) &&
+ test_path_is_dir outer-repo/.git/worktrees/outer-secondary &&
+ test_path_is_dir outer-repo/.git/modules/subn
'
test_expect_success 'safe.bareRepository unset' '
@@ -53,8 +64,7 @@ test_expect_success 'safe.bareRepository in the repository' '
# safe.bareRepository must not be "explicit", otherwise
# git config fails with "fatal: not in a git directory" (like
# safe.directory)
- test_config -C outer-repo/bare-repo safe.bareRepository \
- all &&
+ test_config -C outer-repo/bare-repo safe.bareRepository all &&
test_config_global safe.bareRepository explicit &&
expect_rejected -C outer-repo/bare-repo
'
@@ -86,4 +96,12 @@ 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_expect_success 'no trace in $GIT_DIR of a submodule' '
+ expect_accepted_implicit -C outer-repo/.git/modules/subn
+'
+
test_done
--
2.44.0-165-ge09f1254c5
|
|
On the Git mailing list, Kyle Lippincott wrote (reply to this): On Sat, Mar 9, 2024 at 3:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.
nit: 'to', not 'too'
>
> 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
nit: '$name', not '$hame'
> 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(-)
Looks good, thanks! |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Kyle Lippincott <spectral@google.com> writes:
>> 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!
Thanks. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please be aware that I'm a new contributor (this is my first patch to git's code),
so any style nits, suggestions about how to make this more idiomatic, or any
other suggestions are strongly encouraged.
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:
case-preserving, so users/tools will have had to type out
.GITinstead ofgetting it from readdir/wherever.
we still fail closed.
cc: Kyle Meyer kyle@kyleam.com