{"thread":{"id":"61560","subject":"safe.directory wildcards","startedAt":"2024-05-29T09:09:02Z","lastAt":"2024-05-31T14:49:23Z","messageCount":7,"participants":["Stefan Metzmacher","Junio C Hamano","Konstantin Ryabitsev","Patrick Steinhardt","Chris Torek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"495800","messageId":"715163c3-8d59-46ef-81bf-1dda10e6570c@samba.org","threadId":"61560","inReplyTo":null,"subject":"safe.directory wildcards","fromName":"Stefan Metzmacher","fromEmail":"metze@samba.org","sentAt":"2024-05-29T09:08:50Z","receivedAt":"2024-05-29T09:09:02Z","isPatch":false,"sender":{"key":"metze@samba.org","avatar":null},"body":"Hi,\n\ngiven the recent importance of safe.directory, it would be great to\nhave something like '/data/git/*' to be supported, not just a single '*'\nfor a server that serves a lot of public git repositories owned by different owners.\n\nThanks for taking care of security problem, even if the fixes sometimes\nhave unexpected site effects...\n\nmetze\n"},{"id":"495838","messageId":"xmqqplt4zjw7.fsf@gitster.g","threadId":"61560","inReplyTo":"715163c3-8d59-46ef-81bf-1dda10e6570c@samba.org","subject":"Re: safe.directory wildcards","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-29T16:02:16Z","receivedAt":"2024-05-29T16:02:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Metzmacher writes:\n\n> given the recent importance of safe.directory, it would be great to\n> have something like '/data/git/*' to be supported, not just a single '*'\n> for a server that serves a lot of public git repositories owned by different owners.\n\nInteresting.\n\nThe original commit that introduced the '*' opt-out, 0f85c4a3\n(setup: opt-out of check with safe.directory=*, 2022-04-13), was\ndone to specifically help those who have a large list of shared\nrepositories. We could have moved all the way to allow globs back\nthen, and the possibility certainly was brought up.\n\n https://lore.kernel.org/git/xmqqk0bt9bsb.fsf@gitster.g/\n\nBut the loosening was done in a context of \"brown paper bag fix\"\nso it is very much understandable that we did the simplest and most\nobvious thing to avoid making silly mistakes in a haste.\n\nI am reluctant to use wildmatch() but I would expect that in\npractice \"leading path matches\" (in other words, \"everything under\nthis directory is OK\") is sufficient, perhaps?\n\n------- >8 ------------- >8 ------------- >8 -------\nSubject: safe.directory: allow \"lead/ing/path/*\" match\n\nWhen safe.directory was introduced in v2.30.3 timeframe, 8959555c\n(setup_git_directory(): add an owner check for the top-level\ndirectory, 2022-03-02), it only allowed specific opt-out\ndirectories. Immediately after an embargoed release that included\nthe change, 0f85c4a3 (setup: opt-out of check with safe.directory=*,\n2022-04-13) was done as a response to loosen the check so that a\nsingle '*' can be used to say \"I trust all repositories\" for folks\nwho host too many repositories to list individually.\n\nLet's further allow people to say \"everything under this hierarchy\nis deemed safe\" by specifying such a leading directory with \"/*\"\nappended to it.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config/safe.txt | 3 ++-\n setup.c | 24 +++++++++++++++++-------\n t/t0033-safe-directory.sh | 15 +++++++++++++++\n 3 files changed, 34 insertions(+), 8 deletions(-)\n\ndiff --git c/Documentation/config/safe.txt w/Documentation/config/safe.txt\nindex 577df40223..2d45c98b12 100644\n--- c/Documentation/config/safe.txt\n+++ w/Documentation/config/safe.txt\n@@ -44,7 +44,8 @@ string `*`. This will allow all repositories to be treated as if their\n directory was listed in the `safe.directory` list. If `safe.directory=*`\n is set in system config and you want to re-enable this protection, then\n initialize your list with an empty value before listing the repositories\n-that you deem safe.\n+that you deem safe. Giving a directory with `/*` appended to it will\n+allow access to all repositories under the named directory.\n +\n As explained, Git only allows you to access repositories owned by\n yourself, i.e. the user who is running Git, by default. When Git\ndiff --git c/setup.c w/setup.c\nindex 9247cded6a..83c4ea44cd 100644\n--- c/setup.c\n+++ w/setup.c\n@@ -1177,13 +1177,23 @@ static int safe_directory_cb(const char *key, const char *value,\n \t} else if (!strcmp(value, \"*\")) {\n \t\tdata->is_safe = 1;\n \t} else {\n-\t\tconst char *interpolated = NULL;\n-\n-\t\tif (!git_config_pathname(&interpolated, key, value) &&\n-\t\t !fspathcmp(data->path, interpolated ? interpolated : value))\n-\t\t\tdata->is_safe = 1;\n-\n-\t\tfree((char *)interpolated);\n+\t\tconst char *allowed = NULL;\n+\n+\t\tif (!git_config_pathname(&allowed, key, value)) {\n+\t\t\tsize_t len;\n+\n+\t\t\tif (!allowed)\n+\t\t\t\tallowed = value;\n+\t\t\tif (ends_with(allowed, \"/*\")) {\n+\t\t\t\tlen = strlen(allowed);\n+\t\t\t\tif (!fspathncmp(allowed, data->path, len - 1))\n+\t\t\t\t\tdata->is_safe = 1;\n+\t\t\t} else if (!fspathcmp(data->path, allowed)) {\n+\t\t\t\tdata->is_safe = 1;\n+\t\t\t}\n+\t\t}\n+\t\tif (allowed != value)\n+\t\t\tfree((char *)allowed);\n \t}\n \n \treturn 0;\ndiff --git c/t/t0033-safe-directory.sh w/t/t0033-safe-directory.sh\nindex 11c3e8f28e..5fe61f1291 100755\n--- c/t/t0033-safe-directory.sh\n+++ w/t/t0033-safe-directory.sh\n@@ -71,7 +71,22 @@ test_expect_success 'safe.directory=*, but is reset' '\n \texpect_rejected_dir\n '\n \n+test_expect_success 'safe.directory with matching glob' '\n+\tgit config --global --unset-all safe.directory &&\n+\tp=$(pwd) &&\n+\tgit config --global safe.directory \"${p%/*}/*\" &&\n+\tgit status\n+'\n+\n+test_expect_success 'safe.directory with unmatching glob' '\n+\tgit config --global --unset-all safe.directory &&\n+\tp=$(pwd) &&\n+\tgit config --global safe.directory \"${p%/*}no/*\" &&\n+\texpect_rejected_dir\n+'\n+\n test_expect_success 'safe.directory in included file' '\n+\tgit config --global --unset-all safe.directory &&\n \tcat >gitconfig-include <<-EOF &&\n \t[safe]\n \t\tdirectory = \"$(pwd)\"\n"},{"id":"495841","messageId":"20240529-rough-skunk-of-glamour-edcc8b@meerkat","threadId":"61560","inReplyTo":"xmqqplt4zjw7.fsf@gitster.g","subject":"Re: safe.directory wildcards","fromName":"Konstantin Ryabitsev","fromEmail":"konstantin@linuxfoundation.org","sentAt":"2024-05-29T16:35:44Z","receivedAt":"2024-05-29T16:35:47Z","isPatch":false,"sender":{"key":"konstantin@linuxfoundation.org","avatar":"https://gravatar.com/avatar/7cb8827c6de56e1bd2dea16508c6708aa43feed3bf3813bcdacecdf96ceadd79?d=mp&s=160"},"body":"On Wed, May 29, 2024 at 09:02:16AM GMT, Junio C Hamano wrote:\n> When safe.directory was introduced in v2.30.3 timeframe, 8959555c\n> (setup_git_directory(): add an owner check for the top-level\n> directory, 2022-03-02), it only allowed specific opt-out\n> directories. Immediately after an embargoed release that included\n> the change, 0f85c4a3 (setup: opt-out of check with safe.directory=*,\n> 2022-04-13) was done as a response to loosen the check so that a\n> single '*' can be used to say \"I trust all repositories\" for folks\n> who host too many repositories to list individually.\n> \n> Let's further allow people to say \"everything under this hierarchy\n> is deemed safe\" by specifying such a leading directory with \"/*\"\n> appended to it.\n\nI welcome this change, too!\n\nAcked-by: Konstantin Ryabitsev \n\n-K\n"},{"id":"495854","messageId":"22b487e8-b845-43f9-8abe-5f96a756b376@samba.org","threadId":"61560","inReplyTo":"xmqqplt4zjw7.fsf@gitster.g","subject":"Re: safe.directory wildcards","fromName":"Stefan Metzmacher","fromEmail":"metze@samba.org","sentAt":"2024-05-29T18:15:21Z","receivedAt":"2024-05-29T18:15:30Z","isPatch":false,"sender":{"key":"metze@samba.org","avatar":null},"body":"Am 29.05.24 um 18:02 schrieb Junio C Hamano:\n> Stefan Metzmacher writes:\n> \n>> given the recent importance of safe.directory, it would be great to\n>> have something like '/data/git/*' to be supported, not just a single '*'\n>> for a server that serves a lot of public git repositories owned by different owners.\n> \n> Interesting.\n> \n> The original commit that introduced the '*' opt-out, 0f85c4a3\n> (setup: opt-out of check with safe.directory=*, 2022-04-13), was\n> done to specifically help those who have a large list of shared\n> repositories. We could have moved all the way to allow globs back\n> then, and the possibility certainly was brought up.\n> \n> https://lore.kernel.org/git/xmqqk0bt9bsb.fsf@gitster.g/\n> \n> But the loosening was done in a context of \"brown paper bag fix\"\n> so it is very much understandable that we did the simplest and most\n> obvious thing to avoid making silly mistakes in a haste.\n\n> I am reluctant to use wildmatch() but I would expect that in\n> practice \"leading path matches\" (in other words, \"everything under\n> this directory is OK\") is sufficient, perhaps?\n\nThat's exactly what I tried first and then looked\nat the sources to check why it doesn't help :-)\n\nI guess it will be useful to have.\n\nThanks!\nmetze\n"},{"id":"495979","messageId":"ZlmH1CFZWHokAqso@tanuki","threadId":"61560","inReplyTo":"xmqqplt4zjw7.fsf@gitster.g","subject":"Re: safe.directory wildcards","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-05-31T08:18:28Z","receivedAt":"2024-05-31T08:18:34Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 29, 2024 at 09:02:16AM -0700, Junio C Hamano wrote:\n> Stefan Metzmacher writes:\n> \n> > given the recent importance of safe.directory, it would be great to\n> > have something like '/data/git/*' to be supported, not just a single '*'\n> > for a server that serves a lot of public git repositories owned by different owners.\n> \n> Interesting.\n> \n> The original commit that introduced the '*' opt-out, 0f85c4a3\n> (setup: opt-out of check with safe.directory=*, 2022-04-13), was\n> done to specifically help those who have a large list of shared\n> repositories. We could have moved all the way to allow globs back\n> then, and the possibility certainly was brought up.\n> \n> https://lore.kernel.org/git/xmqqk0bt9bsb.fsf@gitster.g/\n> \n> But the loosening was done in a context of \"brown paper bag fix\"\n> so it is very much understandable that we did the simplest and most\n> obvious thing to avoid making silly mistakes in a haste.\n> \n> I am reluctant to use wildmatch() but I would expect that in\n> practice \"leading path matches\" (in other words, \"everything under\n> this directory is OK\") is sufficient, perhaps?\n\nIs there any particular reason why you don't want to use wildmatch?\nI'd think it to be a natural fit here, and it would provide a superset\nof functionality provided by leading paths, only.\n\nPatrick\n"},{"id":"496009","messageId":"CAPx1GvcNF1_te7v3K0bN1s1OaR8JwHTgfQnoKV_tapz6qzbX7Q@mail.gmail.com","threadId":"61560","inReplyTo":"ZlmH1CFZWHokAqso@tanuki","subject":"Re: safe.directory wildcards","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2024-05-31T14:35:58Z","receivedAt":"2024-05-31T14:36:11Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"On Fri, May 31, 2024 at 1:18 AM Patrick Steinhardt wrote:\n> On Wed, May 29, 2024 at 09:02:16AM -0700, Junio C Hamano wrote:\n> > I am reluctant to use wildmatch() but I would expect that in\n> > practice \"leading path matches\" (in other words, \"everything under\n> > this directory is OK\") is sufficient, perhaps?\n>\n> Is there any particular reason why you don't want to use wildmatch?\n> I'd think it to be a natural fit here, and it would provide a superset\n> of functionality provided by leading paths, only.\n\nI have to agree with Patrick here that wildmatch would not be\nsurprising. If the concern is that it might be accidentally dangerous,\nperhaps requiring one to set two knobs might suffice?\n\nOf course I've also thought at many times that Mercurial's\n\"syntax: glob\" and \"syntax: whatever\" rules were a good idea,\nalthough not necessarily that particular notation. Perhaps\n\"safe.literal.directory\" (aka safe.directory) and \"safe.glob.directory\"?\n\nChris\n"},{"id":"496010","messageId":"xmqqy17qhw9d.fsf@gitster.g","threadId":"61560","inReplyTo":"ZlmH1CFZWHokAqso@tanuki","subject":"Re: safe.directory wildcards","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-05-31T14:49:18Z","receivedAt":"2024-05-31T14:49:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> I am reluctant to use wildmatch() but I would expect that in\n>> practice \"leading path matches\" (in other words, \"everything under\n>> this directory is OK\") is sufficient, perhaps?\n>\n> Is there any particular reason why you don't want to use wildmatch?\n\nMostly out of the principle to avoid anything more complex than\nabsolutely necessary in a security relevant code path.\n\nIt is called \"superstition\" in other languages ;-).\n"}]}