threads / patch / 57724

patch, 3 partsUpdates to the safe.directory config option

Subject: [PATCH 0/3] Updates to the safe.directory config option

## tl;dr

39 messages between Apr 13, 2022 and May 10, 2022. Diffs are folded; open one to read it.

replies: 38people: 8as markdown or json

Derrick Stolee via GitGitGadget· Apr 13, 2022, 15:32 UTC · lore
Here is a very fast response to the security release yesterday.

The second patch here is an adaptation from a contributor who created a pull request against git/git [1]. I augmented the patch with a test (the test infrastructure is added in patch 1).

The third patch is a change to the safe.directory config option to include a possible "*" value to completely opt-out of the check. This will be particularly helpful for cases where users run Git commands within a container. This container workflow always runs as a different user than the host, but also the container does not have access to the host's system or global config files. It's also helpful for users who don't want to set the config for a large number of shared repositories [2].

Thanks, -Stolee

[1] https://github.com/git/git/pull/1235 [2] https://github.com/git-for-windows/git/issues/3787 [3] https://github.com/desktop/desktop/issues/14336

Derrick Stolee (2):
  t0033: add tests for safe.directory
  setup: opt-out of check with safe.directory=*
Matheus Valadares (1):
  setup: fix safe.directory key not being checked
 Documentation/config/safe.txt |  7 +++++
 setup.c                       | 12 ++++++---
 t/t0033-safe-directory.sh     | 49 +++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+), 3 deletions(-)
 create mode 100755 t/t0033-safe-directory.sh
base-commit: 11cfe552610386954886543f5de87dcc49ad5735
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1215%2Fderrickstolee%2Fsafe-directories-star-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1215/derrickstolee/safe-directories-star-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1215
-- 
gitgitgadget
Derrick Stolee via GitGitGadget· Apr 13, 2022, 15:32 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 1/3] t0033: add tests for safe.directory

From: Derrick Stolee <derrickstolee@github.com>

It is difficult to change the ownership on a directory in our test suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment variable to trick Git into thinking we are in a differently-owned directory. This allows us to test that the config is parsed correctly.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 setup.c                   |  3 ++-
 t/t0033-safe-directory.sh | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100755 t/t0033-safe-directory.sh
Show changes to 2 files +36 −1

setup.c, t/t0033-safe-directory.sh

diff --git a/setup.c b/setup.c
index c8f67bfed52..f54f449008a 100644
--- a/setup.c
+++ b/setup.c
@@ -1119,7 +1119,8 @@ static int ensure_valid_ownership(const char *path)
 {
 	struct safe_directory_data data = { .path = path };
 
-	if (is_path_owned_by_current_user(path))
+	if (is_path_owned_by_current_user(path) &&
+	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
 		return 1;
 
 	read_very_early_config(safe_directory_cb, &data);
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
new file mode 100755
index 00000000000..9380ff3d017
--- /dev/null
+++ b/t/t0033-safe-directory.sh
@@ -0,0 +1,34 @@
+#!/bin/sh
+
+test_description='verify safe.directory checks'
+
+. ./test-lib.sh
+
+GIT_TEST_ASSUME_DIFFERENT_OWNER=1
+export GIT_TEST_ASSUME_DIFFERENT_OWNER
+
+expect_rejected_dir () {
+	test_must_fail git status 2>err &&
+	grep "safe.directory" err
+}
+
+test_expect_success 'safe.directory is not set' '
+	expect_rejected_dir
+'
+
+test_expect_success 'safe.directory does not match' '
+	git config --global safe.directory bogus &&
+	expect_rejected_dir
+'
+
+test_expect_success 'safe.directory matches' '
+	git config --global --add safe.directory "$(pwd)" &&
+	git status
+'
+
+test_expect_success 'safe.directory matches, but is reset' '
+	git config --global --add safe.directory "" &&
+	expect_rejected_dir
+'
+
+test_done

-- 
gitgitgadget
Junio C Hamano· Apr 13, 2022, 16:24 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 1/3] t0033: add tests for safe.directory

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> From: Derrick Stolee <derrickstolee@github.com>
>
> It is difficult to change the ownership on a directory in our test
> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
> variable to trick Git into thinking we are in a differently-owned
> directory. This allows us to test that the config is parsed correctly.
OK.
> -	if (is_path_owned_by_current_user(path))
> +	if (is_path_owned_by_current_user(path) &&
> +	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
>  		return 1;

Shouldn't the overriding "GIT_TEST_BLAH" be checked before the real logic kicks in, I wonder?

Show 24 quoted lines
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> new file mode 100755
> index 00000000000..9380ff3d017
> --- /dev/null
> +++ b/t/t0033-safe-directory.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='verify safe.directory checks'
> +
> +. ./test-lib.sh
> +
> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
> +
> +expect_rejected_dir () {
> +	test_must_fail git status 2>err &&
> +	grep "safe.directory" err
> +}
> +...
> +test_expect_success 'safe.directory matches' '
> +	git config --global --add safe.directory "$(pwd)" &&
> +	git status
> +'

Just double checking, as I know you are much closer to the affected platform than I'd ever be ;-) but is the use of $(pwd) safe and correct here?

I always get confused between $(pwd) and $PWD, which does not make any difference on platforms I have access to, but makes difference to hurt Windows users.

Show 6 quoted lines
> +test_expect_success 'safe.directory matches, but is reset' '
> +	git config --global --add safe.directory "" &&
> +	expect_rejected_dir
> +'
> +
> +test_done

Thanks. This step should apply to maint-2.30 cleanly, I would think.

Derrick Stolee· Apr 13, 2022, 16:29 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/3] t0033: add tests for safe.directory

On 4/13/2022 12:24 PM, Junio C Hamano wrote:
Show 18 quoted lines
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> It is difficult to change the ownership on a directory in our test
>> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
>> variable to trick Git into thinking we are in a differently-owned
>> directory. This allows us to test that the config is parsed correctly.
> 
> OK.
> 
>> -	if (is_path_owned_by_current_user(path))
>> +	if (is_path_owned_by_current_user(path) &&
>> +	    !git_env_bool("GIT_TEST_ASSUME_DIFFERENT_OWNER", 0))
>>  		return 1;
> 
> Shouldn't the overriding "GIT_TEST_BLAH" be checked before the
> real logic kicks in, I wonder?
Either order would work. I bet that checking the environment is
faster than checking the disk, so swapping the order would be prudent
here.
Show 32 quoted lines
>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
>> new file mode 100755
>> index 00000000000..9380ff3d017
>> --- /dev/null
>> +++ b/t/t0033-safe-directory.sh
>> @@ -0,0 +1,34 @@
>> +#!/bin/sh
>> +
>> +test_description='verify safe.directory checks'
>> +
>> +. ./test-lib.sh
>> +
>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
>> +
>> +expect_rejected_dir () {
>> +	test_must_fail git status 2>err &&
>> +	grep "safe.directory" err
>> +}
>> +...
>> +test_expect_success 'safe.directory matches' '
>> +	git config --global --add safe.directory "$(pwd)" &&
>> +	git status
>> +'
> 
> Just double checking, as I know you are much closer to the affected
> platform than I'd ever be ;-) but is the use of $(pwd) safe and
> correct here?
> 
> I always get confused between $(pwd) and $PWD, which does not make
> any difference on platforms I have access to, but makes difference
> to hurt Windows users.

These tests pass CI on Windows. I've had issues before using $PWD, thinking back to tests in t7900-maintenance.sh that use $(pwd) instead.

Thanks, -Stolee

Ævar Arnfjörð Bjarmason· Apr 13, 2022, 19:16 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 1/3] t0033: add tests for safe.directory

On Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:
Show 6 quoted lines
> From: Derrick Stolee <derrickstolee@github.com>
>
> It is difficult to change the ownership on a directory in our test
> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
> variable to trick Git into thinking we are in a differently-owned
> directory. This allows us to test that the config is parsed correctly.

I think this is a good trade-off, but FWIW I'd think we could test also without the git_env_bool() by having the test depend on !NOT_ROOT, then check the owner of t/test-lib.sh, and chown to that user (i.e. the "real" user).

But that's all sorts of more fragile than just this test variable..
Show 6 quoted lines
> +test_description='verify safe.directory checks'
> +
> +. ./test-lib.sh
> +
> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
> +export GIT_TEST_ASSUME_DIFFERENT_OWNER

Instead of this "export" perhaps just add it in front of the "git status"?

These tests also pass with SANITIZE=leak, so please add TEST_PASSES_SANITIZE_LEAK=true at the top.

Show 16 quoted lines
> +expect_rejected_dir () {
> +	test_must_fail git status 2>err &&
> +	grep "safe.directory" err
> +}
> +
> +test_expect_success 'safe.directory is not set' '
> +	expect_rejected_dir
> +'
> +
> +test_expect_success 'safe.directory does not match' '
> +	git config --global safe.directory bogus &&
> +	expect_rejected_dir
> +'
> +
> +test_expect_success 'safe.directory matches' '
> +	git config --global --add safe.directory "$(pwd)" &&
nit: $PWD instead of $(pwd)
Junio C Hamano· Apr 13, 2022, 19:46 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/3] t0033: add tests for safe.directory

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
(just this part)
> These tests also pass with SANITIZE=leak, so please add
> TEST_PASSES_SANITIZE_LEAK=true at the top.

Derrick, please ignore the above. It is totally outside the scope of these patches, and they are meant to be applied on top of the 2.30 maintenance track, where TEST_PASSES_SANITIZE_LEAK=true was irrelevant.

I do not mind adding such after the dust settles on top of 'master' (or possibly 'maint'), but not as part of these "let's fix the screw up in 2.30.3 and its friends" effort.

Thanks.
Derrick Stolee· Apr 13, 2022, 19:52 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH 1/3] t0033: add tests for safe.directory

On 4/13/2022 3:16 PM, Ævar Arnfjörð Bjarmason wrote:
Show 25 quoted lines
> 
> On Wed, Apr 13 2022, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>
>>
>> It is difficult to change the ownership on a directory in our test
>> suite, so insert a new GIT_TEST_ASSUME_DIFFERENT_OWNER environment
>> variable to trick Git into thinking we are in a differently-owned
>> directory. This allows us to test that the config is parsed correctly.
> 
> I think this is a good trade-off, but FWIW I'd think we could test also
> without the git_env_bool() by having the test depend on !NOT_ROOT, then
> check the owner of t/test-lib.sh, and chown to that user (i.e. the
> "real" user).
> 
> But that's all sorts of more fragile than just this test variable..
>> +test_description='verify safe.directory checks'
>> +
>> +. ./test-lib.sh
>> +
>> +GIT_TEST_ASSUME_DIFFERENT_OWNER=1
>> +export GIT_TEST_ASSUME_DIFFERENT_OWNER
> 
> Instead of this "export" perhaps just add it in front of the "git
> status"?
If the only runs were in this helper below, then yes.
>> +expect_rejected_dir () {
>> +	test_must_fail git status 2>err &&
>> +	grep "safe.directory" err
>> +}

Later patches add more success cases that run 'git status' as its verification that the match works. I didn't think it was good to have this environment variable set for each of those invocations.

This script has one purpose, and this environment variable is required to make any of the checks work. Setting it globally seems the best way to do that.

>> +test_expect_success 'safe.directory matches' '
>> +	git config --global --add safe.directory "$(pwd)" &&
> 
> nit: $PWD instead of $(pwd)

Historically, $PWD doesn't work properly across platforms, so I have used $(pwd) consistently across many contributions.

Thanks, -Stolee

Matheus Valadares via GitGitGadget· Apr 13, 2022, 15:32 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 2/3] setup: fix safe.directory key not being checked

From: Matheus Valadares <me@m28.io>

It seems that nothing is ever checking to make sure the safe directories in the configs actually have the key safe.directory, so some unrelated config that has a value with a certain directory would also make it a safe directory.

Signed-off-by: Matheus Valadares <me@m28.io>
Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 setup.c                   | 3 +++
 t/t0033-safe-directory.sh | 5 +++++
 2 files changed, 8 insertions(+)
Show changes to 2 files +8 −0

setup.c, t/t0033-safe-directory.sh

