| Message ID | xmqq5xxv0ywi.fsf_-_@gitster.g (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 30b7c4bdcac27ac08a6caf2c70aff55763b9b3ab |
| Headers | show |
| Series | [v2] setup: notice more types of implicit bare repositories | expand |
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!
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.
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
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(-)