* [PATCH 0/1] Git.pm: add semicolon after catch statement @ 2022-10-16 21:22 Michael McClimon 2022-10-16 21:22 ` [PATCH 1/1] " Michael McClimon 2022-10-22 1:19 ` [PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories Michael McClimon 0 siblings, 2 replies; 23+ messages in thread From: Michael McClimon @ 2022-10-16 21:22 UTC (permalink / raw) To: git; +Cc: Michael McClimon When looking into a bug about Git.pm's handling of unsafe repositories [1], I found the immediate cause of the error, which this patch fixes. This patch doesn't do anything about the actual problem (Git.pm happily continues to work in unsafe repositories), but it at least fixes the runtime error. [1] https://lore.kernel.org/git/20221011182607.f1113fff-9333-427d-ba45-741a78fa6040@korelogic.com/ Michael McClimon (1): Git.pm: add semicolon after catch statement perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.38.0.83.gd420dda0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH 1/1] Git.pm: add semicolon after catch statement 2022-10-16 21:22 [PATCH 0/1] Git.pm: add semicolon after catch statement Michael McClimon @ 2022-10-16 21:22 ` Michael McClimon 2022-10-16 23:18 ` Jeff King 2022-10-22 1:19 ` [PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories Michael McClimon 1 sibling, 1 reply; 23+ messages in thread From: Michael McClimon @ 2022-10-16 21:22 UTC (permalink / raw) To: git; +Cc: Michael McClimon When attempting to initialize a repository object in an unsafe directory, a syntax error is reported (Can't use string as a HASH ref while strict refs in use). Fix this runtime error by adding the required semicolon after the catch statement. Without the semicolon, the result of the following line (i.e., the result of Cwd::abs_path) is passed as the third argument to Error.pm's catch function. That function expects that its third argument, $clauses, is a hash reference, and trying to access a string as a hash reference is a fatal error. [1] https://lore.kernel.org/git/20221011182607.f1113fff-9333-427d-ba45-741a78fa6040@korelogic.com/ Reported-by: Hank Leininger <hlein@korelogic.com> Signed-off-by: Michael McClimon <michael@mcclimon.org> --- perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/perl/Git.pm b/perl/Git.pm index 080cdc2a..cf15ead6 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -217,7 +217,7 @@ sub repository { } catch Git::Error::Command with { # Mimic git-rev-parse --git-dir error message: throw Error::Simple("fatal: Not a git repository: $dir"); - } + }; $opts{Repository} = Cwd::abs_path($dir); } -- 2.38.0.83.gd420dda0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] Git.pm: add semicolon after catch statement 2022-10-16 21:22 ` [PATCH 1/1] " Michael McClimon @ 2022-10-16 23:18 ` Jeff King 2022-10-17 2:17 ` Michael McClimon 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2022-10-16 23:18 UTC (permalink / raw) To: Michael McClimon; +Cc: git On Sun, Oct 16, 2022 at 05:22:36PM -0400, Michael McClimon wrote: > When attempting to initialize a repository object in an unsafe > directory, a syntax error is reported (Can't use string as a HASH ref > while strict refs in use). Fix this runtime error by adding the required > semicolon after the catch statement. > > Without the semicolon, the result of the following line (i.e., the > result of Cwd::abs_path) is passed as the third argument to Error.pm's > catch function. That function expects that its third argument, > $clauses, is a hash reference, and trying to access a string as a hash > reference is a fatal error. Curiously this works as expected for me, both before and after your patch. I wonder if it depends on perl version. Mine is 5.34. I've never used Error.pm's try/catch before, so I don't know what's normal. Regular if/unless doesn't need it, but certainly an earlier catch uses a semicolon. So it seems like a reasonable fix. > diff --git a/perl/Git.pm b/perl/Git.pm > index 080cdc2a..cf15ead6 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -217,7 +217,7 @@ sub repository { > } catch Git::Error::Command with { > # Mimic git-rev-parse --git-dir error message: > throw Error::Simple("fatal: Not a git repository: $dir"); > - } > + }; I'd assume t9700 passes for you, since I don't think we cover this case. Maybe it's worth squashing this in: diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db76..5bd3687f37 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -30,6 +30,12 @@ sub adjust_dirsep { # set up our $abs_repo_dir = cwd(); ok(our $r = Git->repository(Directory => "."), "open repository"); +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + my $failed = eval { Git->repository(Directory => ".") }; + ok(!$failed, "reject unsafe repository"); + like($@, qr/not a git repository/i, "unsafe error message"); +} # config is($r->config("test.string"), "value", "config scalar: string"); ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] Git.pm: add semicolon after catch statement 2022-10-16 23:18 ` Jeff King @ 2022-10-17 2:17 ` Michael McClimon 2022-10-17 17:34 ` Jeff King 0 siblings, 1 reply; 23+ messages in thread From: Michael McClimon @ 2022-10-17 2:17 UTC (permalink / raw) To: Jeff King; +Cc: git > Curiously this works as expected for me, both before and after your > patch. I wonder if it depends on perl version. Mine is 5.34. Hm, curious indeed! It reliably fails without my patch and passes with it on all the versions I had lying around (5.8, 5.18, 5.24, 5.26, 5.28, 5.30, 5.34, and 5.36). > I've never used Error.pm's try/catch before, so I don't know what's > normal. Regular if/unless doesn't need it, but certainly an earlier > catch uses a semicolon. So it seems like a reasonable fix. Ha, well Perl is...let's say special. try/catch is not a language construct (until 5.34, where it is experimental), and so it's always implemented by subroutines. One upshot of this is that try/catch needs a semicolon, because it's sugar for try(sub { ... }), and statements need semicolons separating them. Compare these two examples: -MO=Deparse,-p tells perl to deparse the program and print it with parentheses: $ perl -MError -MO=Deparse,-p -e 'try { die "bad" } catch Pkg with { warn "caught" }; print "after"' do { die('bad') }->try('Pkg'->catch(do { warn('caught') }->with)); print('after'); -e syntax OK $ perl -MError -MO=Deparse,-p -e 'try { die "bad" } catch Pkg with { warn "caught" } print "after"' do { die('bad') }->try('Pkg'->catch(do { warn('caught') }->with(print('after')))); -e syntax OK The first here is the good case case (with semicolon), and you can see that print('after') is its own statement. The bad case, with no semicolon, passes it as an argument to with(), which is what's causing the error here: something called by with() is trying to use it as a hash reference, and it's a string. > I'd assume t9700 passes for you, since I don't think we cover this case. > Maybe it's worth squashing this in: > > diff --git a/t/t9700/test.pl b/t/t9700/test.pl > index e046f7db76..5bd3687f37 100755 > --- a/t/t9700/test.pl > +++ b/t/t9700/test.pl > @@ -30,6 +30,12 @@ sub adjust_dirsep { > # set up > our $abs_repo_dir = cwd(); > ok(our $r = Git->repository(Directory => "."), "open repository"); > +{ > + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; > + my $failed = eval { Git->repository(Directory => ".") }; > + ok(!$failed, "reject unsafe repository"); > + like($@, qr/not a git repository/i, "unsafe error message"); > +} > > # config > is($r->config("test.string"), "value", "config scalar: string"); Curiously, t9700 passes for me with this suggestion both with and without my patch. You'd only see this bug in bare repositories, though, and the one set up in t9700 is not bare. I can see about trying to make it do so, but I'll need to do a bit more reading of how even the tests are set up and run first. Thanks! -- Michael McClimon michael@mcclimon.org ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] Git.pm: add semicolon after catch statement 2022-10-17 2:17 ` Michael McClimon @ 2022-10-17 17:34 ` Jeff King 2022-10-18 1:39 ` Michael McClimon 2022-11-10 15:10 ` Johannes Schindelin 0 siblings, 2 replies; 23+ messages in thread From: Jeff King @ 2022-10-17 17:34 UTC (permalink / raw) To: Michael McClimon Cc: Carlo Marcelo Arenas Belón, Glen Choo, Johannes Schindelin, git [+cc people who worked on safe-directory stuff; please check out the included test and final comments] On Sun, Oct 16, 2022 at 10:17:53PM -0400, Michael McClimon wrote: > > Curiously this works as expected for me, both before and after your > > patch. I wonder if it depends on perl version. Mine is 5.34. > > Hm, curious indeed! It reliably fails without my patch and passes with it on > all the versions I had lying around (5.8, 5.18, 5.24, 5.26, 5.28, 5.30, 5.34, > and 5.36). Doh, sorry to mislead you; I hadn't noticed this was in the bare repository code path until you pointed it out. I get the same outcome as you and the OP once that is fixed (both in t9700 and in my manual testing). > Ha, well Perl is...let's say special. try/catch is not a language construct > (until 5.34, where it is experimental), and so it's always implemented by > subroutines. One upshot of this is that try/catch needs a semicolon, because > it's sugar for try(sub { ... }), and statements need semicolons separating > them. Right, I imagined it was something like that. Your fix is definitely the right thing, then. > Curiously, t9700 passes for me with this suggestion both with and without my > patch. You'd only see this bug in bare repositories, though, and the one set > up in t9700 is not bare. I can see about trying to make it do so, but I'll > need to do a bit more reading of how even the tests are set up and run first. Yeah, this test is particularly confusing because unlike most of our suite, it drives the test harness using a separate perl script. So you have setup in one file and tests in another. You'd want something like: diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh index 4aa5d90d32..53a838a8e8 100755 --- a/t/t9700-perl-git.sh +++ b/t/t9700-perl-git.sh @@ -45,6 +45,10 @@ test_expect_success \ git config --add test.pathmulti bar ' +test_expect_success 'set up bare repository' ' + git init --bare bare.git +' + test_expect_success 'use t9700/test.pl to test Git.pm' ' "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr && test_must_be_empty stderr diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db76..917b09cdf9 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -30,6 +30,12 @@ sub adjust_dirsep { # set up our $abs_repo_dir = cwd(); ok(our $r = Git->repository(Directory => "."), "open repository"); +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + my $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; + ok(!$failed, "reject unsafe bare repository"); + like($@, qr/not a git repository/i, "unsafe error message"); +} # config is($r->config("test.string"), "value", "config scalar: string"); But curiously this still does not pass after your patch, because we seem to actually open the repository! I think this is because the C code allows an explicit GIT_DIR to override the safe-directory checks. But in this case that GIT_DIR is set by Git.pm searching for the directory, not because the user desires it. (I know that I used a "Directory" in the example above, but that is only to avoid extra chdir-ing around in the test script; calling a bare Git->repository() triggers the same behavior in the right directory). So your patch is definitely still the right thing to do, but it feels like a hole in the safe-directory mechanism, at least when called via Git.pm. +cc folks who worked on that. -Peff ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] Git.pm: add semicolon after catch statement 2022-10-17 17:34 ` Jeff King @ 2022-10-18 1:39 ` Michael McClimon 2022-11-10 15:10 ` Johannes Schindelin 1 sibling, 0 replies; 23+ messages in thread From: Michael McClimon @ 2022-10-18 1:39 UTC (permalink / raw) To: Jeff King Cc: Carlo Marcelo Arenas Belón, Glen Choo, Johannes Schindelin, git > Yeah, this test is particularly confusing because unlike most of our > suite, it drives the test harness using a separate perl script. So you > have setup in one file and tests in another. Oh good, I'm glad it wasn't just me; this was very helpful, thanks. > > But curiously this still does not pass after your patch, because we seem > to actually open the repository! I think this is because the C code > allows an explicit GIT_DIR to override the safe-directory checks. But in > this case that GIT_DIR is set by Git.pm searching for the directory, not > because the user desires it. Aha; this gets to what I was saying in the cover letter, which is that my patch only fixes the runtime error from perl, and Git.pm happily carries on in an unsafe directory. Fixing _that_ is probably beyond my knowledge! > So your patch is definitely still the right thing to do, but it feels > like a hole in the safe-directory mechanism, at least when called via > Git.pm. +cc folks who worked on that. If we wanted a test for just the runtime error, something like the following (including your earlier suggestion to set up the bare repo) demonstrates the bug and my fix. It doesn't seem like a particularly valuable test on its own, IMO, but I'm happy to reroll if we want it. Thanks! diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db..72681849 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -30,6 +30,11 @@ sub adjust_dirsep { # set up our $abs_repo_dir = cwd(); ok(our $r = Git->repository(Directory => "."), "open repository"); +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; + unlike($@, qr/as a HASH ref/i, "no error from perl"); +} # config is($r->config("test.string"), "value", "config scalar: string"); -- Michael McClimon michael@mcclimon.org ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] Git.pm: add semicolon after catch statement 2022-10-17 17:34 ` Jeff King 2022-10-18 1:39 ` Michael McClimon @ 2022-11-10 15:10 ` Johannes Schindelin 2022-11-10 21:41 ` Jeff King 1 sibling, 1 reply; 23+ messages in thread From: Johannes Schindelin @ 2022-11-10 15:10 UTC (permalink / raw) To: Jeff King Cc: Michael McClimon, Carlo Marcelo Arenas Belón, Glen Choo, git Hi Peff, On Mon, 17 Oct 2022, Jeff King wrote: > [... talking about safe.directory ...] > > But curiously this still does not pass after your patch, because we seem > to actually open the repository! I think this is because the C code > allows an explicit GIT_DIR to override the safe-directory checks. Yes, I remember that this was something we discussed at some length during the embargo: what to do with the explicitly-specified `GIT_DIR` when verifying the ownership, and my recollection is that we asserted that setting `GIT_DIR` qualifies for "they know what they're doing" (in particular when it is done in a script, not interactively). Ciao, Dscho ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH 1/1] Git.pm: add semicolon after catch statement 2022-11-10 15:10 ` Johannes Schindelin @ 2022-11-10 21:41 ` Jeff King 0 siblings, 0 replies; 23+ messages in thread From: Jeff King @ 2022-11-10 21:41 UTC (permalink / raw) To: Johannes Schindelin Cc: Michael McClimon, Carlo Marcelo Arenas Belón, Glen Choo, git On Thu, Nov 10, 2022 at 04:10:22PM +0100, Johannes Schindelin wrote: > Hi Peff, > > On Mon, 17 Oct 2022, Jeff King wrote: > > > [... talking about safe.directory ...] > > > > But curiously this still does not pass after your patch, because we seem > > to actually open the repository! I think this is because the C code > > allows an explicit GIT_DIR to override the safe-directory checks. > > Yes, I remember that this was something we discussed at some length during > the embargo: what to do with the explicitly-specified `GIT_DIR` when > verifying the ownership, and my recollection is that we asserted that > setting `GIT_DIR` qualifies for "they know what they're doing" (in > particular when it is done in a script, not interactively). Thanks for confirming. I'm not sure if you read the rest of the thread, but the bug turned out to be in Git.pm, which sets GIT_DIR without knowing what it's doing. :) We ended up with 20da61f25f (Git.pm: trust rev-parse to find bare repositories, 2022-10-22) as the fix. -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories 2022-10-16 21:22 [PATCH 0/1] Git.pm: add semicolon after catch statement Michael McClimon 2022-10-16 21:22 ` [PATCH 1/1] " Michael McClimon @ 2022-10-22 1:19 ` Michael McClimon 2022-10-22 1:19 ` [PATCH v2 1/2] Git.pm: add semicolon after catch statement Michael McClimon 2022-10-22 1:19 ` [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking Michael McClimon 1 sibling, 2 replies; 23+ messages in thread From: Michael McClimon @ 2022-10-22 1:19 UTC (permalink / raw) To: git; +Cc: Michael McClimon, Jeff King, Glen Choo This adds one commit on top of the last version, to avoid the security problem mentioned by Peff at [1]. It's possible this isn't the best way to go about this (and I would not really call myself a C programmer), but it's a minimal patch that seems to fix the problem. I'm not sure if I need to document the new environment variable somewhere or not; it's really an internal-only thing, so I didn't do so this time around, but happy to if needed. Thanks! [1] https://lore.kernel.org/git/Y1ImS1Muvk1MAQeC@coredump.intra.peff.net/ Michael McClimon (2): Git.pm: add semicolon after catch statement setup: allow Git.pm to do unsafe repo checking perl/Git.pm | 3 ++- setup.c | 3 +++ t/t9700-perl-git.sh | 4 ++++ t/t9700/test.pl | 18 ++++++++++++++++++ 4 files changed, 27 insertions(+), 1 deletion(-) Range-diff against v1: 1: 1337c855 = 1: 1337c855 Git.pm: add semicolon after catch statement -: -------- > 2: 273f77d1 setup: allow Git.pm to do unsafe repo checking -- 2.38.1.130.g45c9f05c ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/2] Git.pm: add semicolon after catch statement 2022-10-22 1:19 ` [PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories Michael McClimon @ 2022-10-22 1:19 ` Michael McClimon 2022-10-22 1:19 ` [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking Michael McClimon 1 sibling, 0 replies; 23+ messages in thread From: Michael McClimon @ 2022-10-22 1:19 UTC (permalink / raw) To: git; +Cc: Michael McClimon, Hank Leininger When attempting to initialize a repository object in an unsafe directory, a syntax error is reported (Can't use string as a HASH ref while strict refs in use). Fix this runtime error by adding the required semicolon after the catch statement. Without the semicolon, the result of the following line (i.e., the result of Cwd::abs_path) is passed as the third argument to Error.pm's catch function. That function expects that its third argument, $clauses, is a hash reference, and trying to access a string as a hash reference is a fatal error. [1] https://lore.kernel.org/git/20221011182607.f1113fff-9333-427d-ba45-741a78fa6040@korelogic.com/ Reported-by: Hank Leininger <hlein@korelogic.com> Signed-off-by: Michael McClimon <michael@mcclimon.org> --- perl/Git.pm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/perl/Git.pm b/perl/Git.pm index 080cdc2a..cf15ead6 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -217,7 +217,7 @@ sub repository { } catch Git::Error::Command with { # Mimic git-rev-parse --git-dir error message: throw Error::Simple("fatal: Not a git repository: $dir"); - } + }; $opts{Repository} = Cwd::abs_path($dir); } -- 2.38.1.130.g45c9f05c ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 1:19 ` [PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories Michael McClimon 2022-10-22 1:19 ` [PATCH v2 1/2] Git.pm: add semicolon after catch statement Michael McClimon @ 2022-10-22 1:19 ` Michael McClimon 2022-10-22 5:29 ` Junio C Hamano ` (2 more replies) 1 sibling, 3 replies; 23+ messages in thread From: Michael McClimon @ 2022-10-22 1:19 UTC (permalink / raw) To: git; +Cc: Michael McClimon The previous commit exposes a security flaw: it is possible to bypass unsafe repository checks by using Git.pm, where before the syntax error accidentally prohibited it. This problem occurs because Git.pm sets GIT_DIR explicitly, which bypasses the safe repository checks. Fix this by introducing a new environment variable, GIT_PERL_FORCE_OWNERSHIP_CHECK, which we set true in Git.pm. In setup.c, if that environment variable is true, force ownership checks even if an explicit GIT_DIR is provided. Signed-off-by: Michael McClimon <michael@mcclimon.org> --- perl/Git.pm | 1 + setup.c | 3 +++ t/t9700-perl-git.sh | 4 ++++ t/t9700/test.pl | 18 ++++++++++++++++++ 4 files changed, 26 insertions(+) diff --git a/perl/Git.pm b/perl/Git.pm index cf15ead6..002c29bb 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -1674,6 +1674,7 @@ sub _cmd_exec { sub _setup_git_cmd_env { my $self = shift; if ($self) { + $ENV{GIT_PERL_FORCE_OWNERSHIP_CHECK} = 1; $self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path(); $self->repo_path() and $self->wc_path() and $ENV{'GIT_WORK_TREE'} = $self->wc_path(); diff --git a/setup.c b/setup.c index cefd5f63..33d4e6fd 100644 --- a/setup.c +++ b/setup.c @@ -1250,6 +1250,9 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, */ gitdirenv = getenv(GIT_DIR_ENVIRONMENT); if (gitdirenv) { + if (git_env_bool("GIT_PERL_FORCE_OWNERSHIP_CHECK", 0) && + !ensure_valid_ownership(NULL, NULL, gitdirenv, report)) + return GIT_DIR_INVALID_OWNERSHIP; strbuf_addstr(gitdir, gitdirenv); return GIT_DIR_EXPLICIT; } diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh index 4aa5d90d..b14a40b1 100755 --- a/t/t9700-perl-git.sh +++ b/t/t9700-perl-git.sh @@ -45,6 +45,10 @@ test_expect_success \ git config --add test.pathmulti bar ' +test_expect_success 'set up bare repository' ' + git init --bare bare.git +' + test_expect_success 'use t9700/test.pl to test Git.pm' ' "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr && test_must_be_empty stderr diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db..1c91019f 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -142,6 +142,24 @@ sub adjust_dirsep { "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ", 'unquote escape sequences'); +# safe directory +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + # Save stderr to a tempfile so we can check the contents + open our $tmpstderr2, ">&STDERR" or die "cannot save STDERR"; + my $tmperr = "unsafeerr.tmp"; + open STDERR, ">", "$tmperr" or die "cannot redirect STDERR to $tmperr"; + my $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; + ok(!$failed, "reject unsafe repository"); + like($@, qr/not a git repository/i, "unsafe error message"); + open TEMPFILE, "<", "$tmperr" or die "Can't open $tmperr $!"; + my $errcontents; + { local $/; $errcontents = <TEMPFILE>; } + like($errcontents, qr/dubious ownership/, "dubious ownership message"); + close STDERR or die "cannot close temp stderr"; + open STDERR, ">&", $tmpstderr2 or die "cannot restore STDERR"; +} + printf "1..%d\n", Test::More->builder->current_test; my $is_passing = eval { Test::More->is_passing }; -- 2.38.1.130.g45c9f05c ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 1:19 ` [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking Michael McClimon @ 2022-10-22 5:29 ` Junio C Hamano 2022-10-22 21:18 ` Jeff King 2022-10-22 19:45 ` Ævar Arnfjörð Bjarmason 2022-10-22 21:16 ` Jeff King 2 siblings, 1 reply; 23+ messages in thread From: Junio C Hamano @ 2022-10-22 5:29 UTC (permalink / raw) To: Michael McClimon; +Cc: git Michael McClimon <michael@mcclimon.org> writes: > The previous commit exposes a security flaw: it is possible to bypass > unsafe repository checks by using Git.pm, where before the syntax error > accidentally prohibited it. This problem occurs because Git.pm sets > GIT_DIR explicitly, which bypasses the safe repository checks. > > Fix this by introducing a new environment variable, > GIT_PERL_FORCE_OWNERSHIP_CHECK, which we set true in Git.pm. In setup.c, > if that environment variable is true, force ownership checks even if > an explicit GIT_DIR is provided. Drop "PERL_" from the name, as any third-party tool that does its own repository discovery and sets GIT_DIR before it invokes git would need a similar "Yes, I am setting GIT_DIR, but I still want you to check if that passes the usual ownership check", and there is no reason to expect that these tools are always written in Perl. How about "GIT_SAFE_DIRECTORY_STRICT" or something along that line, instead? I also have to wonder (and this is *not* a suggestion for inventing an alternative fix for perl/Git.pm) if we were creating perl/Git.pm from scratch today, we even need to be worried about this issue. We may have Git::repo_path() helper but if we call it in a natural way (read: as if an interactive end-user would type commands), it is likely that we would run "git rev-parse --git-dir" or something without setting GIT_DIR, and when we need to run "git" command, say "git diff", we would also run "git diff" as if the end user would type from within their interactive session and without setting GIT_DIR, and everything should work. IOW, why do we even setting and exporting the auto-detected value in GIT_DIR? Also, if the end user had GIT_DIR in the environment _before_ calling something that uses "import Git", what should happen? In that case, the end-user is who is telling us that the named directory is OK without any forced ownership check, so it feels WRONG that this patch UNCONDITIONALLY exports FORCE_CHECK. Only when we did auto-discovery via Git::repo_path() without end-user supplied GIT_DIR and exported GIT_DIR, we should also export FORCE_CHECK to tell the underlying "git" that our auto-detection may be flawed and it needs to double check, no? > Signed-off-by: Michael McClimon <michael@mcclimon.org> > --- > perl/Git.pm | 1 + > setup.c | 3 +++ > t/t9700-perl-git.sh | 4 ++++ > t/t9700/test.pl | 18 ++++++++++++++++++ > 4 files changed, 26 insertions(+) > > diff --git a/perl/Git.pm b/perl/Git.pm > index cf15ead6..002c29bb 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -1674,6 +1674,7 @@ sub _cmd_exec { > sub _setup_git_cmd_env { > my $self = shift; > if ($self) { > + $ENV{GIT_PERL_FORCE_OWNERSHIP_CHECK} = 1; > $self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path(); > $self->repo_path() and $self->wc_path() > and $ENV{'GIT_WORK_TREE'} = $self->wc_path(); > diff --git a/setup.c b/setup.c > index cefd5f63..33d4e6fd 100644 > --- a/setup.c > +++ b/setup.c > @@ -1250,6 +1250,9 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir, > */ > gitdirenv = getenv(GIT_DIR_ENVIRONMENT); > if (gitdirenv) { > + if (git_env_bool("GIT_PERL_FORCE_OWNERSHIP_CHECK", 0) && > + !ensure_valid_ownership(NULL, NULL, gitdirenv, report)) > + return GIT_DIR_INVALID_OWNERSHIP; > strbuf_addstr(gitdir, gitdirenv); > return GIT_DIR_EXPLICIT; > } > diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh > index 4aa5d90d..b14a40b1 100755 > --- a/t/t9700-perl-git.sh > +++ b/t/t9700-perl-git.sh > @@ -45,6 +45,10 @@ test_expect_success \ > git config --add test.pathmulti bar > ' > > +test_expect_success 'set up bare repository' ' > + git init --bare bare.git > +' > + > test_expect_success 'use t9700/test.pl to test Git.pm' ' > "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr && > test_must_be_empty stderr > diff --git a/t/t9700/test.pl b/t/t9700/test.pl > index e046f7db..1c91019f 100755 > --- a/t/t9700/test.pl > +++ b/t/t9700/test.pl > @@ -142,6 +142,24 @@ sub adjust_dirsep { > "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ", > 'unquote escape sequences'); > > +# safe directory > +{ > + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; > + # Save stderr to a tempfile so we can check the contents > + open our $tmpstderr2, ">&STDERR" or die "cannot save STDERR"; > + my $tmperr = "unsafeerr.tmp"; > + open STDERR, ">", "$tmperr" or die "cannot redirect STDERR to $tmperr"; > + my $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; > + ok(!$failed, "reject unsafe repository"); > + like($@, qr/not a git repository/i, "unsafe error message"); > + open TEMPFILE, "<", "$tmperr" or die "Can't open $tmperr $!"; > + my $errcontents; > + { local $/; $errcontents = <TEMPFILE>; } > + like($errcontents, qr/dubious ownership/, "dubious ownership message"); > + close STDERR or die "cannot close temp stderr"; > + open STDERR, ">&", $tmpstderr2 or die "cannot restore STDERR"; > +} > + > printf "1..%d\n", Test::More->builder->current_test; > > my $is_passing = eval { Test::More->is_passing }; ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 5:29 ` Junio C Hamano @ 2022-10-22 21:18 ` Jeff King 2022-10-22 23:17 ` Junio C Hamano 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2022-10-22 21:18 UTC (permalink / raw) To: Junio C Hamano; +Cc: Michael McClimon, git On Fri, Oct 21, 2022 at 10:29:33PM -0700, Junio C Hamano wrote: > I also have to wonder (and this is *not* a suggestion for inventing > an alternative fix for perl/Git.pm) if we were creating perl/Git.pm > from scratch today, we even need to be worried about this issue. We > may have Git::repo_path() helper but if we call it in a natural way > (read: as if an interactive end-user would type commands), it is > likely that we would run "git rev-parse --git-dir" or something > without setting GIT_DIR, and when we need to run "git" command, say > "git diff", we would also run "git diff" as if the end user would > type from within their interactive session and without setting > GIT_DIR, and everything should work. IOW, why do we even setting > and exporting the auto-detected value in GIT_DIR? I think it has to in order to avoid surprises. If I do this: perl -MGit -e ' my $r = Git->repository; chdir("/somewhere/else"); $r->git_command(...); ' that command ought to run in the repository I opened earlier. So I think to keep the illusion of a lib-ified object, creating that object has to lock in the path. But it really seems like we should be asking rev-parse what that path is, not trying to do any magic ourselves. -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 21:18 ` Jeff King @ 2022-10-22 23:17 ` Junio C Hamano 0 siblings, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2022-10-22 23:17 UTC (permalink / raw) To: Jeff King; +Cc: Michael McClimon, git Jeff King <peff@peff.net> writes: > I think it has to in order to avoid surprises. If I do this: > > perl -MGit -e ' > my $r = Git->repository; > chdir("/somewhere/else"); > $r->git_command(...); > ' > > that command ought to run in the repository I opened earlier. So I think > to keep the illusion of a lib-ified object, creating that object has to > lock in the path. > > But it really seems like we should be asking rev-parse what that path > is, not trying to do any magic ourselves. Yeah, whichever caller doing the chdir() needs to take the responsibility of adjusting the future use of git, e.g. going back to the original before spawning git or whatever. Or having the original Git->repository call bail out by having "git" figure out where it is, while honoring safe.directory or any future protection underlying "git" offers. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 1:19 ` [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking Michael McClimon 2022-10-22 5:29 ` Junio C Hamano @ 2022-10-22 19:45 ` Ævar Arnfjörð Bjarmason 2022-10-22 20:55 ` Jeff King 2022-10-22 21:16 ` Jeff King 2 siblings, 1 reply; 23+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-10-22 19:45 UTC (permalink / raw) To: Michael McClimon; +Cc: git On Fri, Oct 21 2022, Michael McClimon wrote: > The previous commit exposes a security flaw: it is possible to bypass > unsafe repository checks by using Git.pm, where before the syntax error > accidentally prohibited it. This problem occurs because Git.pm sets > GIT_DIR explicitly, which bypasses the safe repository checks. > > Fix this by introducing a new environment variable, > GIT_PERL_FORCE_OWNERSHIP_CHECK, which we set true in Git.pm. In setup.c, > if that environment variable is true, force ownership checks even if > an explicit GIT_DIR is provided. The 1/2 is unambiguously good, thanks. As for this step, I think it's good if we want to go in this direction. As to whether it's the direction we want... The vulnerability safe.directory was supposed to address was one where you'd set your fsmonitor hook via a config variable, so running "diff", "status" etc. would unexpectedly execute arbitrary code. Especially on Windows where apparently the equivalent of the root of a shared mounted volume routinely has global write permissions (user's subdirectories being less permissive). An alternative I raised on the security list was to narrowly target just the fsmonitor config, since that was the vulnerability. But it was decided to cast a wider net, as it wasn't disclosed yet, and the list members might have missed some other config variable that would allow shenanigans, fair enough, especially for a time constrained security fix. Now that the cat's thoroughly out of the bag I think we'd do well to reconsider that. I'm unaware of any other variable(s) that provide a similar vector, and safe.directory is encouraging users (especially in core.sharedRepository settings) to mark a dir as "safe", and we'd then later have an exploit from a user with rw access who'd use the fsmonitor hook vector. Better have them "safe by default", and start refusing to read the config when we detect that fsmonitor setting, and insist the user mark *that* safe. Anyway, that's all on the general topic, and the above is just my opinion on it. But on this much more narrow topic: If we couldn't come up with an issue other than the fsmonitor config for git's C codebase, I think extending the same mitigation to the very small part of git that's still in Perl is thoroughly into the tail wagging the dog territory. I.e. what we'd presumably get out of this is mitigation against some exploit via a variable that git-send-email or git-svn use (or the handful of other more obscure Perl tooling we have). Can we think of one that could be an issue, and if not is the current behavior in 1/2 maybe OK as-is? > test_expect_success 'use t9700/test.pl to test Git.pm' ' > "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr && > test_must_be_empty stderr > diff --git a/t/t9700/test.pl b/t/t9700/test.pl > index e046f7db..1c91019f 100755 > --- a/t/t9700/test.pl > +++ b/t/t9700/test.pl > @@ -142,6 +142,24 @@ sub adjust_dirsep { > "abc\"\\ \x07\x08\x09\x0a\x0b\x0c\x0d\x01 ", > 'unquote escape sequences'); > > +# safe directory > +{ > + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; > + # Save stderr to a tempfile so we can check the contents > + open our $tmpstderr2, ">&STDERR" or die "cannot save STDERR"; > + my $tmperr = "unsafeerr.tmp"; > + open STDERR, ">", "$tmperr" or die "cannot redirect STDERR to $tmperr"; > + my $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; > + ok(!$failed, "reject unsafe repository"); > + like($@, qr/not a git repository/i, "unsafe error message"); > + open TEMPFILE, "<", "$tmperr" or die "Can't open $tmperr $!"; > + my $errcontents; > + { local $/; $errcontents = <TEMPFILE>; } > + like($errcontents, qr/dubious ownership/, "dubious ownership message"); > + close STDERR or die "cannot close temp stderr"; > + open STDERR, ">&", $tmpstderr2 or die "cannot restore STDERR"; > +} This whole "save stderr to a file" etc. seems much more complex than just writing the same test in our normal *.sh tests, and doing something like: ! GIT_TEST_ASSUME_DIFFERENT_OWNER=1 perl -MGit -we 'Git->repository(Directory => shift)' 2>expect && grep "reject unusafe" ... I.e. you're spending a lot of effort on capturing STDERR within a single Perl process, then restoring it etc., when we can just run one-off command and have it die (no eval), and just inspect the exit code & stderr perl emitted. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 19:45 ` Ævar Arnfjörð Bjarmason @ 2022-10-22 20:55 ` Jeff King 2022-10-24 10:57 ` Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 23+ messages in thread From: Jeff King @ 2022-10-22 20:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Michael McClimon, git On Sat, Oct 22, 2022 at 09:45:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > The vulnerability safe.directory was supposed to address was one where > you'd set your fsmonitor hook via a config variable, so running "diff", > "status" etc. would unexpectedly execute arbitrary code. > > Especially on Windows where apparently the equivalent of the root of a > shared mounted volume routinely has global write permissions (user's > subdirectories being less permissive). > > An alternative I raised on the security list was to narrowly target just > the fsmonitor config, since that was the vulnerability. > > [...] > > I'm unaware of any other variable(s) that provide a similar vector, and > safe.directory is encouraging users (especially in core.sharedRepository > settings) to mark a dir as "safe", and we'd then later have an exploit > from a user with rw access who'd use the fsmonitor hook vector. Here are a few off the top of my head that you can trigger via git-diff: - core.pager will run an arbitrary program - pager.diff will run an arbitrary program - diff.*.textconv runs an arbitrary program; you need matching .gitattributes, but those are under the control of the repository. (not diff.*.command, though, as you need to pass --ext-diff) - browser/man paths if you run "git diff --help" And of course as you expand the set of commands there are more options. E.g., credential helper commands if you do anything that wants auth, interactive diff-filter for "add -p", hooks for git-commit, git-push, etc. Those commands are less likely to be run in an untrusted repository than inspection commands like "status" or "diff", but the boundary is getting quite fuzzy. fsmonitor _might_ be the only one that is triggered by git-prompt.sh, because it has a limited command palette, generally reads (or sends to /dev/null) the stdout of commands (preventing pager invocation), and doesn't do text diffs (so no textconv). Even if true, I'm not sure if that's a good place to draw the line, though. People do tend to run "git log" themselves. -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 20:55 ` Jeff King @ 2022-10-24 10:57 ` Ævar Arnfjörð Bjarmason 2022-10-24 23:38 ` Jeff King 0 siblings, 1 reply; 23+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-10-24 10:57 UTC (permalink / raw) To: Jeff King; +Cc: Michael McClimon, git On Sat, Oct 22 2022, Jeff King wrote: > On Sat, Oct 22, 2022 at 09:45:14PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> The vulnerability safe.directory was supposed to address was one where >> you'd set your fsmonitor hook via a config variable, so running "diff", >> "status" etc. would unexpectedly execute arbitrary code. >> >> Especially on Windows where apparently the equivalent of the root of a >> shared mounted volume routinely has global write permissions (user's >> subdirectories being less permissive). >> >> An alternative I raised on the security list was to narrowly target just >> the fsmonitor config, since that was the vulnerability. >> >> [...] >> >> I'm unaware of any other variable(s) that provide a similar vector, and >> safe.directory is encouraging users (especially in core.sharedRepository >> settings) to mark a dir as "safe", and we'd then later have an exploit >> from a user with rw access who'd use the fsmonitor hook vector. > > Here are a few off the top of my head that you can trigger via git-diff: > > - core.pager will run an arbitrary program > > - pager.diff will run an arbitrary program > > - diff.*.textconv runs an arbitrary program; you need matching > .gitattributes, but those are under the control of the repository. > (not diff.*.command, though, as you need to pass --ext-diff) > > - browser/man paths if you run "git diff --help" > > And of course as you expand the set of commands there are more options. > E.g., credential helper commands if you do anything that wants auth, > interactive diff-filter for "add -p", hooks for git-commit, git-push, > etc. Those commands are less likely to be run in an untrusted repository > than inspection commands like "status" or "diff", but the boundary is > getting quite fuzzy. > > fsmonitor _might_ be the only one that is triggered by git-prompt.sh, > because it has a limited command palette, generally reads (or sends to > /dev/null) the stdout of commands (preventing pager invocation), and > doesn't do text diffs (so no textconv). Even if true, I'm not sure if > that's a good place to draw the line, though. People do tend to run "git > log" themselves. Right, by "a similar" vector I meant the unexpected execution of fsmonitor as there was software in the wild that was running "status" for the user. These are also a problem, but my understanding of that issue is that if it wasn't for the fsmonitor issue we'd have put that in the bucket of not running arbitrary commands on a .git you just copied to somewhere, i.e. that issue was already known. The difference being that users might have that implicit "status" running if they cd'd to /mnt/$USER/subdir and there was a hostile /mnt/.git, but would be much less likely to run "git diff" or whatever in such a subdir, unless they'd initialized a .git in say /mnt/$USER/subdir, at which point we'd ignore the /mnt/.git. Anyway, that's more into the "would it have been a CVE?" territory, so let's leave that for now. The important point/question I have is whether we can think of any such config variable understood by the code that uses Git.pm. The only ones I can think are the "sendemail.{to,cc}Cmd" variables. I'm just pointing out that the reason we ended up with the facility (per my understand) was among other things: * A. There were too many config variables to exhaustively audit on the security list and be sure we caught them all, and ... * B. ...such a change would probably be larger, which ... * C. ...given the embargo & desire to keep security patches minimal warranted the more general safe.directory approach. * D. You can talk about on the public list, or not have a zero-day, but not both :) Now, we may come up with a reason "E" for extending this at this point, e.g. maybe just being consistent is reason enough. But in this case "A" doesn't apply, it's maybe 20-30 config variables, and it's easy to skim the git-{send-email,svn} docs to see what they are. "B" might be the case, but taht's OK since we're past "D" here, ditto "C" not applying. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-24 10:57 ` Ævar Arnfjörð Bjarmason @ 2022-10-24 23:38 ` Jeff King 0 siblings, 0 replies; 23+ messages in thread From: Jeff King @ 2022-10-24 23:38 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason; +Cc: Michael McClimon, git On Mon, Oct 24, 2022 at 12:57:29PM +0200, Ævar Arnfjörð Bjarmason wrote: > The important point/question I have is whether we can think of any such > config variable understood by the code that uses Git.pm. I don't think that matters. Before the CVE fix, Git.pm scripts were just as vulnerable as all the other parts of Git. After, they were broken because of the syntax error. Fixing the syntax error re-opened the bug there, but as long as we close it again before releasing, we don't have to care. You can argue that the CVE wasn't that important for Git.pm, and thus not that big a deal to re-open. But I think post-CVE we're making the stronger promise that Git won't discover a repo directory with funky ownership. And Git.pm is violating that (or would be after the syntax fix if we don't go further). > The only ones I can think are the "sendemail.{to,cc}Cmd" variables. I don't think we can be that exhaustive. It's also any programs that are called by scripts using Git.pm. But even that is not a closed set, since we ship Git.pm for people to use in their own scripts. We don't know what those scripts might be doing. -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 1:19 ` [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking Michael McClimon 2022-10-22 5:29 ` Junio C Hamano 2022-10-22 19:45 ` Ævar Arnfjörð Bjarmason @ 2022-10-22 21:16 ` Jeff King 2022-10-22 22:08 ` Jeff King 2022-10-22 23:14 ` Junio C Hamano 2 siblings, 2 replies; 23+ messages in thread From: Jeff King @ 2022-10-22 21:16 UTC (permalink / raw) To: Michael McClimon; +Cc: git On Fri, Oct 21, 2022 at 09:19:32PM -0400, Michael McClimon wrote: > diff --git a/perl/Git.pm b/perl/Git.pm > index cf15ead6..002c29bb 100644 > --- a/perl/Git.pm > +++ b/perl/Git.pm > @@ -1674,6 +1674,7 @@ sub _cmd_exec { > sub _setup_git_cmd_env { > my $self = shift; > if ($self) { > + $ENV{GIT_PERL_FORCE_OWNERSHIP_CHECK} = 1; > $self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path(); > $self->repo_path() and $self->wc_path() > and $ENV{'GIT_WORK_TREE'} = $self->wc_path(); I'm not familiar enough with Git.pm to know if this is the right spot. But we'd not want to break the case where GIT_DIR is set already. I.e.: GIT_DIR=/path/to/repo.git perl -MGit -e 'Git->repository' should continue to work regardless of the ownership of repo.git. Only the repo-discovery phase would want to force the ownership check. Again, I'm not too familiar with Git.pm, but it seems it ought to be asking Git: are we in a valid Git repo, and if so where is it? Something like: my $git_dir = `git rev-parse --absolute-git-dir`; $? and die "nope, not in a git repo"; # later, when we run git commands, we do specify this; the script may # have chdir()'d in the meantime, and we want to make sure we are # referring to the same repo via the object. local $ENV{GIT_DIR} = abs_path($git_dir); ...run some git command... Looking at the code, we even seem to do that first part! But if it returns an error, then we go on to check for a bare repository ourselves by looking for refs/, objects/, etc. Which is just...weird. It feels like this try/catch should just go away: diff --git a/perl/Git.pm b/perl/Git.pm index cf15ead664..7a7d8a2987 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -177,13 +177,7 @@ sub repository { -d $opts{Directory} or throw Error::Simple("Directory not found: $opts{Directory} $!"); my $search = Git->repository(WorkingCopy => $opts{Directory}); - my $dir; - try { - $dir = $search->command_oneline(['rev-parse', '--git-dir'], - STDERR => 0); - } catch Git::Error::Command with { - $dir = undef; - }; + my $dir = $search->command_oneline(['rev-parse', '--git-dir']); require Cwd; if ($dir) { And then the code below that to check for bare/not-bare should be using "git rev-parse --is-bare-repository" or similar. Something like: diff --git a/perl/Git.pm b/perl/Git.pm index 7a7d8a2987..280df9cee1 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -179,8 +179,14 @@ sub repository { my $search = Git->repository(WorkingCopy => $opts{Directory}); my $dir = $search->command_oneline(['rev-parse', '--git-dir']); + # could be merged with command above to be more efficient; or + # could probably use --show-toplevel to avoid prefix query + # below + my $bare = $search->command_oneline(['rev-parse', '--is-bare-repository']) + eq 'true'; + require Cwd; - if ($dir) { + if (!$bare) { require File::Spec; File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir; $opts{Repository} = Cwd::abs_path($dir); @@ -198,21 +204,6 @@ sub repository { $opts{WorkingSubdir} = $prefix; } else { - # A bare repository? Let's see... - $dir = $opts{Directory}; - - unless (-d "$dir/refs" and -d "$dir/objects" and -e "$dir/HEAD") { - # Mimic git-rev-parse --git-dir error message: - throw Error::Simple("fatal: Not a git repository: $dir"); - } - my $search = Git->repository(Repository => $dir); - try { - $search->command('symbolic-ref', 'HEAD'); - } catch Git::Error::Command with { - # Mimic git-rev-parse --git-dir error message: - throw Error::Simple("fatal: Not a git repository: $dir"); - }; - $opts{Repository} = Cwd::abs_path($dir); } But given how much more complicated the current code is, I wonder if I am missing some case. Or perhaps this code is just so old that it used to do this stuff itself (because rev-parse didn't give us so much help). -Peff ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 21:16 ` Jeff King @ 2022-10-22 22:08 ` Jeff King 2022-10-22 23:19 ` Michael McClimon 2022-10-22 23:14 ` Junio C Hamano 1 sibling, 1 reply; 23+ messages in thread From: Jeff King @ 2022-10-22 22:08 UTC (permalink / raw) To: Michael McClimon; +Cc: Junio C Hamano, git On Sat, Oct 22, 2022 at 05:16:38PM -0400, Jeff King wrote: > Again, I'm not too familiar with Git.pm, but it seems it ought to be > asking Git: are we in a valid Git repo, and if so where is it? Something > like: > > my $git_dir = `git rev-parse --absolute-git-dir`; > $? and die "nope, not in a git repo"; > > # later, when we run git commands, we do specify this; the script may > # have chdir()'d in the meantime, and we want to make sure we are > # referring to the same repo via the object. > local $ENV{GIT_DIR} = abs_path($git_dir); > ...run some git command... > > Looking at the code, we even seem to do that first part! But if it > returns an error, then we go on to check for a bare repository > ourselves by looking for refs/, objects/, etc. Which is just...weird. > > It feels like this try/catch should just go away: It's a little more complicated than that, because presumably people rely on the error handling for a missing repo to not be noisy. So here's a polished version of what I showed, along with the tests we were discussing earlier. I prepared it on top of your fix in the mm/git-pm-try-catch-syntax-fix branch. That's not strictly necessary, since my patch deletes the line you fixed. :) But I think it's nicer to use your fix as the starting point, since it means the test runs but produces the wrong behavior, rather than barfing with a syntax error. -- >8 -- Subject: [PATCH] Git.pm: trust rev-parse to find bare repositories When initializing a repository object, we run "git rev-parse --git-dir" to let the C version of Git find the correct directory. But curiously, if this fails we don't automatically say "not a git repository". Instead, we do our own pure-perl check to see if we're in a bare repository. This makes little sense, as rev-parse will report both bare and non-bare directories. This logic comes from d5c7721d58 (Git.pm: Add support for subdirectories inside of working copies, 2006-06-24), but I don't see any reason given why we can't just rely on rev-parse. Worse, because we treat any non-error response from rev-parse as a non-bare repository, we'll erroneously set the object's WorkingCopy, even in a bare repository. But it gets worse. Since 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02), it's actively wrong (and dangerous). The perl code doesn't implement the same ownership checks. And worse, after "finding" the bare repository, it sets GIT_DIR in the environment, which tells any subsequent Git commands that we've confirmed the directory is OK, and to trust us. I.e., it re-opens the vulnerability plugged by 8959555cee when using Git.pm's repository discovery code. We can fix this by just relying on rev-parse to tell us when we're not in a repository, which fixes the vulnerability. Furthermore, we'll ask its --is-bare-repository function to tell us if we're bare or not, and rely on that. Signed-off-by: Jeff King <peff@peff.net> --- I didn't dig into the "oops, we set WorkingCopy" thing beyond manually verifying that it happens. It doesn't look like its really used beyond the wc_path() method, so it's not like it would have been breaking git sub-commands, etc. I guess we could add a test that wc_path() returns undef in a bare repository, though. perl/Git.pm | 36 ++++++++++++++++-------------------- t/t9700-perl-git.sh | 4 ++++ t/t9700/test.pl | 12 ++++++++++++ 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/perl/Git.pm b/perl/Git.pm index cf15ead664..117765dc73 100644 --- a/perl/Git.pm +++ b/perl/Git.pm @@ -177,16 +177,27 @@ sub repository { -d $opts{Directory} or throw Error::Simple("Directory not found: $opts{Directory} $!"); my $search = Git->repository(WorkingCopy => $opts{Directory}); - my $dir; + + # This rev-parse will throw an exception if we're not in a + # repository, which is what we want, but it's kind of noisy. + # Ideally we'd capture stderr and relay it, but doing so is + # awkward without depending on it fitting in a pipe buffer. So + # we just reproduce a plausible error message ourselves. + my $out; try { - $dir = $search->command_oneline(['rev-parse', '--git-dir'], - STDERR => 0); + # Note that "--is-bare-repository" must come first, as + # --git-dir output could contain newlines. + $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)], + STDERR => 0); } catch Git::Error::Command with { - $dir = undef; + throw Error::Simple("fatal: not a git repository: $opts{Directory}"); }; + chomp $out; + my ($bare, $dir) = split /\n/, $out, 2; + require Cwd; - if ($dir) { + if ($bare ne 'true') { require File::Spec; File::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir; $opts{Repository} = Cwd::abs_path($dir); @@ -204,21 +215,6 @@ sub repository { $opts{WorkingSubdir} = $prefix; } else { - # A bare repository? Let's see... - $dir = $opts{Directory}; - - unless (-d "$dir/refs" and -d "$dir/objects" and -e "$dir/HEAD") { - # Mimic git-rev-parse --git-dir error message: - throw Error::Simple("fatal: Not a git repository: $dir"); - } - my $search = Git->repository(Repository => $dir); - try { - $search->command('symbolic-ref', 'HEAD'); - } catch Git::Error::Command with { - # Mimic git-rev-parse --git-dir error message: - throw Error::Simple("fatal: Not a git repository: $dir"); - }; - $opts{Repository} = Cwd::abs_path($dir); } diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh index 4aa5d90d32..b105d6d9d5 100755 --- a/t/t9700-perl-git.sh +++ b/t/t9700-perl-git.sh @@ -45,6 +45,10 @@ test_expect_success \ git config --add test.pathmulti bar ' +test_expect_success 'set up bare repository' ' + git init --bare bare.git +' + test_expect_success 'use t9700/test.pl to test Git.pm' ' "$PERL_PATH" "$TEST_DIRECTORY"/t9700/test.pl 2>stderr && test_must_be_empty stderr diff --git a/t/t9700/test.pl b/t/t9700/test.pl index e046f7db76..6d753708d2 100755 --- a/t/t9700/test.pl +++ b/t/t9700/test.pl @@ -30,6 +30,18 @@ sub adjust_dirsep { # set up our $abs_repo_dir = cwd(); ok(our $r = Git->repository(Directory => "."), "open repository"); +{ + local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1; + my $failed; + + $failed = eval { Git->repository(Directory => $abs_repo_dir) }; + ok(!$failed, "reject unsafe non-bare repository"); + like($@, qr/not a git repository/i, "unsafe error message"); + + $failed = eval { Git->repository(Directory => "$abs_repo_dir/bare.git") }; + ok(!$failed, "reject unsafe bare repository"); + like($@, qr/not a git repository/i, "unsafe error message"); +} # config is($r->config("test.string"), "value", "config scalar: string"); -- 2.38.1.497.g093b959162 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 22:08 ` Jeff King @ 2022-10-22 23:19 ` Michael McClimon 2022-10-24 23:33 ` Jeff King 0 siblings, 1 reply; 23+ messages in thread From: Michael McClimon @ 2022-10-22 23:19 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, git > I prepared it on top of your fix in the mm/git-pm-try-catch-syntax-fix > branch. That's not strictly necessary, since my patch deletes the line > you fixed. :) But I think it's nicer to use your fix as the starting > point, since it means the test runs but produces the wrong behavior, > rather than barfing with a syntax error. My vanity thanks you for this, even if it's not strictly necessary. As a professional programmer with roughly no C chops and a long-time admirer of the Git project, all I _really_ wanted to do was to fix a thing that was in my wheelhouse so that I could say I have a commit in the history. (This isn't a good reason on its own, of course, but I'm happy it was useful even if the line is immediately deleted!) > We can fix this by just relying on rev-parse to tell us when we're not > in a repository, which fixes the vulnerability. Furthermore, we'll ask > its --is-bare-repository function to tell us if we're bare or not, and > rely on that. Your suggested patch seems fine to me, and indeed I think if we were writing it today we'd just rely on rev-parse to do the heavy lifting. It looks like the code in question -- and indeed, the syntax error in question -- blames to d5c7721d (Git.pm: Add support for subdirectories inside of working copies, 2006-06-23), at which point rev-parse did not appear to have any special handling for bare repositories. -- Michael McClimon michael@mcclimon.org ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 23:19 ` Michael McClimon @ 2022-10-24 23:33 ` Jeff King 0 siblings, 0 replies; 23+ messages in thread From: Jeff King @ 2022-10-24 23:33 UTC (permalink / raw) To: Michael McClimon; +Cc: Junio C Hamano, git On Sat, Oct 22, 2022 at 07:19:21PM -0400, Michael McClimon wrote: > > I prepared it on top of your fix in the mm/git-pm-try-catch-syntax-fix > > branch. That's not strictly necessary, since my patch deletes the line > > you fixed. :) But I think it's nicer to use your fix as the starting > > point, since it means the test runs but produces the wrong behavior, > > rather than barfing with a syntax error. > > My vanity thanks you for this, even if it's not strictly necessary. As a > professional programmer with roughly no C chops and a long-time admirer of the > Git project, all I _really_ wanted to do was to fix a thing that was in my > wheelhouse so that I could say I have a commit in the history. (This isn't a > good reason on its own, of course, but I'm happy it was useful even if the > line is immediately deleted!) :) I think it makes my patch easier to understand, but I'm glad you are getting some you, too. Finding the original issue and the ensuing discussion was as much (or more) effort than the actual fix. Thanks for starting it! -Peff ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking 2022-10-22 21:16 ` Jeff King 2022-10-22 22:08 ` Jeff King @ 2022-10-22 23:14 ` Junio C Hamano 1 sibling, 0 replies; 23+ messages in thread From: Junio C Hamano @ 2022-10-22 23:14 UTC (permalink / raw) To: Jeff King; +Cc: Michael McClimon, git Jeff King <peff@peff.net> writes: > It feels like this try/catch should just go away: > ... > And then the code below that to check for bare/not-bare should be using > "git rev-parse --is-bare-repository" or similar. Something like: Yeah, exactly. I very much like the straight-forward way of thinking to have "git" do the real work. Thanks. ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-11-10 21:42 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-16 21:22 [PATCH 0/1] Git.pm: add semicolon after catch statement Michael McClimon 2022-10-16 21:22 ` [PATCH 1/1] " Michael McClimon 2022-10-16 23:18 ` Jeff King 2022-10-17 2:17 ` Michael McClimon 2022-10-17 17:34 ` Jeff King 2022-10-18 1:39 ` Michael McClimon 2022-11-10 15:10 ` Johannes Schindelin 2022-11-10 21:41 ` Jeff King 2022-10-22 1:19 ` [PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories Michael McClimon 2022-10-22 1:19 ` [PATCH v2 1/2] Git.pm: add semicolon after catch statement Michael McClimon 2022-10-22 1:19 ` [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking Michael McClimon 2022-10-22 5:29 ` Junio C Hamano 2022-10-22 21:18 ` Jeff King 2022-10-22 23:17 ` Junio C Hamano 2022-10-22 19:45 ` Ævar Arnfjörð Bjarmason 2022-10-22 20:55 ` Jeff King 2022-10-24 10:57 ` Ævar Arnfjörð Bjarmason 2022-10-24 23:38 ` Jeff King 2022-10-22 21:16 ` Jeff King 2022-10-22 22:08 ` Jeff King 2022-10-22 23:19 ` Michael McClimon 2022-10-24 23:33 ` Jeff King 2022-10-22 23:14 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).