diff --git a/setup.c b/setup.c
index f54f449008a..a995c359c32 100644
--- a/setup.c
+++ b/setup.c
@@ -1100,6 +1100,9 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
 {
 	struct safe_directory_data *data = d;
 
+	if (strcmp(key, "safe.directory"))
+		return 0;
+
 	if (!value || !*value)
 		data->is_safe = 0;
 	else {
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 9380ff3d017..6f33c0dfefa 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -21,6 +21,11 @@ test_expect_success 'safe.directory does not match' '
 	expect_rejected_dir
 '
 
+test_expect_success 'path exist as different key' '
+	git config --global foo.bar "$(pwd)" &&
+	expect_rejected_dir
+'
+
 test_expect_success 'safe.directory matches' '
 	git config --global --add safe.directory "$(pwd)" &&
 	git status

-- 
gitgitgadget
Junio C Hamano· Apr 13, 2022, 16:34 UTC · re: Matheus Valadares via GitGitGadget · lore

Re: [PATCH 2/3] setup: fix safe.directory key not being checked

"Matheus Valadares via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines
> From: Matheus Valadares <me@m28.io>
>
> It seems that nothing is ever checking to make sure the safe directories
> in the configs actually have the key safe.directory, so some unrelated
> config that has a value with a certain directory would also make it a
> safe directory.

Good finding, and the fix is straight-forward and obviously correct. Thanks.

Show 6 quoted lines
> Signed-off-by: Matheus Valadares <me@m28.io>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  setup.c                   | 3 +++
>  t/t0033-safe-directory.sh | 5 +++++
>  2 files changed, 8 insertions(+)
Show 30 quoted lines
> diff --git a/setup.c b/setup.c
> index f54f449008a..a995c359c32 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1100,6 +1100,9 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
>  {
>  	struct safe_directory_data *data = d;
>  
> +	if (strcmp(key, "safe.directory"))
> +		return 0;
> +
>  	if (!value || !*value)
>  		data->is_safe = 0;
>  	else {
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 9380ff3d017..6f33c0dfefa 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -21,6 +21,11 @@ test_expect_success 'safe.directory does not match' '
>  	expect_rejected_dir
>  '
>  
> +test_expect_success 'path exist as different key' '
> +	git config --global foo.bar "$(pwd)" &&
> +	expect_rejected_dir
> +'
> +
>  test_expect_success 'safe.directory matches' '
>  	git config --global --add safe.directory "$(pwd)" &&
>  	git status
Derrick Stolee via GitGitGadget· Apr 13, 2022, 15:32 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 3/3] setup: opt-out of check with safe.directory=*

From: Derrick Stolee <derrickstolee@github.com>

With the addition of the safe.directory in 8959555ce (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) released in v2.35.2, we are receiving feedback from a variety of users about the feature.

Some users have a very large list of shared repositories and find it cumbersome to add this config for every one of them.

In a more difficult case, certain workflows involve running Git commands within containers. The container boundary prevents any global or system config from communicating `safe.directory` values from the host into the container. Further, the container almost always runs as a different user than the owner of the directory in the host.

To simplify the reactions necessary for these users, extend the definition of the safe.directory config value to include a possible '*' value. This value implies that all directories are safe, providing a single setting to opt-out of this protection.

Note that an empty assignment of safe.directory clears all previous values, and this is already the case with the "if (!value || !*value)" condition.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 Documentation/config/safe.txt |  7 +++++++
 setup.c                       |  6 ++++--
 t/t0033-safe-directory.sh     | 10 ++++++++++
 3 files changed, 21 insertions(+), 2 deletions(-)
Show changes to 3 files +21 −2

Documentation/config/safe.txt, setup.c, t/t0033-safe-directory.sh

diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
index 63597b2df8f..6d764fe0ccf 100644
--- a/Documentation/config/safe.txt
+++ b/Documentation/config/safe.txt
@@ -19,3 +19,10 @@ line option `-c safe.directory=<path>`.
 The value of this setting is interpolated, i.e. `~/<path>` expands to a
 path relative to the home directory and `%(prefix)/<path>` expands to a
 path relative to Git's (runtime) prefix.
++
+To completely opt-out of this security check, set `safe.directory` to the
+string `*`. This will allow all repositories to be treated as if their
+directory was listed in the `safe.directory` list. If `safe.directory=*`
+is set in system config and you want to re-enable this protection, then
+initialize your list with an empty value before listing the repositories
+that you deem safe.
diff --git a/setup.c b/setup.c
index a995c359c32..a42b21307f7 100644
--- a/setup.c
+++ b/setup.c
@@ -1103,9 +1103,11 @@ static int safe_directory_cb(const char *key, const char *value, void *d)
 	if (strcmp(key, "safe.directory"))
 		return 0;
 
-	if (!value || !*value)
+	if (!value || !*value) {
 		data->is_safe = 0;
-	else {
+	} else if (!strcmp(value, "*")) {
+		data->is_safe = 1;
+	} else {
 		const char *interpolated = NULL;
 
 		if (!git_config_pathname(&interpolated, key, value) &&
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 6f33c0dfefa..239d93f4d21 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -36,4 +36,14 @@ test_expect_success 'safe.directory matches, but is reset' '
 	expect_rejected_dir
 '
 
+test_expect_success 'safe.directory=*' '
+	git config --global --add safe.directory "*" &&
+	git status
+'
+
+test_expect_success 'safe.directory=*, but is reset' '
+	git config --global --add safe.directory "" &&
+	expect_rejected_dir
+'
+
 test_done

-- 
gitgitgadget
Junio C Hamano· Apr 13, 2022, 16:40 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 3/3] setup: opt-out of check with safe.directory=*

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> To simplify the reactions necessary for these users, extend the
> definition of the safe.directory config value to include a possible '*'
> value. This value implies that all directories are safe, providing a
> single setting to opt-out of this protection.

OK. During the development of the original fix, we discussed if a more flexible mechanism, like allowing globs, but ended up with the simplest and easiest to explain option, with the expectation that we may want to loosen it later as necessary. And this is certainly what we would have expected to add.

> Note that an empty assignment of safe.directory clears all previous
> values, and this is already the case with the "if (!value || !*value)"
> condition.
OK.
Show 14 quoted lines
>  	if (strcmp(key, "safe.directory"))
>  		return 0;
>  
> -	if (!value || !*value)
> +	if (!value || !*value) {
>  		data->is_safe = 0;
> -	else {
> +	} else if (!strcmp(value, "*")) {
> +		data->is_safe = 1;
> +	} else {
>  		const char *interpolated = NULL;
>  
>  		if (!git_config_pathname(&interpolated, key, value) &&
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
OK.
Show 16 quoted lines
> index 6f33c0dfefa..239d93f4d21 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -36,4 +36,14 @@ test_expect_success 'safe.directory matches, but is reset' '
>  	expect_rejected_dir
>  '
>  
> +test_expect_success 'safe.directory=*' '
> +	git config --global --add safe.directory "*" &&
> +	git status
> +'
> +
> +test_expect_success 'safe.directory=*, but is reset' '
> +	git config --global --add safe.directory "" &&
> +	expect_rejected_dir
> +'
Thanks.
Junio C Hamano· Apr 13, 2022, 16:15 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 0/3] Updates to the safe.directory config option

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Here is a very fast response to the security release yesterday.

Wow. While I were down the whole day yesterday after sending the release announcement, it seems a lot have happened X-<. Does your "a very fast" expect only "wow, thanks for a fast reponse", or does it also expect "ok, we'll take a deep look with a spoonful of salt as it was prepared in haste"?

Show 11 quoted lines
> The second patch here is an adaptation from a contributor who created a pull
> request against git/git [1]. I augmented the patch with a test (the test
> infrastructure is added in patch 1).
>
> The third patch is a change to the safe.directory config option to include a
> possible "*" value to completely opt-out of the check. This will be
> particularly helpful for cases where users run Git commands within a
> container. This container workflow always runs as a different user than the
> host, but also the container does not have access to the host's system or
> global config files. It's also helpful for users who don't want to set the
> config for a large number of shared repositories [2].

Let me take a look how well these integrate into the maintenance tracks.

I would appreciate something that is targetted and narrow that can be applied to the oldest maintenance track (2.30.3) and then merged upwards, plus niceties on top that does not necessarily have to apply to the oldest ones if the surrounding code or tests were changed more recently.

Thanks.
Show 24 quoted lines
> Thanks, -Stolee
>
> [1] https://github.com/git/git/pull/1235 [2]
> https://github.com/git-for-windows/git/issues/3787 [3]
> https://github.com/desktop/desktop/issues/14336
>
> Derrick Stolee (2):
>   t0033: add tests for safe.directory
>   setup: opt-out of check with safe.directory=*
>
> Matheus Valadares (1):
>   setup: fix safe.directory key not being checked
>
>  Documentation/config/safe.txt |  7 +++++
>  setup.c                       | 12 ++++++---
>  t/t0033-safe-directory.sh     | 49 +++++++++++++++++++++++++++++++++++
>  3 files changed, 65 insertions(+), 3 deletions(-)
>  create mode 100755 t/t0033-safe-directory.sh
>
>
> base-commit: 11cfe552610386954886543f5de87dcc49ad5735
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1215%2Fderrickstolee%2Fsafe-directories-star-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1215/derrickstolee/safe-directories-star-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1215
Derrick Stolee· Apr 13, 2022, 16:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Updates to the safe.directory config option

On 4/13/2022 12:15 PM, Junio C Hamano wrote:
Show 9 quoted lines
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> Here is a very fast response to the security release yesterday.
> 
> Wow.  While I were down the whole day yesterday after sending the
> release announcement, it seems a lot have happened X-<.  Does your
> "a very fast" expect only "wow, thanks for a fast reponse", or does
> it also expect "ok, we'll take a deep look with a spoonful of salt
> as it was prepared in haste"?

I tried to do my due diligence here, but I will admit to some amount of haste being applied due to the many distinct sources that have motivated the change.

Show 20 quoted lines
>> The second patch here is an adaptation from a contributor who created a pull
>> request against git/git [1]. I augmented the patch with a test (the test
>> infrastructure is added in patch 1).
>>
>> The third patch is a change to the safe.directory config option to include a
>> possible "*" value to completely opt-out of the check. This will be
>> particularly helpful for cases where users run Git commands within a
>> container. This container workflow always runs as a different user than the
>> host, but also the container does not have access to the host's system or
>> global config files. It's also helpful for users who don't want to set the
>> config for a large number of shared repositories [2].
> 
> Let me take a look how well these integrate into the maintenance
> tracks.
> 
> I would appreciate something that is targetted and narrow that can
> be applied to the oldest maintenance track (2.30.3) and then merged
> upwards, plus niceties on top that does not necessarily have to
> apply to the oldest ones if the surrounding code or tests were
> changed more recently.

The tests that are added are in a new test file, so hopefully those don't collide with anything.

The changes in setup.c apply within the ensure_valid_ownership() so should apply to any versions that have the fix.

Thanks, -Stolee

Junio C Hamano· Apr 13, 2022, 16:44 UTC · re: Derrick Stolee · lore

Re: [PATCH 0/3] Updates to the safe.directory config option

Derrick Stolee <derrickstolee@github.com> writes:
> I tried to do my due diligence here, but I will admit to some amount
> of haste being applied due to the many distinct sources that have
> motivated the change.
OK.  Thanks for a fast response to a brown-paper-bag bug or two.
Show 5 quoted lines
> The tests that are added are in a new test file, so hopefully
> those don't collide with anything.
>
> The changes in setup.c apply within the ensure_valid_ownership()
> so should apply to any versions that have the fix.

Yup, I was more worried about newer test helpers and API functions that did not exist in the older code base, but after reading the patches through, I think these are all from battle tested and solid part that applies to all relevant maintenance tracks.

Thanks.
Junio C Hamano· Apr 13, 2022, 20:27 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Updates to the safe.directory config option

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> Derrick Stolee <derrickstolee@github.com> writes:
>
>> I tried to do my due diligence here, but I will admit to some amount
>> of haste being applied due to the many distinct sources that have
>> motivated the change.
>
> OK.  Thanks for a fast response to a brown-paper-bag bug or two.

Here is a draft release notes to 2.30.4.txt; I may tag this later tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together with the result of merging the fixes into newer maintenance releases.

Thanks, everybody, for working on the issue.

Git v2.30.4 Release Notes (draft) =================================

This release contains minor fix-ups for the changes that went into Git 2.30.3, which was made to address CVE-2022-24765.

 * The code that was meant to parse the new `safe.directory`
   configuration variable was not checking what configuration
   variable was being fed to it, which has been corrected.
 * '*' can be used as the value for the `safe.directory` variable to
   signal that the user considers that any directory is safe.
Derrick Stolee (2):
      t0033: add tests for safe.directory
      setup: opt-out of check with safe.directory=*
Matheus Valadares (1):
      setup: fix safe.directory key not being checked
Taylor Blau· Apr 13, 2022, 21:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/3] Updates to the safe.directory config option

On Wed, Apr 13, 2022 at 01:27:05PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Derrick Stolee <derrickstolee@github.com> writes:
> >
> >> I tried to do my due diligence here, but I will admit to some amount
> >> of haste being applied due to the many distinct sources that have
> >> motivated the change.
> >
> > OK.  Thanks for a fast response to a brown-paper-bag bug or two.
>
> Here is a draft release notes to 2.30.4.txt; I may tag this later
> tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together
> with the result of merging the fixes into newer maintenance releases.
>
> Thanks, everybody, for working on the issue.

Thanks; this all looks good to me. I took a careful look at Stolee's new patches as well as the (elided) release notes from you below, and both look good to me.

Don't worry about applying my Reviewed-by, but I did want to acknowledge having looked at these.

Thanks, Taylor

Junio C Hamano· Apr 13, 2022, 21:45 UTC · re: Taylor Blau · lore

Re: [PATCH 0/3] Updates to the safe.directory config option

Taylor Blau <me@ttaylorr.com> writes:
Show 23 quoted lines
> On Wed, Apr 13, 2022 at 01:27:05PM -0700, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> > Derrick Stolee <derrickstolee@github.com> writes:
>> >
>> >> I tried to do my due diligence here, but I will admit to some amount
>> >> of haste being applied due to the many distinct sources that have
>> >> motivated the change.
>> >
>> > OK.  Thanks for a fast response to a brown-paper-bag bug or two.
>>
>> Here is a draft release notes to 2.30.4.txt; I may tag this later
>> tonight (i.e. in 4 hours or so) or perhaps tomorrow morning, together
>> with the result of merging the fixes into newer maintenance releases.
>>
>> Thanks, everybody, for working on the issue.
>
> Thanks; this all looks good to me. I took a careful look at Stolee's new
> patches as well as the (elided) release notes from you below, and both
> look good to me.
>
> Don't worry about applying my Reviewed-by, but I did want to acknowledge
> having looked at these.
Thanks for giving an extra set of eyes.
SZEDER Gábor· Apr 27, 2022, 17:06 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 0/3] t0033-safe-directory: test improvements and a documentation clarification

Add coverage for the cases when 'safe.directory' is ignored, a minor test improvement, and a small documentation clarification.

SZEDER Gábor (3):
  t0033-safe-directory: check the error message without matching the
    trash dir
  t0033-safe-directory: check when 'safe.directory' is ignored
  safe.directory: document and check that it's ignored in the
    environment
 Documentation/config/safe.txt |  4 ++--
 t/t0033-safe-directory.sh     | 30 +++++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 3 deletions(-)
-- 
2.36.0.676.gf39b21ed98
SZEDER Gábor· Apr 27, 2022, 17:06 UTC · re: SZEDER Gábor · lore

[PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir

Since 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) when git finds itself in a repository owned by someone else, it aborts with a "fatal: unsafe repository (<repo path>)" error message and an advice about how to set the 'safe.directory' config variable to mark that repository as safe. 't0033-safe-directory.sh' contains tests that check that this feature and handling said config work as intended. To ensure that git dies for the right reason, several of those tests check that its standard error contains the name of that config variable, but:

  - it only appears in the advice part, not in the actual error
    message.
  - it is interpreted as a regexp by 'grep', so, because of the dot,
    it matches the name of the test script and the path of the trash
    directory as well.  Consequently, these tests could be fooled by
    any error message that would happen to include the path of the
    test repository.
Tighten these checks to look for "unsafe repository" instead.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0033-safe-directory.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t0033-safe-directory.sh +1 −1
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 239d93f4d2..6f9680e8b0 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER
 
 expect_rejected_dir () {
 	test_must_fail git status 2>err &&
-	grep "safe.directory" err
+	grep "unsafe repository" err
 }
 
 test_expect_success 'safe.directory is not set' '

-- 
2.36.0.676.gf39b21ed98
Taylor Blau· May 9, 2022, 22:27 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir

On Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote:
Show 5 quoted lines
>   - it is interpreted as a regexp by 'grep', so, because of the dot,
>     it matches the name of the test script and the path of the trash
>     directory as well.  Consequently, these tests could be fooled by
>     any error message that would happen to include the path of the
>     test repository.

Nice catch. I wouldn't have even thought about "could match the test directory" as a potential way for this test to generate a false positive. So I'm happy to see us tightening this down.

Thanks.
Taylor
Carlo Marcelo Arenas Belón· May 10, 2022, 06:04 UTC · re: SZEDER Gábor · lore

Re: [PATCH 1/3] t0033-safe-directory: check the error message without matching the trash dir

Apologies for the late feedback, hoping it is still timely though.
On Wed, Apr 27, 2022 at 07:06:47PM +0200, SZEDER Gábor wrote:
Show 9 quoted lines
> 
>   - it only appears in the advice part, not in the actual error
>     message.
> 
>   - it is interpreted as a regexp by 'grep', so, because of the dot,
>     it matches the name of the test script and the path of the trash
>     directory as well.  Consequently, these tests could be fooled by
>     any error message that would happen to include the path of the
>     test repository.

The subject says something else and I think it is confusing because the change you propose only seems to be removing the path as an implementation detail while the core of the change seems to be making sure that the text that is being matched is a "Fixed String" and part of the real error.

I agree is an improvement, but I think it could be done better by :
* using `grep -F` instead to make sure we don't interpret that string
  as a regex by mistake.
* including the path, so we are sure we really matched the error message
  that was really expected.  As a side effect we also validate this way
  that the code that does the lookup and reports the "name" of the
  repository that could be added to the exception list, reports the righ
  directory, which seems by itself an important thing we should want to
  avoid regressing against in future changes.

Both changed are implemented by the patch below and I think might be worth considering as a replacement of this one.

Carlo
PS. Commit message could be improved, but wasn't sure how in the context
    of your series, so left it as a barebones one hoping it would be part
    of the patch you could adopt into your series.
----- >8 -----
Subject: t0033: verify detected worktree is accurate and part of the error
    
Change the helper function to check for a fixed message and make
sure to include the directory that we are testing from (which in
all current tests is the current directory).
    
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
Show changes to t/t0033-safe-directory.sh +1 −1
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 239d93f4d2..c524b74b01 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -9,7 +9,7 @@ export GIT_TEST_ASSUME_DIFFERENT_OWNER
 
 expect_rejected_dir () {
 	test_must_fail git status 2>err &&
-	grep "safe.directory" err
+	grep -F "unsafe repository ('$(pwd)'" err
 }
 
 test_expect_success 'safe.directory is not set' '
SZEDER Gábor· Apr 27, 2022, 17:06 UTC · re: SZEDER Gábor · lore

[PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

According to the documentation 'safe.directory' "is only respected when specified in a system or global config, not when it is specified in a repository config or via the command line option -c safe.directory=<path>".

Add tests to check that 'safe.directory' in the repository config or on the command line is indeed ignored.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t0033-safe-directory.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t0033-safe-directory.sh +13 −0
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 6f9680e8b0..82dac0eb93 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
 	expect_rejected_dir
 '
 
+test_expect_success 'ignoring safe.directory on the command line' '
+	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
+	grep "unsafe repository" err
+'
+
+test_expect_success 'ignoring safe.directory in repo config' '
+	(
+		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
+		git config safe.directory "$(pwd)"
+	) &&
+	expect_rejected_dir
+'
+
 test_expect_success 'safe.directory does not match' '
 	git config --global safe.directory bogus &&
 	expect_rejected_dir

-- 
2.36.0.676.gf39b21ed98
Junio C Hamano· Apr 27, 2022, 20:37 UTC · re: SZEDER Gábor · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 33 quoted lines
> According to the documentation 'safe.directory' "is only respected
> when specified in a system or global config, not when it is specified
> in a repository config or via the command line option -c
> safe.directory=<path>".
>
> Add tests to check that 'safe.directory' in the repository config or
> on the command line is indeed ignored.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  t/t0033-safe-directory.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 6f9680e8b0..82dac0eb93 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
>  	expect_rejected_dir
>  '
>  
> +test_expect_success 'ignoring safe.directory on the command line' '
> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
> +	grep "unsafe repository" err
> +'
> +
> +test_expect_success 'ignoring safe.directory in repo config' '
> +	(
> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> +		git config safe.directory "$(pwd)"
> +	) &&
> +	expect_rejected_dir
> +'

I am debating myself if we want to remove the in-repository safe.directory configuration setting after this test piece is done, with test_when_finished. We just made sure, with this test, that having the variable does not affect anything, so the subsequent tests should not care hence it is probably OK. On the other hand, to make sure those settings they make (e.g. setting it globally is what the next test we can see below does) is what affects their outcome, it removes one more thing we need to worry about if we clean after ourselves. I dunno.

Other than that, looking good so far; both [1/3] and [2/3] look good.
>  test_expect_success 'safe.directory does not match' '
>  	git config --global safe.directory bogus &&
>  	expect_rejected_dir
Derrick Stolee· Apr 29, 2022, 16:12 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On 4/27/2022 4:37 PM, Junio C Hamano wrote:
Show 45 quoted lines
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
>> According to the documentation 'safe.directory' "is only respected
>> when specified in a system or global config, not when it is specified
>> in a repository config or via the command line option -c
>> safe.directory=<path>".
>>
>> Add tests to check that 'safe.directory' in the repository config or
>> on the command line is indeed ignored.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>  t/t0033-safe-directory.sh | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
>> index 6f9680e8b0..82dac0eb93 100755
>> --- a/t/t0033-safe-directory.sh
>> +++ b/t/t0033-safe-directory.sh
>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
>>  	expect_rejected_dir
>>  '
>>  
>> +test_expect_success 'ignoring safe.directory on the command line' '
>> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
>> +	grep "unsafe repository" err
>> +'
>> +
>> +test_expect_success 'ignoring safe.directory in repo config' '
>> +	(
>> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>> +		git config safe.directory "$(pwd)"
>> +	) &&
>> +	expect_rejected_dir
>> +'
> 
> I am debating myself if we want to remove the in-repository
> safe.directory configuration setting after this test piece is done,
> with test_when_finished.  We just made sure, with this test, that
> having the variable does not affect anything, so the subsequent
> tests should not care hence it is probably OK.  On the other hand,
> to make sure those settings they make (e.g. setting it globally is
> what the next test we can see below does) is what affects their
> outcome, it removes one more thing we need to worry about if we
> clean after ourselves.  I dunno.

test_config would do the same, right? I think it automatically does the test_when_finished for us.

Thanks, -Stolee

Junio C Hamano· Apr 29, 2022, 17:57 UTC · re: Derrick Stolee · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

Derrick Stolee <derrickstolee@github.com> writes:
Show 49 quoted lines
> On 4/27/2022 4:37 PM, Junio C Hamano wrote:
>> SZEDER Gábor <szeder.dev@gmail.com> writes:
>> 
>>> According to the documentation 'safe.directory' "is only respected
>>> when specified in a system or global config, not when it is specified
>>> in a repository config or via the command line option -c
>>> safe.directory=<path>".
>>>
>>> Add tests to check that 'safe.directory' in the repository config or
>>> on the command line is indeed ignored.
>>>
>>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>>> ---
>>>  t/t0033-safe-directory.sh | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
>>> index 6f9680e8b0..82dac0eb93 100755
>>> --- a/t/t0033-safe-directory.sh
>>> +++ b/t/t0033-safe-directory.sh
>>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
>>>  	expect_rejected_dir
>>>  '
>>>  
>>> +test_expect_success 'ignoring safe.directory on the command line' '
>>> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
>>> +	grep "unsafe repository" err
>>> +'
>>> +
>>> +test_expect_success 'ignoring safe.directory in repo config' '
>>> +	(
>>> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
>>> +		git config safe.directory "$(pwd)"
>>> +	) &&
>>> +	expect_rejected_dir
>>> +'
>> 
>> I am debating myself if we want to remove the in-repository
>> safe.directory configuration setting after this test piece is done,
>> with test_when_finished.  We just made sure, with this test, that
>> having the variable does not affect anything, so the subsequent
>> tests should not care hence it is probably OK.  On the other hand,
>> to make sure those settings they make (e.g. setting it globally is
>> what the next test we can see below does) is what affects their
>> outcome, it removes one more thing we need to worry about if we
>> clean after ourselves.  I dunno.
>
> test_config would do the same, right? I think it automatically
> does the test_when_finished for us.

I thought it (specifically, anything depends on test_when_finished) has trouble working well from inside a subprocess?

SZEDER Gábor· Apr 29, 2022, 19:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote:
Show 54 quoted lines
> Derrick Stolee <derrickstolee@github.com> writes:
> 
> > On 4/27/2022 4:37 PM, Junio C Hamano wrote:
> >> SZEDER Gábor <szeder.dev@gmail.com> writes:
> >> 
> >>> According to the documentation 'safe.directory' "is only respected
> >>> when specified in a system or global config, not when it is specified
> >>> in a repository config or via the command line option -c
> >>> safe.directory=<path>".
> >>>
> >>> Add tests to check that 'safe.directory' in the repository config or
> >>> on the command line is indeed ignored.
> >>>
> >>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> >>> ---
> >>>  t/t0033-safe-directory.sh | 13 +++++++++++++
> >>>  1 file changed, 13 insertions(+)
> >>>
> >>> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> >>> index 6f9680e8b0..82dac0eb93 100755
> >>> --- a/t/t0033-safe-directory.sh
> >>> +++ b/t/t0033-safe-directory.sh
> >>> @@ -16,6 +16,19 @@ test_expect_success 'safe.directory is not set' '
> >>>  	expect_rejected_dir
> >>>  '
> >>>  
> >>> +test_expect_success 'ignoring safe.directory on the command line' '
> >>> +	test_must_fail git -c safe.directory="$(pwd)" status 2>err &&
> >>> +	grep "unsafe repository" err
> >>> +'
> >>> +
> >>> +test_expect_success 'ignoring safe.directory in repo config' '
> >>> +	(
> >>> +		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
> >>> +		git config safe.directory "$(pwd)"
> >>> +	) &&
> >>> +	expect_rejected_dir
> >>> +'
> >> 
> >> I am debating myself if we want to remove the in-repository
> >> safe.directory configuration setting after this test piece is done,
> >> with test_when_finished.  We just made sure, with this test, that
> >> having the variable does not affect anything, so the subsequent
> >> tests should not care hence it is probably OK.  On the other hand,
> >> to make sure those settings they make (e.g. setting it globally is
> >> what the next test we can see below does) is what affects their
> >> outcome, it removes one more thing we need to worry about if we
> >> clean after ourselves.  I dunno.
> >
> > test_config would do the same, right? I think it automatically
> > does the test_when_finished for us.
> 
> I thought it (specifically, anything depends on test_when_finished)
> has trouble working well from inside a subprocess?

Yeah, test_config doesn't work in a subshell, because modifying the variable holding the cleanup commands won't be visible after leaving the subshell, and the protection added in 0968f12a99 (test-lib-functions: detect test_when_finished in subshell, 2015-09-05) will kick in. And we do need a subshell to set the config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git config' would refuse to touch the config file.

I think something like
  test_when_finished "(
        unset GIT_TEST_ASSUME_DIFFERENT_USER &&
        git config --unset safe.directory
        )"
would work, though.

I considered cleaning up the config file, but all subsequent tests leave behind config settings for later tests as well (in the global config file, though).

Derrick Stolee· Apr 29, 2022, 19:19 UTC · re: SZEDER Gábor · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On 4/29/2022 3:06 PM, SZEDER Gábor wrote:
Show 15 quoted lines
> On Fri, Apr 29, 2022 at 10:57:58AM -0700, Junio C Hamano wrote:
>> Derrick Stolee <derrickstolee@github.com> writes:
>>> test_config would do the same, right? I think it automatically
>>> does the test_when_finished for us.
>>
>> I thought it (specifically, anything depends on test_when_finished)
>> has trouble working well from inside a subprocess?
> 
> Yeah, test_config doesn't work in a subshell, because modifying
> the variable holding the cleanup commands won't be visible after
> leaving the subshell, and the protection added in 0968f12a99
> (test-lib-functions: detect test_when_finished in subshell,
> 2015-09-05) will kick in.  And we do need a subshell to set the
> config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git
> config' would refuse to touch the config file.
Ah yes, of course.
Show 8 quoted lines
> I think something like
> 
>   test_when_finished "(
>         unset GIT_TEST_ASSUME_DIFFERENT_USER &&
>         git config --unset safe.directory
>         )"
> 
> would work, though.
Would it be simpler to use this?
	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory

Thanks, -Stolee

SZEDER Gábor· May 10, 2022, 18:33 UTC · re: Derrick Stolee · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:
Show 18 quoted lines
> > And we do need a subshell to set the
> > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git
> > config' would refuse to touch the config file.
> 
> Ah yes, of course.
>  
> > I think something like
> > 
> >   test_when_finished "(
> >         unset GIT_TEST_ASSUME_DIFFERENT_USER &&
> >         git config --unset safe.directory
> >         )"
> > 
> > would work, though.
> 
> Would it be simpler to use this?
> 
> 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
Oh, wow.  This is so obvious, no wonder it didn't occur to me :)
Taylor Blau· May 10, 2022, 19:55 UTC · re: SZEDER Gábor · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote:
Show 21 quoted lines
> On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:
> > > And we do need a subshell to set the
> > > config, because without unsetting GIT_TEST_ASSUME_DIFFERENT_OWNER 'git
> > > config' would refuse to touch the config file.
> >
> > Ah yes, of course.
> >
> > > I think something like
> > >
> > >   test_when_finished "(
> > >         unset GIT_TEST_ASSUME_DIFFERENT_USER &&
> > >         git config --unset safe.directory
> > >         )"
> > >
> > > would work, though.
> >
> > Would it be simpler to use this?
> >
> > 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
>
> Oh, wow.  This is so obvious, no wonder it didn't occur to me :)

Don't we consider this one-shot environment variable to be sticky on some shells (i.e., that it persists beyond just the "git config" invocation here)?

Thanks, Taylor

Carlo Marcelo Arenas Belón· May 10, 2022, 20:06 UTC · re: Taylor Blau · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote:
Show 8 quoted lines
> > >
> > > 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
> >
> > Oh, wow.  This is so obvious, no wonder it didn't occur to me :)
> 
> Don't we consider this one-shot environment variable to be sticky on
> some shells (i.e., that it persists beyond just the "git config"
> invocation here)?

do you have an example of such a shell?, I would assume that since the mechanism to implement these would be similar to local and we already require local for running our tests, that shouldn't be an issue (at least in the test suite), right?

any such variables should be only set as part of the environment used by the posix shell before it call execve to invoke the next command IMHO.

Carlo
Taylor Blau· May 10, 2022, 20:11 UTC · re: Carlo Marcelo Arenas Belón · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On Tue, May 10, 2022 at 01:06:58PM -0700, Carlo Marcelo Arenas Belón wrote:
Show 17 quoted lines
> On Tue, May 10, 2022 at 03:55:23PM -0400, Taylor Blau wrote:
> > > >
> > > > 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
> > >
> > > Oh, wow.  This is so obvious, no wonder it didn't occur to me :)
> >
> > Don't we consider this one-shot environment variable to be sticky on
> > some shells (i.e., that it persists beyond just the "git config"
> > invocation here)?
>
> do you have an example of such a shell?, I would assume that since the
> mechanism to implement these would be similar to local and we already
> require local for running our tests, that shouldn't be an issue (at
> least in the test suite), right?
>
> any such variables should be only set as part of the environment used
> by the posix shell before it call execve to invoke the next command IMHO.

This is completely my mistake, that stickiness exists only when invoking shell _functions_, not other commands (like "git").

I have gotten so used to looking for the former, that I didn't read carefully enough to realize that we are in the latter situation instead.

> Carlo

Thanks, Taylor

Eric Sunshine· May 10, 2022, 20:18 UTC · re: Taylor Blau · lore

Re: [PATCH 2/3] t0033-safe-directory: check when 'safe.directory' is ignored

On 5/10/22 3:55 PM, Taylor Blau wrote:
Show 11 quoted lines
> On Tue, May 10, 2022 at 08:33:21PM +0200, SZEDER Gábor wrote:
>> On Fri, Apr 29, 2022 at 03:19:01PM -0400, Derrick Stolee wrote:
>>> Would it be simpler to use this?
>>>
>>> 	GIT_TEST_ASSUME_DIFFERENT_USER=0 git config --unset safe.directory
>>
>> Oh, wow.  This is so obvious, no wonder it didn't occur to me :)
> 
> Don't we consider this one-shot environment variable to be sticky on
> some shells (i.e., that it persists beyond just the "git config"
> invocation here)?

Almost. Some old/buggy shells incorrectly allow the one-shot environment variable assignment to bleed past command invocation when the "command" is a shell function, like this:

     SOME_ENV_VAR=somevalue shell_function_call
We do "lint"[*] for that sort of problem in t/check-non-portable-shell.pl.

Stolee's example, however, is invoking a normal program (`git`, in this case), so isn't subject to that unwanted behavior from buggy shells.

[*]: a0a630192d (t/check-non-portable-shell: detect "FOO=bar shell_func", 2018-07-13)

SZEDER Gábor· Apr 27, 2022, 17:06 UTC · re: SZEDER Gábor · lore

[PATCH 3/3] safe.directory: document and check that it's ignored in the environment

The description of 'safe.directory' mentions that it's respected in the system and global configs, and ignored in the repository config and on the command line, but it doesn't mention whether it's respected or ignored when specified via environment variables (nor does the commit message adding 'safe.directory' [1]).

Clarify that 'safe.directory' is ignored when specified in the environment, and add tests to make sure that it remains so.

[1] 8959555cee (setup_git_directory(): add an owner check for the
                top-level directory, 2022-03-02)
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/config/safe.txt |  4 ++--
 t/t0033-safe-directory.sh     | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)
