* [PATCH 1/2] setup: detect to be in $GIT_DIR with a new helper
2024-03-08 21:19 ` [PATCH 0/2] Loosening safe.bareRepository=explicit even further Junio C Hamano
@ 2024-03-08 21:19 ` Junio C Hamano
2024-03-08 21:19 ` [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree Junio C Hamano
2024-03-09 23:27 ` [PATCH v2] setup: notice more types of implicit bare repositories Junio C Hamano
2 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2024-03-08 21:19 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-08 21:19 ` [PATCH 0/2] Loosening safe.bareRepository=explicit even further Junio C Hamano
2024-03-08 21:19 ` [PATCH 1/2] setup: detect to be in $GIT_DIR with a new helper Junio C Hamano
@ 2024-03-08 21:19 ` Junio C Hamano
2024-03-08 22:30 ` Junio C Hamano
` (2 more replies)
2024-03-09 23:27 ` [PATCH v2] setup: notice more types of implicit bare repositories Junio C Hamano
2 siblings, 3 replies; 17+ messages in thread
From: Junio C Hamano @ 2024-03-08 21:19 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-08 21:19 ` [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree Junio C Hamano
@ 2024-03-08 22:30 ` Junio C Hamano
2024-03-08 23:10 ` Kyle Lippincott
2024-03-09 3:20 ` Kyle Meyer
2 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2024-03-08 22:30 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-08 21:19 ` [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree Junio C Hamano
2024-03-08 22:30 ` Junio C Hamano
@ 2024-03-08 23:10 ` Kyle Lippincott
2024-03-08 23:32 ` Junio C Hamano
2024-03-09 3:20 ` Kyle Meyer
2 siblings, 1 reply; 17+ messages in thread
From: Kyle Lippincott @ 2024-03-08 23:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-08 23:10 ` Kyle Lippincott
@ 2024-03-08 23:32 ` Junio C Hamano
2024-03-09 0:12 ` Kyle Lippincott
0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2024-03-08 23:32 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-08 23:32 ` Junio C Hamano
@ 2024-03-09 0:12 ` Kyle Lippincott
2024-03-09 1:14 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Kyle Lippincott @ 2024-03-09 0:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
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.
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-09 0:12 ` Kyle Lippincott
@ 2024-03-09 1:14 ` Junio C Hamano
0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2024-03-09 1:14 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-08 21:19 ` [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree Junio C Hamano
2024-03-08 22:30 ` Junio C Hamano
2024-03-08 23:10 ` Kyle Lippincott
@ 2024-03-09 3:20 ` Kyle Meyer
2024-03-09 5:53 ` Junio C Hamano
2 siblings, 1 reply; 17+ messages in thread
From: Kyle Meyer @ 2024-03-09 3:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kyle Lippincott
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree
2024-03-09 3:20 ` Kyle Meyer
@ 2024-03-09 5:53 ` Junio C Hamano
0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2024-03-09 5:53 UTC (permalink / raw)
To: Kyle Meyer; +Cc: git, Kyle Lippincott
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.
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2] setup: notice more types of implicit bare repositories
2024-03-08 21:19 ` [PATCH 0/2] Loosening safe.bareRepository=explicit even further Junio C Hamano
2024-03-08 21:19 ` [PATCH 1/2] setup: detect to be in $GIT_DIR with a new helper Junio C Hamano
2024-03-08 21:19 ` [PATCH 2/2] setup: make bareRepository=explicit work in GIT_DIR of a secondary worktree Junio C Hamano
@ 2024-03-09 23:27 ` Junio C Hamano
2024-03-11 19:23 ` Kyle Lippincott
2 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2024-03-09 23:27 UTC (permalink / raw)
To: git; +Cc: Kyle Lippincott, Kyle Meyer
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2] setup: notice more types of implicit bare repositories
2024-03-09 23:27 ` [PATCH v2] setup: notice more types of implicit bare repositories Junio C Hamano
@ 2024-03-11 19:23 ` Kyle Lippincott
2024-03-11 21:02 ` Junio C Hamano
0 siblings, 1 reply; 17+ messages in thread
From: Kyle Lippincott @ 2024-03-11 19:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kyle Meyer
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!
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] setup: notice more types of implicit bare repositories
2024-03-11 19:23 ` Kyle Lippincott
@ 2024-03-11 21:02 ` Junio C Hamano
0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2024-03-11 21:02 UTC (permalink / raw)
To: Kyle Lippincott; +Cc: git, Kyle Meyer
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.
^ permalink raw reply [flat|nested] 17+ messages in thread