From 1bdb9e35c041fdba60f110cc35dc850bbe041e42 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 13 Apr 2022 11:08:28 -0400 Subject: [PATCH 1/5] t0033: add tests for safe.directory 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 --- setup.c | 3 ++- t/t0033-safe-directory.sh | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) create mode 100755 t/t0033-safe-directory.sh diff --git a/setup.c b/setup.c index c8f67bfed5206f..f54f449008a09f 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 00000000000000..9380ff3d017096 --- /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 From 5d60f3c4e1760fd95f7dbe537e99dc2b800312c4 Mon Sep 17 00:00:00 2001 From: Matheus Valadares Date: Tue, 12 Apr 2022 19:39:33 -0700 Subject: [PATCH 2/5] setup: fix safe.directory key not being checked 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 Signed-off-by: Derrick Stolee --- setup.c | 3 +++ t/t0033-safe-directory.sh | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/setup.c b/setup.c index f54f449008a09f..a995c359c3223f 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 9380ff3d017096..6f33c0dfefaaf3 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 From 93310f0ac09c297ded0cd870d6ee637367f28e06 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 13 Apr 2022 10:54:32 -0400 Subject: [PATCH 3/5] setup: opt-out of check with safe.directory=* 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 --- Documentation/config/safe.txt | 7 +++++++ setup.c | 6 ++++-- t/t0033-safe-directory.sh | 10 ++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index 9e57855f27c799..57ef566a3f735f 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -25,3 +25,10 @@ Unix' simpler permission model, it can be a bit tricky to figure out why a directory is considered unsafe. To help with this, Git will provide more detailed information when the environment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES` is set to `true`. ++ +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 a995c359c3223f..a42b21307f7708 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 6f33c0dfefaaf3..239d93f4d21141 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 From e64daf281c3a4f96677b2f89de3f5c1bc728f851 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 13 Apr 2022 14:49:17 -0400 Subject: [PATCH 4/5] setup: properly use "%(prefix)/" when in WSL Signed-off-by: Derrick Stolee --- setup.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/setup.c b/setup.c index a42b21307f7708..3f66dd5173ae03 100644 --- a/setup.c +++ b/setup.c @@ -1374,9 +1374,17 @@ const char *setup_git_directory_gently(int *nongit_ok) break; case GIT_DIR_INVALID_OWNERSHIP: if (!nongit_ok) { + struct strbuf prequoted = STRBUF_INIT; struct strbuf quoted = STRBUF_INIT; - sq_quote_buf_pretty("ed, dir.buf); +#ifdef __MINGW32__ + if (dir.buf[0] == '/') + strbuf_addstr(&prequoted, "%(prefix)/"); +#endif + + strbuf_add(&prequoted, dir.buf, dir.len); + sq_quote_buf_pretty("ed, prequoted.buf); + die(_("unsafe repository ('%s' is owned by someone else)\n" "To add an exception for this directory, call:\n" "\n" From 109ae350a0d1b1c67a83e2c34ca31497f63cee69 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Wed, 13 Apr 2022 14:54:43 -0400 Subject: [PATCH 5/5] compat/mingw.c: do not warn when failing to get owner In the case of Git for Windows (say, in a Git Bash window) running in a Windows Subsystem for Linux (WSL) directory, the GetNamedSecurityInfoW() call in is_path_owned_By_current_side() returns an error code other than ERROR_SUCCESS. This is consistent behavior across this boundary. In these cases, the owner would always be different because the WSL owner is a different entity than the Windows user. The change here is to suppress the error message that looks like this: error: failed to get owner for '//wsl.localhost/...' (1) Before this change, this warning happens for every Git command, regardless of whether the directory is marked with safe.directory. Signed-off-by: Derrick Stolee --- compat/mingw.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index ea34ec5f6d2490..f3acdc57ad1538 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -3509,9 +3509,7 @@ int is_path_owned_by_current_sid(const char *path) DACL_SECURITY_INFORMATION, &sid, NULL, NULL, NULL, &descriptor); - if (err != ERROR_SUCCESS) - error(_("failed to get owner for '%s' (%ld)"), path, err); - else if (sid && IsValidSid(sid)) { + if (err == ERROR_SUCCESS && sid && IsValidSid(sid)) { /* Now, verify that the SID matches the current user's */ static PSID current_user_sid; BOOL is_member;