Show changes to 2 files +17 −2

Documentation/config/safe.txt, t/t0033-safe-directory.sh

diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
index 6d764fe0cc..ae0e2e3bdb 100644
--- a/Documentation/config/safe.txt
+++ b/Documentation/config/safe.txt
@@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
 `safe.directory` entry with an empty value.
 +
 This config setting is only respected when specified in a system or global
-config, not when it is specified in a repository config or via the command
-line option `-c safe.directory=<path>`.
+config, not when it is specified in a repository config, via the command
+line option `-c safe.directory=<path>`, or in environment variables.
 +
 The value of this setting is interpolated, i.e. `~/<path>` expands to a
 path relative to the home directory and `%(prefix)/<path>` expands to a
diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
index 82dac0eb93..238b25f91a 100755
--- a/t/t0033-safe-directory.sh
+++ b/t/t0033-safe-directory.sh
@@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' '
 	grep "unsafe repository" err
 '
 
+test_expect_success 'ignoring safe.directory in the environment' '
+	test_must_fail env GIT_CONFIG_COUNT=1 \
+		GIT_CONFIG_KEY_0="safe.directory" \
+		GIT_CONFIG_VALUE_0="$(pwd)" \
+		git status 2>err &&
+	grep "unsafe repository" err
+'
+
+test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '
+	test_must_fail env \
+		GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
+		git status 2>err &&
+	grep "unsafe repository" err
+'
+
 test_expect_success 'ignoring safe.directory in repo config' '
 	(
 		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&

-- 
2.36.0.676.gf39b21ed98
Junio C Hamano· Apr 27, 2022, 20:42 UTC · re: SZEDER Gábor · lore

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 5 quoted lines
> The description of 'safe.directory' mentions that it's respected in
> the system and global configs, and ignored in the repository config
> and on the command line, but it doesn't mention whether it's respected
> or ignored when specified via environment variables (nor does the
> commit message adding 'safe.directory' [1]).

If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like $PATH does), that would have been absolutely necessary to document how it works, but GIT_CONFIG_* is merely an implementation detail of how "git -c var=val" works and I am not sure if it is even a good idea to hardcode how they happen to work like these tests. The only thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are used internally by the implementation and they should not muck with it, no?

