| Message ID | 20240308211957.3758770-3-gitster@pobox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Loosening safe.bareRepository=explicit even further | expand |
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 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 >
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 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.
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.
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
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.
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
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(-)