{"thread":{"id":"58633","subject":"[PATCH 0/1] Git.pm: add semicolon after catch statement","startedAt":"2022-10-16T21:22:48Z","lastAt":"2022-11-10T21:42:04Z","messageCount":23,"participants":["Michael McClimon","Jeff King","Junio C Hamano","Ævar Arnfjörð Bjarmason","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"465047","messageId":"20221016212236.12453-1-michael@mcclimon.org","threadId":"58633","inReplyTo":null,"subject":"[PATCH 0/1] Git.pm: add semicolon after catch statement","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-16T21:22:35Z","receivedAt":"2022-10-16T21:22:48Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"When looking into a bug about Git.pm's handling of unsafe repositories [1],\nI found the immediate cause of the error, which this patch fixes. This\npatch doesn't do anything about the actual problem (Git.pm happily\ncontinues to work in unsafe repositories), but it at least fixes the\nruntime error.\n\n[1] https://lore.kernel.org/git/20221011182607.f1113fff-9333-427d-ba45-741a78fa6040@korelogic.com/\n\n\nMichael McClimon (1):\n  Git.pm: add semicolon after catch statement\n\n perl/Git.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\n-- \n2.38.0.83.gd420dda0\n\n"},{"id":"465048","messageId":"20221016212236.12453-2-michael@mcclimon.org","threadId":"58633","inReplyTo":"20221016212236.12453-1-michael@mcclimon.org","subject":"[PATCH 1/1] Git.pm: add semicolon after catch statement","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-16T21:22:36Z","receivedAt":"2022-10-16T21:22:58Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"When attempting to initialize a repository object in an unsafe\ndirectory, a syntax error is reported (Can't use string as a HASH ref\nwhile strict refs in use). Fix this runtime error by adding the required\nsemicolon after the catch statement.\n\nWithout the semicolon, the result of the following line (i.e., the\nresult of Cwd::abs_path) is passed as the third argument to Error.pm's\ncatch function. That function expects that its third argument,\n$clauses, is a hash reference, and trying to access a string as a hash\nreference is a fatal error.\n\n[1] https://lore.kernel.org/git/20221011182607.f1113fff-9333-427d-ba45-741a78fa6040@korelogic.com/\n\nReported-by: Hank Leininger <hlein@korelogic.com>\nSigned-off-by: Michael McClimon <michael@mcclimon.org>\n---\n perl/Git.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 080cdc2a..cf15ead6 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -217,7 +217,7 @@ sub repository {\n \t\t\t} catch Git::Error::Command with {\n \t\t\t\t# Mimic git-rev-parse --git-dir error message:\n \t\t\t\tthrow Error::Simple(\"fatal: Not a git repository: $dir\");\n-\t\t\t}\n+\t\t\t};\n \n \t\t\t$opts{Repository} = Cwd::abs_path($dir);\n \t\t}\n-- \n2.38.0.83.gd420dda0\n\n"},{"id":"465050","messageId":"Y0yRStZ6gM+H8/Bf@coredump.intra.peff.net","threadId":"58633","inReplyTo":"20221016212236.12453-2-michael@mcclimon.org","subject":"Re: [PATCH 1/1] Git.pm: add semicolon after catch statement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-16T23:18:34Z","receivedAt":"2022-10-16T23:20:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 16, 2022 at 05:22:36PM -0400, Michael McClimon wrote:\n\n> When attempting to initialize a repository object in an unsafe\n> directory, a syntax error is reported (Can't use string as a HASH ref\n> while strict refs in use). Fix this runtime error by adding the required\n> semicolon after the catch statement.\n> \n> Without the semicolon, the result of the following line (i.e., the\n> result of Cwd::abs_path) is passed as the third argument to Error.pm's\n> catch function. That function expects that its third argument,\n> $clauses, is a hash reference, and trying to access a string as a hash\n> reference is a fatal error.\n\nCuriously this works as expected for me, both before and after your\npatch. I wonder if it depends on perl version. Mine is 5.34.\n\nI've never used Error.pm's try/catch before, so I don't know what's\nnormal. Regular if/unless doesn't need it, but certainly an earlier\ncatch uses a semicolon. So it seems like a reasonable fix.\n\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index 080cdc2a..cf15ead6 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -217,7 +217,7 @@ sub repository {\n>  \t\t\t} catch Git::Error::Command with {\n>  \t\t\t\t# Mimic git-rev-parse --git-dir error message:\n>  \t\t\t\tthrow Error::Simple(\"fatal: Not a git repository: $dir\");\n> -\t\t\t}\n> +\t\t\t};\n\nI'd assume t9700 passes for you, since I don't think we cover this case.\nMaybe it's worth squashing this in:\n\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex e046f7db76..5bd3687f37 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -30,6 +30,12 @@ sub adjust_dirsep {\n # set up\n our $abs_repo_dir = cwd();\n ok(our $r = Git->repository(Directory => \".\"), \"open repository\");\n+{\n+\tlocal $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n+\tmy $failed = eval { Git->repository(Directory => \".\") };\n+\tok(!$failed, \"reject unsafe repository\");\n+\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n+}\n \n # config\n is($r->config(\"test.string\"), \"value\", \"config scalar: string\");\n"},{"id":"465055","messageId":"Y0y7UdLf3qd7RgVQ@newk","threadId":"58633","inReplyTo":"Y0yRStZ6gM+H8/Bf@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] Git.pm: add semicolon after catch statement","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-17T02:17:53Z","receivedAt":"2022-10-17T02:18:01Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"> Curiously this works as expected for me, both before and after your\n> patch. I wonder if it depends on perl version. Mine is 5.34.\n\nHm, curious indeed! It reliably fails without my patch and passes with it on\nall the versions I had lying around (5.8, 5.18, 5.24, 5.26, 5.28, 5.30, 5.34,\nand 5.36).\n\n> I've never used Error.pm's try/catch before, so I don't know what's\n> normal. Regular if/unless doesn't need it, but certainly an earlier\n> catch uses a semicolon. So it seems like a reasonable fix.\n\nHa, well Perl is...let's say special. try/catch is not a language construct\n(until 5.34, where it is experimental), and so it's always implemented by\nsubroutines. One upshot of this is that try/catch needs a semicolon, because\nit's sugar for try(sub { ... }), and statements need semicolons separating\nthem. \n\nCompare these two examples: -MO=Deparse,-p tells perl to deparse the program\nand print it with parentheses:\n\n    $ perl -MError -MO=Deparse,-p -e 'try { die \"bad\" } catch Pkg with { warn \"caught\" }; print \"after\"'\n    do {\n        die('bad')\n    }->try('Pkg'->catch(do {\n        warn('caught')\n    }->with));\n    print('after');\n    -e syntax OK\n\n    $ perl -MError -MO=Deparse,-p -e 'try { die \"bad\" } catch Pkg with { warn \"caught\" } print \"after\"'\n    do {\n        die('bad')\n    }->try('Pkg'->catch(do {\n        warn('caught')\n    }->with(print('after'))));\n    -e syntax OK\n\nThe first here is the good case case (with semicolon), and you can see that\nprint('after') is its own statement. The bad case, with no semicolon, passes\nit as an argument to with(), which is what's causing the error here: something\ncalled by with() is trying to use it as a hash reference, and it's a string.\n\n> I'd assume t9700 passes for you, since I don't think we cover this case.\n> Maybe it's worth squashing this in:\n> \n> diff --git a/t/t9700/test.pl b/t/t9700/test.pl\n> index e046f7db76..5bd3687f37 100755\n> --- a/t/t9700/test.pl\n> +++ b/t/t9700/test.pl\n> @@ -30,6 +30,12 @@ sub adjust_dirsep {\n>  # set up\n>  our $abs_repo_dir = cwd();\n>  ok(our $r = Git->repository(Directory => \".\"), \"open repository\");\n> +{\n> +\tlocal $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n> +\tmy $failed = eval { Git->repository(Directory => \".\") };\n> +\tok(!$failed, \"reject unsafe repository\");\n> +\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n> +}\n>  \n>  # config\n>  is($r->config(\"test.string\"), \"value\", \"config scalar: string\");\n\nCuriously, t9700 passes for me with this suggestion both with and without my\npatch. You'd only see this bug in bare repositories, though, and the one set\nup in t9700 is not bare. I can see about trying to make it do so, but I'll\nneed to do a bit more reading of how even the tests are set up and run first.\nThanks!\n\n-- \nMichael McClimon\nmichael@mcclimon.org\n"},{"id":"465117","messageId":"Y02SHlW1rNQdfCHI@coredump.intra.peff.net","threadId":"58633","inReplyTo":"Y0y7UdLf3qd7RgVQ@newk","subject":"Re: [PATCH 1/1] Git.pm: add semicolon after catch statement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-17T17:34:22Z","receivedAt":"2022-10-17T17:34:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc people who worked on safe-directory stuff; please check out the\nincluded test and final comments]\n\nOn Sun, Oct 16, 2022 at 10:17:53PM -0400, Michael McClimon wrote:\n\n> > Curiously this works as expected for me, both before and after your\n> > patch. I wonder if it depends on perl version. Mine is 5.34.\n> \n> Hm, curious indeed! It reliably fails without my patch and passes with it on\n> all the versions I had lying around (5.8, 5.18, 5.24, 5.26, 5.28, 5.30, 5.34,\n> and 5.36).\n\nDoh, sorry to mislead you; I hadn't noticed this was in the bare\nrepository code path until you pointed it out. I get the same outcome as\nyou and the OP once that is fixed (both in t9700 and in my manual\ntesting).\n\n> Ha, well Perl is...let's say special. try/catch is not a language construct\n> (until 5.34, where it is experimental), and so it's always implemented by\n> subroutines. One upshot of this is that try/catch needs a semicolon, because\n> it's sugar for try(sub { ... }), and statements need semicolons separating\n> them.\n\nRight, I imagined it was something like that. Your fix is definitely the\nright thing, then.\n\n> Curiously, t9700 passes for me with this suggestion both with and without my\n> patch. You'd only see this bug in bare repositories, though, and the one set\n> up in t9700 is not bare. I can see about trying to make it do so, but I'll\n> need to do a bit more reading of how even the tests are set up and run first.\n\nYeah, this test is particularly confusing because unlike most of our\nsuite, it drives the test harness using a separate perl script. So you\nhave setup in one file and tests in another.\n\nYou'd want something like:\n\ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex 4aa5d90d32..53a838a8e8 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -45,6 +45,10 @@ test_expect_success \\\n      git config --add test.pathmulti bar\n      '\n \n+test_expect_success 'set up bare repository' '\n+        git init --bare bare.git\n+'\n+\n test_expect_success 'use t9700/test.pl to test Git.pm' '\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY\"/t9700/test.pl 2>stderr &&\n \ttest_must_be_empty stderr\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex e046f7db76..917b09cdf9 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -30,6 +30,12 @@ sub adjust_dirsep {\n # set up\n our $abs_repo_dir = cwd();\n ok(our $r = Git->repository(Directory => \".\"), \"open repository\");\n+{\n+\tlocal $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n+\tmy $failed = eval { Git->repository(Directory => \"$abs_repo_dir/bare.git\") };\n+\tok(!$failed, \"reject unsafe bare repository\");\n+\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n+}\n \n # config\n is($r->config(\"test.string\"), \"value\", \"config scalar: string\");\n\nBut curiously this still does not pass after your patch, because we seem\nto actually open the repository! I think this is because the C code\nallows an explicit GIT_DIR to override the safe-directory checks. But in\nthis case that GIT_DIR is set by Git.pm searching for the directory, not\nbecause the user desires it.\n\n(I know that I used a \"Directory\" in the example above, but that is only\nto avoid extra chdir-ing around in the test script; calling a bare\nGit->repository() triggers the same behavior in the right directory).\n\nSo your patch is definitely still the right thing to do, but it feels\nlike a hole in the safe-directory mechanism, at least when called via\nGit.pm. +cc folks who worked on that.\n\n-Peff\n"},{"id":"465163","messageId":"Y04D66rlhYhP99RU@newk","threadId":"58633","inReplyTo":"Y02SHlW1rNQdfCHI@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] Git.pm: add semicolon after catch statement","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-18T01:39:55Z","receivedAt":"2022-10-18T01:40:06Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"> Yeah, this test is particularly confusing because unlike most of our\n> suite, it drives the test harness using a separate perl script. So you\n> have setup in one file and tests in another.\n\nOh good, I'm glad it wasn't just me; this was very helpful, thanks.\n\n> \n> But curiously this still does not pass after your patch, because we seem\n> to actually open the repository! I think this is because the C code\n> allows an explicit GIT_DIR to override the safe-directory checks. But in\n> this case that GIT_DIR is set by Git.pm searching for the directory, not\n> because the user desires it.\n\nAha; this gets to what I was saying in the cover letter, which is that my\npatch only fixes the runtime error from perl, and Git.pm happily carries on in\nan unsafe directory. Fixing _that_ is probably beyond my knowledge!\n\n> So your patch is definitely still the right thing to do, but it feels\n> like a hole in the safe-directory mechanism, at least when called via\n> Git.pm. +cc folks who worked on that.\n\nIf we wanted a test for just the runtime error, something like the following\n(including your earlier suggestion to set up the bare repo) demonstrates the\nbug and my fix. It doesn't seem like a particularly valuable test on its own,\nIMO, but I'm happy to reroll if we want it. Thanks!\n\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex e046f7db..72681849 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -30,6 +30,11 @@ sub adjust_dirsep {\n # set up\n our $abs_repo_dir = cwd();\n ok(our $r = Git->repository(Directory => \".\"), \"open repository\");\n+{\n+       local $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n+       eval { Git->repository(Directory => \"$abs_repo_dir/bare.git\") };\n+       unlike($@, qr/as a HASH ref/i, \"no error from perl\");\n+}\n\n # config\n is($r->config(\"test.string\"), \"value\", \"config scalar: string\");\n\n-- \nMichael McClimon\nmichael@mcclimon.org\n"},{"id":"465559","messageId":"20221022011931.43992-1-michael@mcclimon.org","threadId":"58633","inReplyTo":"20221016212236.12453-1-michael@mcclimon.org","subject":"[PATCH v2 0/2] Fix behavior of Git.pm in unsafe bare repositories","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-22T01:19:29Z","receivedAt":"2022-10-22T01:19:54Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"This adds one commit on top of the last version, to avoid the security problem\nmentioned by Peff at [1]. It's possible this isn't the best way to go about\nthis (and I would not really call myself a C programmer), but it's a minimal\npatch that seems to fix the problem.\n\nI'm not sure if I need to document the new environment variable somewhere or\nnot; it's really an internal-only thing, so I didn't do so this time around,\nbut happy to if needed. Thanks!\n\n[1] https://lore.kernel.org/git/Y1ImS1Muvk1MAQeC@coredump.intra.peff.net/\n\nMichael McClimon (2):\n  Git.pm: add semicolon after catch statement\n  setup: allow Git.pm to do unsafe repo checking\n\n perl/Git.pm         |  3 ++-\n setup.c             |  3 +++\n t/t9700-perl-git.sh |  4 ++++\n t/t9700/test.pl     | 18 ++++++++++++++++++\n 4 files changed, 27 insertions(+), 1 deletion(-)\n\nRange-diff against v1:\n1:  1337c855 = 1:  1337c855 Git.pm: add semicolon after catch statement\n-:  -------- > 2:  273f77d1 setup: allow Git.pm to do unsafe repo checking\n-- \n2.38.1.130.g45c9f05c\n\n"},{"id":"465560","messageId":"20221022011931.43992-2-michael@mcclimon.org","threadId":"58633","inReplyTo":"20221022011931.43992-1-michael@mcclimon.org","subject":"[PATCH v2 1/2] Git.pm: add semicolon after catch statement","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-22T01:19:31Z","receivedAt":"2022-10-22T01:20:14Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"When attempting to initialize a repository object in an unsafe\ndirectory, a syntax error is reported (Can't use string as a HASH ref\nwhile strict refs in use). Fix this runtime error by adding the required\nsemicolon after the catch statement.\n\nWithout the semicolon, the result of the following line (i.e., the\nresult of Cwd::abs_path) is passed as the third argument to Error.pm's\ncatch function. That function expects that its third argument,\n$clauses, is a hash reference, and trying to access a string as a hash\nreference is a fatal error.\n\n[1] https://lore.kernel.org/git/20221011182607.f1113fff-9333-427d-ba45-741a78fa6040@korelogic.com/\n\nReported-by: Hank Leininger <hlein@korelogic.com>\nSigned-off-by: Michael McClimon <michael@mcclimon.org>\n---\n perl/Git.pm | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 080cdc2a..cf15ead6 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -217,7 +217,7 @@ sub repository {\n \t\t\t} catch Git::Error::Command with {\n \t\t\t\t# Mimic git-rev-parse --git-dir error message:\n \t\t\t\tthrow Error::Simple(\"fatal: Not a git repository: $dir\");\n-\t\t\t}\n+\t\t\t};\n \n \t\t\t$opts{Repository} = Cwd::abs_path($dir);\n \t\t}\n-- \n2.38.1.130.g45c9f05c\n\n"},{"id":"465561","messageId":"20221022011931.43992-3-michael@mcclimon.org","threadId":"58633","inReplyTo":"20221022011931.43992-1-michael@mcclimon.org","subject":"[PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-22T01:19:32Z","receivedAt":"2022-10-22T01:20:28Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"The previous commit exposes a security flaw: it is possible to bypass\nunsafe repository checks by using Git.pm, where before the syntax error\naccidentally prohibited it. This problem occurs because Git.pm sets\nGIT_DIR explicitly, which bypasses the safe repository checks.\n\nFix this by introducing a new environment variable,\nGIT_PERL_FORCE_OWNERSHIP_CHECK, which we set true in Git.pm. In setup.c,\nif that environment variable is true, force ownership checks even if\nan explicit GIT_DIR is provided.\n\nSigned-off-by: Michael McClimon <michael@mcclimon.org>\n---\n perl/Git.pm         |  1 +\n setup.c             |  3 +++\n t/t9700-perl-git.sh |  4 ++++\n t/t9700/test.pl     | 18 ++++++++++++++++++\n 4 files changed, 26 insertions(+)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex cf15ead6..002c29bb 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1674,6 +1674,7 @@ sub _cmd_exec {\n sub _setup_git_cmd_env {\n \tmy $self = shift;\n \tif ($self) {\n+\t\t$ENV{GIT_PERL_FORCE_OWNERSHIP_CHECK} = 1;\n \t\t$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();\n \t\t$self->repo_path() and $self->wc_path()\n \t\t\tand $ENV{'GIT_WORK_TREE'} = $self->wc_path();\ndiff --git a/setup.c b/setup.c\nindex cefd5f63..33d4e6fd 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1250,6 +1250,9 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n \t */\n \tgitdirenv = getenv(GIT_DIR_ENVIRONMENT);\n \tif (gitdirenv) {\n+\t\tif (git_env_bool(\"GIT_PERL_FORCE_OWNERSHIP_CHECK\", 0) &&\n+\t\t    !ensure_valid_ownership(NULL, NULL, gitdirenv, report))\n+\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n \t\tstrbuf_addstr(gitdir, gitdirenv);\n \t\treturn GIT_DIR_EXPLICIT;\n \t}\ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex 4aa5d90d..b14a40b1 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -45,6 +45,10 @@ test_expect_success \\\n      git config --add test.pathmulti bar\n      '\n \n+test_expect_success 'set up bare repository' '\n+     git init --bare bare.git\n+'\n+\n test_expect_success 'use t9700/test.pl to test Git.pm' '\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY\"/t9700/test.pl 2>stderr &&\n \ttest_must_be_empty stderr\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex e046f7db..1c91019f 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -142,6 +142,24 @@ sub adjust_dirsep {\n \t\t     \"abc\\\"\\\\ \\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x01 \",\n \t\t     'unquote escape sequences');\n \n+# safe directory\n+{\n+\tlocal $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n+\t# Save stderr to a tempfile so we can check the contents\n+\topen our $tmpstderr2, \">&STDERR\" or die \"cannot save STDERR\";\n+\tmy $tmperr = \"unsafeerr.tmp\";\n+\topen STDERR, \">\", \"$tmperr\" or die \"cannot redirect STDERR to $tmperr\";\n+\tmy $failed = eval { Git->repository(Directory => \"$abs_repo_dir/bare.git\") };\n+\tok(!$failed, \"reject unsafe repository\");\n+\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n+\topen TEMPFILE, \"<\", \"$tmperr\" or die \"Can't open $tmperr $!\";\n+\tmy $errcontents;\n+\t{ local $/; $errcontents = <TEMPFILE>; }\n+\tlike($errcontents, qr/dubious ownership/, \"dubious ownership message\");\n+\tclose STDERR or die \"cannot close temp stderr\";\n+\topen STDERR, \">&\", $tmpstderr2 or die \"cannot restore STDERR\";\n+}\n+\n printf \"1..%d\\n\", Test::More->builder->current_test;\n \n my $is_passing = eval { Test::More->is_passing };\n-- \n2.38.1.130.g45c9f05c\n\n"},{"id":"465562","messageId":"xmqq1qr0ifya.fsf@gitster.g","threadId":"58633","inReplyTo":"20221022011931.43992-3-michael@mcclimon.org","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-22T05:29:33Z","receivedAt":"2022-10-22T05:29:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael McClimon <michael@mcclimon.org> writes:\n\n> The previous commit exposes a security flaw: it is possible to bypass\n> unsafe repository checks by using Git.pm, where before the syntax error\n> accidentally prohibited it. This problem occurs because Git.pm sets\n> GIT_DIR explicitly, which bypasses the safe repository checks.\n>\n> Fix this by introducing a new environment variable,\n> GIT_PERL_FORCE_OWNERSHIP_CHECK, which we set true in Git.pm. In setup.c,\n> if that environment variable is true, force ownership checks even if\n> an explicit GIT_DIR is provided.\n\nDrop \"PERL_\" from the name, as any third-party tool that does its\nown repository discovery and sets GIT_DIR before it invokes git\nwould need a similar \"Yes, I am setting GIT_DIR, but I still want\nyou to check if that passes the usual ownership check\", and there\nis no reason to expect that these tools are always written in Perl.\n\nHow about \"GIT_SAFE_DIRECTORY_STRICT\" or something along that line,\ninstead?\n\nI also have to wonder (and this is *not* a suggestion for inventing\nan alternative fix for perl/Git.pm) if we were creating perl/Git.pm\nfrom scratch today, we even need to be worried about this issue.  We\nmay have Git::repo_path() helper but if we call it in a natural way\n(read: as if an interactive end-user would type commands), it is\nlikely that we would run \"git rev-parse --git-dir\" or something\nwithout setting GIT_DIR, and when we need to run \"git\" command, say\n\"git diff\", we would also run \"git diff\" as if the end user would\ntype from within their interactive session and without setting\nGIT_DIR, and everything should work.  IOW, why do we even setting\nand exporting the auto-detected value in GIT_DIR?\n\nAlso, if the end user had GIT_DIR in the environment _before_\ncalling something that uses \"import Git\", what should happen?  In\nthat case, the end-user is who is telling us that the named\ndirectory is OK without any forced ownership check, so it feels\nWRONG that this patch UNCONDITIONALLY exports FORCE_CHECK.  Only\nwhen we did auto-discovery via Git::repo_path() without end-user\nsupplied GIT_DIR and exported GIT_DIR, we should also export\nFORCE_CHECK to tell the underlying \"git\" that our auto-detection\nmay be flawed and it needs to double check, no?\n\n> Signed-off-by: Michael McClimon <michael@mcclimon.org>\n> ---\n>  perl/Git.pm         |  1 +\n>  setup.c             |  3 +++\n>  t/t9700-perl-git.sh |  4 ++++\n>  t/t9700/test.pl     | 18 ++++++++++++++++++\n>  4 files changed, 26 insertions(+)\n>\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index cf15ead6..002c29bb 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -1674,6 +1674,7 @@ sub _cmd_exec {\n>  sub _setup_git_cmd_env {\n>  \tmy $self = shift;\n>  \tif ($self) {\n> +\t\t$ENV{GIT_PERL_FORCE_OWNERSHIP_CHECK} = 1;\n>  \t\t$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();\n>  \t\t$self->repo_path() and $self->wc_path()\n>  \t\t\tand $ENV{'GIT_WORK_TREE'} = $self->wc_path();\n> diff --git a/setup.c b/setup.c\n> index cefd5f63..33d4e6fd 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1250,6 +1250,9 @@ static enum discovery_result setup_git_directory_gently_1(struct strbuf *dir,\n>  \t */\n>  \tgitdirenv = getenv(GIT_DIR_ENVIRONMENT);\n>  \tif (gitdirenv) {\n> +\t\tif (git_env_bool(\"GIT_PERL_FORCE_OWNERSHIP_CHECK\", 0) &&\n> +\t\t    !ensure_valid_ownership(NULL, NULL, gitdirenv, report))\n> +\t\t\treturn GIT_DIR_INVALID_OWNERSHIP;\n>  \t\tstrbuf_addstr(gitdir, gitdirenv);\n>  \t\treturn GIT_DIR_EXPLICIT;\n>  \t}\n> diff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\n> index 4aa5d90d..b14a40b1 100755\n> --- a/t/t9700-perl-git.sh\n> +++ b/t/t9700-perl-git.sh\n> @@ -45,6 +45,10 @@ test_expect_success \\\n>       git config --add test.pathmulti bar\n>       '\n>  \n> +test_expect_success 'set up bare repository' '\n> +     git init --bare bare.git\n> +'\n> +\n>  test_expect_success 'use t9700/test.pl to test Git.pm' '\n>  \t\"$PERL_PATH\" \"$TEST_DIRECTORY\"/t9700/test.pl 2>stderr &&\n>  \ttest_must_be_empty stderr\n> diff --git a/t/t9700/test.pl b/t/t9700/test.pl\n> index e046f7db..1c91019f 100755\n> --- a/t/t9700/test.pl\n> +++ b/t/t9700/test.pl\n> @@ -142,6 +142,24 @@ sub adjust_dirsep {\n>  \t\t     \"abc\\\"\\\\ \\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x01 \",\n>  \t\t     'unquote escape sequences');\n>  \n> +# safe directory\n> +{\n> +\tlocal $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n> +\t# Save stderr to a tempfile so we can check the contents\n> +\topen our $tmpstderr2, \">&STDERR\" or die \"cannot save STDERR\";\n> +\tmy $tmperr = \"unsafeerr.tmp\";\n> +\topen STDERR, \">\", \"$tmperr\" or die \"cannot redirect STDERR to $tmperr\";\n> +\tmy $failed = eval { Git->repository(Directory => \"$abs_repo_dir/bare.git\") };\n> +\tok(!$failed, \"reject unsafe repository\");\n> +\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n> +\topen TEMPFILE, \"<\", \"$tmperr\" or die \"Can't open $tmperr $!\";\n> +\tmy $errcontents;\n> +\t{ local $/; $errcontents = <TEMPFILE>; }\n> +\tlike($errcontents, qr/dubious ownership/, \"dubious ownership message\");\n> +\tclose STDERR or die \"cannot close temp stderr\";\n> +\topen STDERR, \">&\", $tmpstderr2 or die \"cannot restore STDERR\";\n> +}\n> +\n>  printf \"1..%d\\n\", Test::More->builder->current_test;\n>  \n>  my $is_passing = eval { Test::More->is_passing };\n"},{"id":"465574","messageId":"221022.86eduzeiek.gmgdl@evledraar.gmail.com","threadId":"58633","inReplyTo":"20221022011931.43992-3-michael@mcclimon.org","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-22T19:45:14Z","receivedAt":"2022-10-22T20:02:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Oct 21 2022, Michael McClimon wrote:\n\n> The previous commit exposes a security flaw: it is possible to bypass\n> unsafe repository checks by using Git.pm, where before the syntax error\n> accidentally prohibited it. This problem occurs because Git.pm sets\n> GIT_DIR explicitly, which bypasses the safe repository checks.\n>\n> Fix this by introducing a new environment variable,\n> GIT_PERL_FORCE_OWNERSHIP_CHECK, which we set true in Git.pm. In setup.c,\n> if that environment variable is true, force ownership checks even if\n> an explicit GIT_DIR is provided.\n\nThe 1/2 is unambiguously good, thanks.\n\nAs for this step, I think it's good if we want to go in this direction.\n\nAs to whether it's the direction we want...\n\nThe vulnerability safe.directory was supposed to address was one where\nyou'd set your fsmonitor hook via a config variable, so running \"diff\",\n\"status\" etc. would unexpectedly execute arbitrary code.\n\nEspecially on Windows where apparently the equivalent of the root of a\nshared mounted volume routinely has global write permissions (user's\nsubdirectories being less permissive).\n\nAn alternative I raised on the security list was to narrowly target just\nthe fsmonitor config, since that was the vulnerability.\n\nBut it was decided to cast a wider net, as it wasn't disclosed yet, and\nthe list members might have missed some other config variable that would\nallow shenanigans, fair enough, especially for a time constrained\nsecurity fix.\n\nNow that the cat's thoroughly out of the bag I think we'd do well to\nreconsider that.\n\nI'm unaware of any other variable(s) that provide a similar vector, and\nsafe.directory is encouraging users (especially in core.sharedRepository\nsettings) to mark a dir as \"safe\", and we'd then later have an exploit\nfrom a user with rw access who'd use the fsmonitor hook vector.\n\nBetter have them \"safe by default\", and start refusing to read the\nconfig when we detect that fsmonitor setting, and insist the user mark\n*that* safe.\n\nAnyway, that's all on the general topic, and the above is just my\nopinion on it.\n\nBut on this much more narrow topic: If we couldn't come up with an issue\nother than the fsmonitor config for git's C codebase, I think extending\nthe same mitigation to the very small part of git that's still in Perl\nis thoroughly into the tail wagging the dog territory.\n\nI.e. what we'd presumably get out of this is mitigation against some\nexploit via a variable that git-send-email or git-svn use (or the\nhandful of other more obscure Perl tooling we have).\n\nCan we think of one that could be an issue, and if not is the current\nbehavior in 1/2 maybe OK as-is?\n\n>  test_expect_success 'use t9700/test.pl to test Git.pm' '\n>  \t\"$PERL_PATH\" \"$TEST_DIRECTORY\"/t9700/test.pl 2>stderr &&\n>  \ttest_must_be_empty stderr\n> diff --git a/t/t9700/test.pl b/t/t9700/test.pl\n> index e046f7db..1c91019f 100755\n> --- a/t/t9700/test.pl\n> +++ b/t/t9700/test.pl\n> @@ -142,6 +142,24 @@ sub adjust_dirsep {\n>  \t\t     \"abc\\\"\\\\ \\x07\\x08\\x09\\x0a\\x0b\\x0c\\x0d\\x01 \",\n>  \t\t     'unquote escape sequences');\n>  \n> +# safe directory\n> +{\n> +\tlocal $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n> +\t# Save stderr to a tempfile so we can check the contents\n> +\topen our $tmpstderr2, \">&STDERR\" or die \"cannot save STDERR\";\n> +\tmy $tmperr = \"unsafeerr.tmp\";\n> +\topen STDERR, \">\", \"$tmperr\" or die \"cannot redirect STDERR to $tmperr\";\n> +\tmy $failed = eval { Git->repository(Directory => \"$abs_repo_dir/bare.git\") };\n> +\tok(!$failed, \"reject unsafe repository\");\n> +\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n> +\topen TEMPFILE, \"<\", \"$tmperr\" or die \"Can't open $tmperr $!\";\n> +\tmy $errcontents;\n> +\t{ local $/; $errcontents = <TEMPFILE>; }\n> +\tlike($errcontents, qr/dubious ownership/, \"dubious ownership message\");\n> +\tclose STDERR or die \"cannot close temp stderr\";\n> +\topen STDERR, \">&\", $tmpstderr2 or die \"cannot restore STDERR\";\n> +}\n\nThis whole \"save stderr to a file\" etc. seems much more complex than\njust writing the same test in our normal *.sh tests, and doing something\nlike:\n\n\n\t! GIT_TEST_ASSUME_DIFFERENT_OWNER=1 perl -MGit -we 'Git->repository(Directory => shift)' 2>expect &&\n\tgrep \"reject unusafe\" ...\n\nI.e. you're spending a lot of effort on capturing STDERR within a single\nPerl process, then restoring it etc., when we can just run one-off\ncommand and have it die (no eval), and just inspect the exit code &\nstderr perl emitted.\n\n\n"},{"id":"465577","messageId":"Y1RY38RULkqd9pBN@coredump.intra.peff.net","threadId":"58633","inReplyTo":"221022.86eduzeiek.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T20:55:59Z","receivedAt":"2022-10-22T20:59:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 22, 2022 at 09:45:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The vulnerability safe.directory was supposed to address was one where\n> you'd set your fsmonitor hook via a config variable, so running \"diff\",\n> \"status\" etc. would unexpectedly execute arbitrary code.\n> \n> Especially on Windows where apparently the equivalent of the root of a\n> shared mounted volume routinely has global write permissions (user's\n> subdirectories being less permissive).\n> \n> An alternative I raised on the security list was to narrowly target just\n> the fsmonitor config, since that was the vulnerability.\n>\n> [...]\n>\n> I'm unaware of any other variable(s) that provide a similar vector, and\n> safe.directory is encouraging users (especially in core.sharedRepository\n> settings) to mark a dir as \"safe\", and we'd then later have an exploit\n> from a user with rw access who'd use the fsmonitor hook vector.\n\nHere are a few off the top of my head that you can trigger via git-diff:\n\n  - core.pager will run an arbitrary program\n\n  - pager.diff will run an arbitrary program\n\n  - diff.*.textconv runs an arbitrary program; you need matching\n    .gitattributes, but those are under the control of the repository.\n    (not diff.*.command, though, as you need to pass --ext-diff)\n\n  - browser/man paths if you run \"git diff --help\"\n\nAnd of course as you expand the set of commands there are more options.\nE.g., credential helper commands if you do anything that wants auth,\ninteractive diff-filter for \"add -p\", hooks for git-commit, git-push,\netc. Those commands are less likely to be run in an untrusted repository\nthan inspection commands like \"status\" or \"diff\", but the boundary is\ngetting quite fuzzy.\n\nfsmonitor _might_ be the only one that is triggered by git-prompt.sh,\nbecause it has a limited command palette, generally reads (or sends to\n/dev/null) the stdout of commands (preventing pager invocation), and\ndoesn't do text diffs (so no textconv). Even if true, I'm not sure if\nthat's a good place to draw the line, though. People do tend to run \"git\nlog\" themselves.\n\n-Peff\n"},{"id":"465579","messageId":"Y1Rdtog/XQV0YLj0@coredump.intra.peff.net","threadId":"58633","inReplyTo":"20221022011931.43992-3-michael@mcclimon.org","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T21:16:38Z","receivedAt":"2022-10-22T21:16:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 09:19:32PM -0400, Michael McClimon wrote:\n\n> diff --git a/perl/Git.pm b/perl/Git.pm\n> index cf15ead6..002c29bb 100644\n> --- a/perl/Git.pm\n> +++ b/perl/Git.pm\n> @@ -1674,6 +1674,7 @@ sub _cmd_exec {\n>  sub _setup_git_cmd_env {\n>  \tmy $self = shift;\n>  \tif ($self) {\n> +\t\t$ENV{GIT_PERL_FORCE_OWNERSHIP_CHECK} = 1;\n>  \t\t$self->repo_path() and $ENV{'GIT_DIR'} = $self->repo_path();\n>  \t\t$self->repo_path() and $self->wc_path()\n>  \t\t\tand $ENV{'GIT_WORK_TREE'} = $self->wc_path();\n\nI'm not familiar enough with Git.pm to know if this is the right spot.\nBut we'd not want to break the case where GIT_DIR is set already. I.e.:\n\n  GIT_DIR=/path/to/repo.git perl -MGit -e 'Git->repository'\n\nshould continue to work regardless of the ownership of repo.git. Only\nthe repo-discovery phase would want to force the ownership check.\n\nAgain, I'm not too familiar with Git.pm, but it seems it ought to be\nasking Git: are we in a valid Git repo, and if so where is it? Something\nlike:\n\n  my $git_dir = `git rev-parse --absolute-git-dir`;\n  $? and die \"nope, not in a git repo\";\n\n  # later, when we run git commands, we do specify this; the script may\n  # have chdir()'d in the meantime, and we want to make sure we are\n  # referring to the same repo via the object.\n  local $ENV{GIT_DIR} = abs_path($git_dir);\n  ...run some git command...\n\nLooking at the code, we even seem to do that first part! But if it\nreturns an error, then we go on to check for a bare repository\nourselves by looking for refs/, objects/, etc. Which is just...weird.\n\nIt feels like this try/catch should just go away:\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex cf15ead664..7a7d8a2987 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -177,13 +177,7 @@ sub repository {\n \t\t-d $opts{Directory} or throw Error::Simple(\"Directory not found: $opts{Directory} $!\");\n \n \t\tmy $search = Git->repository(WorkingCopy => $opts{Directory});\n-\t\tmy $dir;\n-\t\ttry {\n-\t\t\t$dir = $search->command_oneline(['rev-parse', '--git-dir'],\n-\t\t\t                                STDERR => 0);\n-\t\t} catch Git::Error::Command with {\n-\t\t\t$dir = undef;\n-\t\t};\n+\t\tmy $dir = $search->command_oneline(['rev-parse', '--git-dir']);\n \n \t\trequire Cwd;\n \t\tif ($dir) {\n\nAnd then the code below that to check for bare/not-bare should be using\n\"git rev-parse --is-bare-repository\" or similar. Something like:\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 7a7d8a2987..280df9cee1 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -179,8 +179,14 @@ sub repository {\n \t\tmy $search = Git->repository(WorkingCopy => $opts{Directory});\n \t\tmy $dir = $search->command_oneline(['rev-parse', '--git-dir']);\n \n+\t\t# could be merged with command above to be more efficient; or\n+\t\t# could probably use --show-toplevel to avoid prefix query\n+\t\t# below\n+\t\tmy $bare = $search->command_oneline(['rev-parse', '--is-bare-repository'])\n+\t\t             eq 'true';\n+\n \t\trequire Cwd;\n-\t\tif ($dir) {\n+\t\tif (!$bare) {\n \t\t\trequire File::Spec;\n \t\t\tFile::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;\n \t\t\t$opts{Repository} = Cwd::abs_path($dir);\n@@ -198,21 +204,6 @@ sub repository {\n \t\t\t$opts{WorkingSubdir} = $prefix;\n \n \t\t} else {\n-\t\t\t# A bare repository? Let's see...\n-\t\t\t$dir = $opts{Directory};\n-\n-\t\t\tunless (-d \"$dir/refs\" and -d \"$dir/objects\" and -e \"$dir/HEAD\") {\n-\t\t\t\t# Mimic git-rev-parse --git-dir error message:\n-\t\t\t\tthrow Error::Simple(\"fatal: Not a git repository: $dir\");\n-\t\t\t}\n-\t\t\tmy $search = Git->repository(Repository => $dir);\n-\t\t\ttry {\n-\t\t\t\t$search->command('symbolic-ref', 'HEAD');\n-\t\t\t} catch Git::Error::Command with {\n-\t\t\t\t# Mimic git-rev-parse --git-dir error message:\n-\t\t\t\tthrow Error::Simple(\"fatal: Not a git repository: $dir\");\n-\t\t\t};\n-\n \t\t\t$opts{Repository} = Cwd::abs_path($dir);\n \t\t}\n \n\nBut given how much more complicated the current code is, I wonder if I\nam missing some case. Or perhaps this code is just so old that it used\nto do this stuff itself (because rev-parse didn't give us so much help).\n\n-Peff\n"},{"id":"465580","messageId":"Y1ReKB701QXRv64A@coredump.intra.peff.net","threadId":"58633","inReplyTo":"xmqq1qr0ifya.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T21:18:32Z","receivedAt":"2022-10-22T21:18:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 10:29:33PM -0700, Junio C Hamano wrote:\n\n> I also have to wonder (and this is *not* a suggestion for inventing\n> an alternative fix for perl/Git.pm) if we were creating perl/Git.pm\n> from scratch today, we even need to be worried about this issue.  We\n> may have Git::repo_path() helper but if we call it in a natural way\n> (read: as if an interactive end-user would type commands), it is\n> likely that we would run \"git rev-parse --git-dir\" or something\n> without setting GIT_DIR, and when we need to run \"git\" command, say\n> \"git diff\", we would also run \"git diff\" as if the end user would\n> type from within their interactive session and without setting\n> GIT_DIR, and everything should work.  IOW, why do we even setting\n> and exporting the auto-detected value in GIT_DIR?\n\nI think it has to in order to avoid surprises. If I do this:\n\n  perl -MGit -e '\n    my $r = Git->repository;\n    chdir(\"/somewhere/else\");\n    $r->git_command(...);\n  '\n\nthat command ought to run in the repository I opened earlier. So I think\nto keep the illusion of a lib-ified object, creating that object has to\nlock in the path.\n\nBut it really seems like we should be asking rev-parse what that path\nis, not trying to do any magic ourselves.\n\n-Peff\n"},{"id":"465581","messageId":"Y1Rp+7R7e+LFa5k6@coredump.intra.peff.net","threadId":"58633","inReplyTo":"Y1Rdtog/XQV0YLj0@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T22:08:59Z","receivedAt":"2022-10-22T22:09:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 22, 2022 at 05:16:38PM -0400, Jeff King wrote:\n\n> Again, I'm not too familiar with Git.pm, but it seems it ought to be\n> asking Git: are we in a valid Git repo, and if so where is it? Something\n> like:\n> \n>   my $git_dir = `git rev-parse --absolute-git-dir`;\n>   $? and die \"nope, not in a git repo\";\n> \n>   # later, when we run git commands, we do specify this; the script may\n>   # have chdir()'d in the meantime, and we want to make sure we are\n>   # referring to the same repo via the object.\n>   local $ENV{GIT_DIR} = abs_path($git_dir);\n>   ...run some git command...\n> \n> Looking at the code, we even seem to do that first part! But if it\n> returns an error, then we go on to check for a bare repository\n> ourselves by looking for refs/, objects/, etc. Which is just...weird.\n> \n> It feels like this try/catch should just go away:\n\nIt's a little more complicated than that, because presumably people rely\non the error handling for a missing repo to not be noisy. So here's a\npolished version of what I showed, along with the tests we were\ndiscussing earlier.\n\nI prepared it on top of your fix in the mm/git-pm-try-catch-syntax-fix\nbranch. That's not strictly necessary, since my patch deletes the line\nyou fixed. :) But I think it's nicer to use your fix as the starting\npoint, since it means the test runs but produces the wrong behavior,\nrather than barfing with a syntax error.\n\n-- >8 --\nSubject: [PATCH] Git.pm: trust rev-parse to find bare repositories\n\nWhen initializing a repository object, we run \"git rev-parse --git-dir\"\nto let the C version of Git find the correct directory. But curiously,\nif this fails we don't automatically say \"not a git repository\".\nInstead, we do our own pure-perl check to see if we're in a bare\nrepository.\n\nThis makes little sense, as rev-parse will report both bare and non-bare\ndirectories. This logic comes from d5c7721d58 (Git.pm: Add support for\nsubdirectories inside of working copies, 2006-06-24), but I don't see\nany reason given why we can't just rely on rev-parse. Worse, because we\ntreat any non-error response from rev-parse as a non-bare repository,\nwe'll erroneously set the object's WorkingCopy, even in a bare\nrepository.\n\nBut it gets worse. Since 8959555cee (setup_git_directory(): add an owner\ncheck for the top-level directory, 2022-03-02), it's actively wrong (and\ndangerous). The perl code doesn't implement the same ownership checks.\nAnd worse, after \"finding\" the bare repository, it sets GIT_DIR in the\nenvironment, which tells any subsequent Git commands that we've\nconfirmed the directory is OK, and to trust us. I.e., it re-opens the\nvulnerability plugged by 8959555cee when using Git.pm's repository\ndiscovery code.\n\nWe can fix this by just relying on rev-parse to tell us when we're not\nin a repository, which fixes the vulnerability. Furthermore, we'll ask\nits --is-bare-repository function to tell us if we're bare or not, and\nrely on that.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI didn't dig into the \"oops, we set WorkingCopy\" thing beyond manually\nverifying that it happens. It doesn't look like its really used beyond\nthe wc_path() method, so it's not like it would have been breaking git\nsub-commands, etc. I guess we could add a test that wc_path() returns\nundef in a bare repository, though.\n\n perl/Git.pm         | 36 ++++++++++++++++--------------------\n t/t9700-perl-git.sh |  4 ++++\n t/t9700/test.pl     | 12 ++++++++++++\n 3 files changed, 32 insertions(+), 20 deletions(-)\n\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex cf15ead664..117765dc73 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -177,16 +177,27 @@ sub repository {\n \t\t-d $opts{Directory} or throw Error::Simple(\"Directory not found: $opts{Directory} $!\");\n \n \t\tmy $search = Git->repository(WorkingCopy => $opts{Directory});\n-\t\tmy $dir;\n+\n+\t\t# This rev-parse will throw an exception if we're not in a\n+\t\t# repository, which is what we want, but it's kind of noisy.\n+\t\t# Ideally we'd capture stderr and relay it, but doing so is\n+\t\t# awkward without depending on it fitting in a pipe buffer. So\n+\t\t# we just reproduce a plausible error message ourselves.\n+\t\tmy $out;\n \t\ttry {\n-\t\t\t$dir = $search->command_oneline(['rev-parse', '--git-dir'],\n-\t\t\t                                STDERR => 0);\n+\t\t  # Note that \"--is-bare-repository\" must come first, as\n+\t\t  # --git-dir output could contain newlines.\n+\t\t  $out = $search->command([qw(rev-parse --is-bare-repository --git-dir)],\n+\t\t\t                  STDERR => 0);\n \t\t} catch Git::Error::Command with {\n-\t\t\t$dir = undef;\n+\t\t\tthrow Error::Simple(\"fatal: not a git repository: $opts{Directory}\");\n \t\t};\n \n+\t\tchomp $out;\n+\t\tmy ($bare, $dir) = split /\\n/, $out, 2;\n+\n \t\trequire Cwd;\n-\t\tif ($dir) {\n+\t\tif ($bare ne 'true') {\n \t\t\trequire File::Spec;\n \t\t\tFile::Spec->file_name_is_absolute($dir) or $dir = $opts{Directory} . '/' . $dir;\n \t\t\t$opts{Repository} = Cwd::abs_path($dir);\n@@ -204,21 +215,6 @@ sub repository {\n \t\t\t$opts{WorkingSubdir} = $prefix;\n \n \t\t} else {\n-\t\t\t# A bare repository? Let's see...\n-\t\t\t$dir = $opts{Directory};\n-\n-\t\t\tunless (-d \"$dir/refs\" and -d \"$dir/objects\" and -e \"$dir/HEAD\") {\n-\t\t\t\t# Mimic git-rev-parse --git-dir error message:\n-\t\t\t\tthrow Error::Simple(\"fatal: Not a git repository: $dir\");\n-\t\t\t}\n-\t\t\tmy $search = Git->repository(Repository => $dir);\n-\t\t\ttry {\n-\t\t\t\t$search->command('symbolic-ref', 'HEAD');\n-\t\t\t} catch Git::Error::Command with {\n-\t\t\t\t# Mimic git-rev-parse --git-dir error message:\n-\t\t\t\tthrow Error::Simple(\"fatal: Not a git repository: $dir\");\n-\t\t\t};\n-\n \t\t\t$opts{Repository} = Cwd::abs_path($dir);\n \t\t}\n \ndiff --git a/t/t9700-perl-git.sh b/t/t9700-perl-git.sh\nindex 4aa5d90d32..b105d6d9d5 100755\n--- a/t/t9700-perl-git.sh\n+++ b/t/t9700-perl-git.sh\n@@ -45,6 +45,10 @@ test_expect_success \\\n      git config --add test.pathmulti bar\n      '\n \n+test_expect_success 'set up bare repository' '\n+\tgit init --bare bare.git\n+'\n+\n test_expect_success 'use t9700/test.pl to test Git.pm' '\n \t\"$PERL_PATH\" \"$TEST_DIRECTORY\"/t9700/test.pl 2>stderr &&\n \ttest_must_be_empty stderr\ndiff --git a/t/t9700/test.pl b/t/t9700/test.pl\nindex e046f7db76..6d753708d2 100755\n--- a/t/t9700/test.pl\n+++ b/t/t9700/test.pl\n@@ -30,6 +30,18 @@ sub adjust_dirsep {\n # set up\n our $abs_repo_dir = cwd();\n ok(our $r = Git->repository(Directory => \".\"), \"open repository\");\n+{\n+\tlocal $ENV{GIT_TEST_ASSUME_DIFFERENT_OWNER} = 1;\n+\tmy $failed;\n+\n+\t$failed = eval { Git->repository(Directory => $abs_repo_dir) };\n+\tok(!$failed, \"reject unsafe non-bare repository\");\n+\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n+\n+\t$failed = eval { Git->repository(Directory => \"$abs_repo_dir/bare.git\") };\n+\tok(!$failed, \"reject unsafe bare repository\");\n+\tlike($@, qr/not a git repository/i, \"unsafe error message\");\n+}\n \n # config\n is($r->config(\"test.string\"), \"value\", \"config scalar: string\");\n-- \n2.38.1.497.g093b959162\n\n"},{"id":"465587","messageId":"xmqq35bfh2o5.fsf@gitster.g","threadId":"58633","inReplyTo":"Y1Rdtog/XQV0YLj0@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-22T23:14:02Z","receivedAt":"2022-10-22T23:14:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It feels like this try/catch should just go away:\n> ...\n> And then the code below that to check for bare/not-bare should be using\n> \"git rev-parse --is-bare-repository\" or similar. Something like:\n\nYeah, exactly.  I very much like the straight-forward way of\nthinking to have \"git\" do the real work.\n\nThanks.\n\n\n\n\n"},{"id":"465588","messageId":"xmqqy1t7fnyg.fsf@gitster.g","threadId":"58633","inReplyTo":"Y1ReKB701QXRv64A@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-22T23:17:11Z","receivedAt":"2022-10-22T23:17:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think it has to in order to avoid surprises. If I do this:\n>\n>   perl -MGit -e '\n>     my $r = Git->repository;\n>     chdir(\"/somewhere/else\");\n>     $r->git_command(...);\n>   '\n>\n> that command ought to run in the repository I opened earlier. So I think\n> to keep the illusion of a lib-ified object, creating that object has to\n> lock in the path.\n>\n> But it really seems like we should be asking rev-parse what that path\n> is, not trying to do any magic ourselves.\n\nYeah, whichever caller doing the chdir() needs to take the\nresponsibility of adjusting the future use of git, e.g. going back\nto the original before spawning git or whatever.\n\nOr having the original Git->repository call bail out by having \"git\"\nfigure out where it is, while honoring safe.directory or any future\nprotection underlying \"git\" offers.\n\nThanks.\n\n"},{"id":"465589","messageId":"Y1R6ecMO+6X8tFlF@newk","threadId":"58633","inReplyTo":"Y1Rp+7R7e+LFa5k6@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Michael McClimon","fromEmail":"michael@mcclimon.org","sentAt":"2022-10-22T23:19:21Z","receivedAt":"2022-10-22T23:19:35Z","isPatch":true,"sender":{"key":"michael@mcclimon.org","avatar":"https://avatars.githubusercontent.com/u/1698723?v=4"},"body":"> I prepared it on top of your fix in the mm/git-pm-try-catch-syntax-fix\n> branch. That's not strictly necessary, since my patch deletes the line\n> you fixed. :) But I think it's nicer to use your fix as the starting\n> point, since it means the test runs but produces the wrong behavior,\n> rather than barfing with a syntax error.\n\nMy vanity thanks you for this, even if it's not strictly necessary. As a\nprofessional programmer with roughly no C chops and a long-time admirer of the\nGit project, all I _really_ wanted to do was to fix a thing that was in my\nwheelhouse so that I could say I have a commit in the history. (This isn't a\ngood reason on its own, of course, but I'm happy it was useful even if the\nline is immediately deleted!)\n\n> We can fix this by just relying on rev-parse to tell us when we're not\n> in a repository, which fixes the vulnerability. Furthermore, we'll ask\n> its --is-bare-repository function to tell us if we're bare or not, and\n> rely on that.\n\nYour suggested patch seems fine to me, and indeed I think if we were writing\nit today we'd just rely on rev-parse to do the heavy lifting. It looks like\nthe code in question -- and indeed, the syntax error in question --  blames to\nd5c7721d (Git.pm: Add support for subdirectories inside of working copies,\n2006-06-23), at which point rev-parse did not appear to have any special\nhandling for bare repositories.\n\n-- \nMichael McClimon\nmichael@mcclimon.org\n"},{"id":"465611","messageId":"221024.861qqxeah5.gmgdl@evledraar.gmail.com","threadId":"58633","inReplyTo":"Y1RY38RULkqd9pBN@coredump.intra.peff.net","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-24T10:57:29Z","receivedAt":"2022-10-24T11:18:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 22 2022, Jeff King wrote:\n\n> On Sat, Oct 22, 2022 at 09:45:14PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> The vulnerability safe.directory was supposed to address was one where\n>> you'd set your fsmonitor hook via a config variable, so running \"diff\",\n>> \"status\" etc. would unexpectedly execute arbitrary code.\n>> \n>> Especially on Windows where apparently the equivalent of the root of a\n>> shared mounted volume routinely has global write permissions (user's\n>> subdirectories being less permissive).\n>> \n>> An alternative I raised on the security list was to narrowly target just\n>> the fsmonitor config, since that was the vulnerability.\n>>\n>> [...]\n>>\n>> I'm unaware of any other variable(s) that provide a similar vector, and\n>> safe.directory is encouraging users (especially in core.sharedRepository\n>> settings) to mark a dir as \"safe\", and we'd then later have an exploit\n>> from a user with rw access who'd use the fsmonitor hook vector.\n>\n> Here are a few off the top of my head that you can trigger via git-diff:\n>\n>   - core.pager will run an arbitrary program\n>\n>   - pager.diff will run an arbitrary program\n>\n>   - diff.*.textconv runs an arbitrary program; you need matching\n>     .gitattributes, but those are under the control of the repository.\n>     (not diff.*.command, though, as you need to pass --ext-diff)\n>\n>   - browser/man paths if you run \"git diff --help\"\n>\n> And of course as you expand the set of commands there are more options.\n> E.g., credential helper commands if you do anything that wants auth,\n> interactive diff-filter for \"add -p\", hooks for git-commit, git-push,\n> etc. Those commands are less likely to be run in an untrusted repository\n> than inspection commands like \"status\" or \"diff\", but the boundary is\n> getting quite fuzzy.\n>\n> fsmonitor _might_ be the only one that is triggered by git-prompt.sh,\n> because it has a limited command palette, generally reads (or sends to\n> /dev/null) the stdout of commands (preventing pager invocation), and\n> doesn't do text diffs (so no textconv). Even if true, I'm not sure if\n> that's a good place to draw the line, though. People do tend to run \"git\n> log\" themselves.\n\nRight, by \"a similar\" vector I meant the unexpected execution of\nfsmonitor as there was software in the wild that was running \"status\"\nfor the user.\n\nThese are also a problem, but my understanding of that issue is that if\nit wasn't for the fsmonitor issue we'd have put that in the bucket of\nnot running arbitrary commands on a .git you just copied to somewhere,\ni.e. that issue was already known.\n\nThe difference being that users might have that implicit \"status\"\nrunning if they cd'd to /mnt/$USER/subdir and there was a hostile\n/mnt/.git, but would be much less likely to run \"git diff\" or whatever\nin such a subdir, unless they'd initialized a .git in say\n/mnt/$USER/subdir, at which point we'd ignore the /mnt/.git.\n\nAnyway, that's more into the \"would it have been a CVE?\" territory, so\nlet's leave that for now.\n\nThe important point/question I have is whether we can think of any such\nconfig variable understood by the code that uses Git.pm.\n\nThe only ones I can think are the \"sendemail.{to,cc}Cmd\" variables.\n\nI'm just pointing out that the reason we ended up with the facility (per\nmy understand) was among other things:\n\n * A. There were too many config variables to exhaustively audit on the\n   security list and be sure we caught them all, and ...\n * B. ...such a change would probably be larger, which ...\n * C. ...given the embargo & desire to keep security patches minimal\n   warranted the more general safe.directory approach.\n * D. You can talk about on the public list, or not have a zero-day, but\n   not both :)\n\nNow, we may come up with a reason \"E\" for extending this at this point,\ne.g. maybe just being consistent is reason enough.\n\nBut in this case \"A\" doesn't apply, it's maybe 20-30 config variables,\nand it's easy to skim the git-{send-email,svn} docs to see what they\nare. \"B\" might be the case, but taht's OK since we're past \"D\" here,\nditto \"C\" not applying.\n"},{"id":"465669","messageId":"Y1cgvTcMk6OT4axk@coredump.intra.peff.net","threadId":"58633","inReplyTo":"Y1R6ecMO+6X8tFlF@newk","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-24T23:33:17Z","receivedAt":"2022-10-25T00:52:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 22, 2022 at 07:19:21PM -0400, Michael McClimon wrote:\n\n> > I prepared it on top of your fix in the mm/git-pm-try-catch-syntax-fix\n> > branch. That's not strictly necessary, since my patch deletes the line\n> > you fixed. :) But I think it's nicer to use your fix as the starting\n> > point, since it means the test runs but produces the wrong behavior,\n> > rather than barfing with a syntax error.\n> \n> My vanity thanks you for this, even if it's not strictly necessary. As a\n> professional programmer with roughly no C chops and a long-time admirer of the\n> Git project, all I _really_ wanted to do was to fix a thing that was in my\n> wheelhouse so that I could say I have a commit in the history. (This isn't a\n> good reason on its own, of course, but I'm happy it was useful even if the\n> line is immediately deleted!)\n\n:) I think it makes my patch easier to understand, but I'm glad you are\ngetting some you, too. Finding the original issue and the ensuing\ndiscussion was as much (or more) effort than the actual fix. Thanks for\nstarting it!\n\n-Peff\n"},{"id":"465670","messageId":"Y1ch/PKAvNbYNLY7@coredump.intra.peff.net","threadId":"58633","inReplyTo":"221024.861qqxeah5.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 2/2] setup: allow Git.pm to do unsafe repo checking","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-24T23:38:36Z","receivedAt":"2022-10-25T00:55:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 24, 2022 at 12:57:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> The important point/question I have is whether we can think of any such\n> config variable understood by the code that uses Git.pm.\n\nI don't think that matters. Before the CVE fix, Git.pm scripts were just\nas vulnerable as all the other parts of Git. After, they were broken\nbecause of the syntax error. Fixing the syntax error re-opened the bug\nthere, but as long as we close it again before releasing, we don't have\nto care.\n\nYou can argue that the CVE wasn't that important for Git.pm, and thus\nnot that big a deal to re-open. But I think post-CVE we're making the\nstronger promise that Git won't discover a repo directory with funky\nownership. And Git.pm is violating that (or would be after the syntax\nfix if we don't go further).\n\n> The only ones I can think are the \"sendemail.{to,cc}Cmd\" variables.\n\nI don't think we can be that exhaustive. It's also any programs that are\ncalled by scripts using Git.pm. But even that is not a closed set, since\nwe ship Git.pm for people to use in their own scripts. We don't know\nwhat those scripts might be doing.\n\n-Peff\n"},{"id":"467048","messageId":"28orrrr1-444q-6595-po76-3nr67r1pp4p0@tzk.qr","threadId":"58633","inReplyTo":"Y02SHlW1rNQdfCHI@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] Git.pm: add semicolon after catch statement","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-11-10T15:10:22Z","receivedAt":"2022-11-10T15:10:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 17 Oct 2022, Jeff King wrote:\n\n> [... talking about safe.directory ...]\n>\n> But curiously this still does not pass after your patch, because we seem\n> to actually open the repository! I think this is because the C code\n> allows an explicit GIT_DIR to override the safe-directory checks.\n\nYes, I remember that this was something we discussed at some length during\nthe embargo: what to do with the explicitly-specified `GIT_DIR` when\nverifying the ownership, and my recollection is that we asserted that\nsetting `GIT_DIR` qualifies for \"they know what they're doing\" (in\nparticular when it is done in a script, not interactively).\n\nCiao,\nDscho\n"},{"id":"467088","messageId":"Y21wHWXyO/J+Jn9O@coredump.intra.peff.net","threadId":"58633","inReplyTo":"28orrrr1-444q-6595-po76-3nr67r1pp4p0@tzk.qr","subject":"Re: [PATCH 1/1] Git.pm: add semicolon after catch statement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-11-10T21:41:49Z","receivedAt":"2022-11-10T21:42:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 10, 2022 at 04:10:22PM +0100, Johannes Schindelin wrote:\n\n> Hi Peff,\n> \n> On Mon, 17 Oct 2022, Jeff King wrote:\n> \n> > [... talking about safe.directory ...]\n> >\n> > But curiously this still does not pass after your patch, because we seem\n> > to actually open the repository! I think this is because the C code\n> > allows an explicit GIT_DIR to override the safe-directory checks.\n> \n> Yes, I remember that this was something we discussed at some length during\n> the embargo: what to do with the explicitly-specified `GIT_DIR` when\n> verifying the ownership, and my recollection is that we asserted that\n> setting `GIT_DIR` qualifies for \"they know what they're doing\" (in\n> particular when it is done in a script, not interactively).\n\nThanks for confirming. I'm not sure if you read the rest of the thread,\nbut the bug turned out to be in Git.pm, which sets GIT_DIR without\nknowing what it's doing. :)\n\nWe ended up with 20da61f25f (Git.pm: trust rev-parse to find bare\nrepositories, 2022-10-22) as the fix.\n\n-Peff\n"}]}