I am moderately negative about this change.
Show 26 quoted lines
> diff --git a/t/t0033-safe-directory.sh b/t/t0033-safe-directory.sh
> index 82dac0eb93..238b25f91a 100755
> --- a/t/t0033-safe-directory.sh
> +++ b/t/t0033-safe-directory.sh
> @@ -21,6 +21,21 @@ test_expect_success 'ignoring safe.directory on the command line' '
>  	grep "unsafe repository" err
>  '
>  
> +test_expect_success 'ignoring safe.directory in the environment' '
> +	test_must_fail env GIT_CONFIG_COUNT=1 \
> +		GIT_CONFIG_KEY_0="safe.directory" \
> +		GIT_CONFIG_VALUE_0="$(pwd)" \
> +		git status 2>err &&
> +	grep "unsafe repository" err
> +'
> +
> +test_expect_success 'ignoring safe.directory in GIT_CONFIG_PARAMETERS' '
> +	test_must_fail env \
> +		GIT_CONFIG_PARAMETERS="${SQ}safe.directory${SQ}=${SQ}$(pwd)${SQ}" \
> +		git status 2>err &&
> +	grep "unsafe repository" err
> +'
> +
>  test_expect_success 'ignoring safe.directory in repo config' '
>  	(
>  		unset GIT_TEST_ASSUME_DIFFERENT_OWNER &&
Junio C Hamano· Apr 27, 2022, 20:49 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

Junio C Hamano <gitster@pobox.com> writes:
Show 8 quoted lines
> If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
> $PATH does), that would have been absolutely necessary to document
> how it works, but GIT_CONFIG_* is merely an implementation detail of
> how "git -c var=val" works and I am not sure if it is even a good
> idea to hardcode how they happen to work like these tests.  The only
> thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
> used internally by the implementation and they should not muck with
> it, no?

I misremembered. GIT_CONFIG_COUNT and stuff are usable by end user scripts, but then ...

Show 12 quoted lines
> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> index 6d764fe0cc..ae0e2e3bdb 100644
> --- a/Documentation/config/safe.txt
> +++ b/Documentation/config/safe.txt
> @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
>  `safe.directory` entry with an empty value.
>  +
>  This config setting is only respected when specified in a system or global
> -config, not when it is specified in a repository config or via the command
> -line option `-c safe.directory=<path>`.
> +config, not when it is specified in a repository config, via the command
> +line option `-c safe.directory=<path>`, or in environment variables.

... this part must clarify what environment variables it is talking about.

    ... via the command line option `-c safe.directory=<path>`, or
    via the GIT_CONFIG_{KEY,VALUE} mechanism.

or something, perhaps. I actually do think it is a useful addition to have GIT_SAFE_DIRECTORIES environment variable that should NOT be ignored.

Derrick Stolee· Apr 29, 2022, 16:13 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

On 4/27/2022 4:49 PM, Junio C Hamano wrote:
Show 36 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
>> $PATH does), that would have been absolutely necessary to document
>> how it works, but GIT_CONFIG_* is merely an implementation detail of
>> how "git -c var=val" works and I am not sure if it is even a good
>> idea to hardcode how they happen to work like these tests.  The only
>> thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
>> used internally by the implementation and they should not muck with
>> it, no?
> 
> I misremembered.  GIT_CONFIG_COUNT and stuff are usable by end user
> scripts, but then ...
> 
>> diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
>> index 6d764fe0cc..ae0e2e3bdb 100644
>> --- a/Documentation/config/safe.txt
>> +++ b/Documentation/config/safe.txt
>> @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
>>  `safe.directory` entry with an empty value.
>>  +
>>  This config setting is only respected when specified in a system or global
>> -config, not when it is specified in a repository config or via the command
>> -line option `-c safe.directory=<path>`.
>> +config, not when it is specified in a repository config, via the command
>> +line option `-c safe.directory=<path>`, or in environment variables.
> 
> ... this part must clarify what environment variables it is talking
> about.
> 
>     ... via the command line option `-c safe.directory=<path>`, or
>     via the GIT_CONFIG_{KEY,VALUE} mechanism.
> 
> or something, perhaps.  I actually do think it is a useful addition
> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be
> ignored.
I agree on both points.

Thanks for working on this! -Stolee

SZEDER Gábor· May 9, 2022, 21:39 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

On Wed, Apr 27, 2022 at 01:49:32PM -0700, Junio C Hamano wrote:
Show 32 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > If we had GIT_SAFE_DIRECTORIES that lists the safe directories (like
> > $PATH does), that would have been absolutely necessary to document
> > how it works, but GIT_CONFIG_* is merely an implementation detail of
> > how "git -c var=val" works and I am not sure if it is even a good
> > idea to hardcode how they happen to work like these tests.  The only
> > thing the users should know is that GIT_CONFIG_{KEY,VALUE}_* are
> > used internally by the implementation and they should not muck with
> > it, no?
> 
> I misremembered.  GIT_CONFIG_COUNT and stuff are usable by end user
> scripts, but then ...
> 
> > diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt
> > index 6d764fe0cc..ae0e2e3bdb 100644
> > --- a/Documentation/config/safe.txt
> > +++ b/Documentation/config/safe.txt
> > @@ -13,8 +13,8 @@ override any such directories specified in the system config), add a
> >  `safe.directory` entry with an empty value.
> >  +
> >  This config setting is only respected when specified in a system or global
> > -config, not when it is specified in a repository config or via the command
> > -line option `-c safe.directory=<path>`.
> > +config, not when it is specified in a repository config, via the command
> > +line option `-c safe.directory=<path>`, or in environment variables.
> 
> ... this part must clarify what environment variables it is talking
> about.
> 
>     ... via the command line option `-c safe.directory=<path>`, or
>     via the GIT_CONFIG_{KEY,VALUE} mechanism.

Well, the proposed phrasing covers all environment variables that affect the configuration.

It doesn't feel right to explicitly list all those variables, including GIT_CONFIG_PARAMETERS, because that vairable is such an implementation detail that it isn't even mentioned elsewhere in the documentation at all. OTOH, listing only some of the ignored variables but omitting others doesn't feel quite right either, hence the general "in environment variables".

> or something, perhaps.  I actually do think it is a useful addition
> to have GIT_SAFE_DIRECTORIES environment variable that should NOT be
> ignored.
Is it safe to have such an environment variable?

So, it's quite clear why 'safe.directory' is ignored in the repository config: it can be under control of someone else, and thus a malicious repositoy could trivially safe-list itself. However, it's unclear (to me) why 'git -c safe.directory=...' is ignored: 8959555cee (setup_git_directory(): add an owner check for the top-level directory, 2022-03-02) only states that it's ignored, but doesn't justify why. Now, let's suppose that there was a compelling reason to do so, e.g. in some way that I don't readily see it can be spoofed and thus could be abused by an attacker to safe-list a malicious repository. If that were indeed the case, then couldn't 'GIT_SAFE_DIRECTORIES=/path/... git cmd' could be spoofed and abused just as easily?

Or to approach it from the opposite direction: if such a GIT_SAFE_DIRECTORIES environment variable is safe, then why should we ignore 'safe.directory' in the command line, or in the environment for that matter?

Junio C Hamano· May 9, 2022, 21:45 UTC · re: SZEDER Gábor · lore

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 5 quoted lines
> repositoy could trivially safe-list itself.  However, it's unclear (to
> me) why 'git -c safe.directory=...' is ignored: 8959555cee
> (setup_git_directory(): add an owner check for the top-level
> directory, 2022-03-02) only states that it's ignored, but doesn't
> justify why.  Now, let's suppose that there was a compelling reason to

Correct. I do not think it is a restriction with any sensible justification, just that it happened to be implemented that way.

IOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user interface than fixing the "we ignore 'git -c safe.directory'?" bug.

SZEDER Gábor· May 10, 2022, 18:48 UTC · re: Junio C Hamano · lore

Re: [PATCH 3/3] safe.directory: document and check that it's ignored in the environment

On Mon, May 09, 2022 at 02:45:21PM -0700, Junio C Hamano wrote:
Show 13 quoted lines
> SZEDER Gábor <szeder.dev@gmail.com> writes:
> 
> > repositoy could trivially safe-list itself.  However, it's unclear (to
> > me) why 'git -c safe.directory=...' is ignored: 8959555cee
> > (setup_git_directory(): add an owner check for the top-level
> > directory, 2022-03-02) only states that it's ignored, but doesn't
> > justify why.  Now, let's suppose that there was a compelling reason to
> 
> Correct.  I do not think it is a restriction with any sensible
> justification, just that it happened to be implemented that way.
> 
> IOW, I am saying that GIT_SAFE_DIRECTORIES may be a lot nicer user
> interface than fixing the "we ignore 'git -c safe.directory'?"  bug.

In light of this, I'm not sure we want to test said buggy behavior. IOW, do we want 2/3 and 3/3 of this patch series at all? Or perhaps add those test as 'test_expect_failure' instead?

← back to recent threads