From fd44b5f32ee05ae6a25442db70b038a4c4e2f89f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 30 Mar 2022 11:58:37 +0200 Subject: [PATCH 1/3] Allow debugging unsafe directories' ownership When Git refuses to use an existing repository because it is owned by someone else than the current user, it can be a bit tricky on Windows to figure out what is going on. Let's help with that by offering some more information via the environment variable `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`. Signed-off-by: Johannes Schindelin --- Documentation/config/safe.txt | 6 ++++++ compat/mingw.c | 21 +++++++++++++++++++++ setup.c | 14 ++++++++++++-- 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/Documentation/config/safe.txt b/Documentation/config/safe.txt index fa02f3ccc5..ec9c85c8f8 100644 --- a/Documentation/config/safe.txt +++ b/Documentation/config/safe.txt @@ -40,3 +40,9 @@ which id the original user has. If that is not what you would prefer and want git to only trust repositories that are owned by root instead, then you can remove the `SUDO_UID` variable from root's environment before invoking git. ++ +Due to the permission model on Windows where ACLs are used instead of +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`. diff --git a/compat/mingw.c b/compat/mingw.c index 2607de93af..719ef8c51f 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1,6 +1,7 @@ #include "../git-compat-util.h" #include "win32.h" #include +#include #include #include #include "../strbuf.h" @@ -2720,6 +2721,26 @@ int is_path_owned_by_current_sid(const char *path) IsValidSid(current_user_sid) && EqualSid(sid, current_user_sid)) result = 1; + else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { + LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; + + if (ConvertSidToStringSidA(sid, &str1)) + to_free1 = str1; + else + str1 = "(inconvertible)"; + + if (!current_user_sid) + str2 = "(none)"; + else if (!IsValidSid(current_user_sid)) + str2 = "(invalid)"; + else if (ConvertSidToStringSidA(current_user_sid, &str2)) + to_free2 = str2; + else + str2 = "(inconvertible)"; + warning("'%s' is owned by:\n\t'%s'\nbut the current user is:\n\t'%s'", path, str1, str2); + LocalFree(to_free1); + LocalFree(to_free2); + } } /* diff --git a/setup.c b/setup.c index 7f64f34477..bf4c7badd3 100644 --- a/setup.c +++ b/setup.c @@ -1433,13 +1433,23 @@ const char *setup_git_directory_gently(int *nongit_ok) case GIT_DIR_INVALID_OWNERSHIP: if (!nongit_ok) { struct strbuf quoted = STRBUF_INIT; + struct strbuf hint = STRBUF_INIT; + +#ifdef __MINGW32__ + if (!git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) + strbuf_addstr(&hint, + _("\n\nSet the environment variable " + "GIT_TEST_DEBUG_UNSAFE_DIRECTORIES=true " + "and run\n" + "again for more information.")); +#endif sq_quote_buf_pretty("ed, dir.buf); die(_("detected dubious ownership in repository at '%s'\n" "To add an exception for this directory, call:\n" "\n" - "\tgit config --global --add safe.directory %s"), - dir.buf, quoted.buf); + "\tgit config --global --add safe.directory %s%s"), + dir.buf, quoted.buf, hint.buf); } *nongit_ok = 1; break; From e8e6aa08f79582b6cd58602db6d3987779320d60 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 30 Mar 2022 15:31:52 +0200 Subject: [PATCH 2/3] mingw: handle a file owned by the Administrators group correctly When an Administrator creates a file or directory, the created file/directory is owned not by the Administrator SID, but by the _Administrators Group_ SID. The reason is that users with administrator privileges usually run in unprivileged ("non-elevated") mode, and their user SID does not change when running in elevated mode. This is is relevant e.g. when running a GitHub workflow on a build agent, which runs in elevated mode: cloning a Git repository in a script step will cause the worktree to be owned by the Administrators Group SID, for example. Let's handle this case as following: if the current user is an administrator, Git should consider a worktree owned by the Administrators Group as if it were owned by said user. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/compat/mingw.c b/compat/mingw.c index 719ef8c51f..ec21c4dc43 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2713,6 +2713,7 @@ int is_path_owned_by_current_sid(const char *path) else if (sid && IsValidSid(sid)) { /* Now, verify that the SID matches the current user's */ static PSID current_user_sid; + BOOL is_member; if (!current_user_sid) current_user_sid = get_current_user_sid(); @@ -2721,6 +2722,15 @@ int is_path_owned_by_current_sid(const char *path) IsValidSid(current_user_sid) && EqualSid(sid, current_user_sid)) result = 1; + else if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) && + CheckTokenMembership(NULL, sid, &is_member) && + is_member) + /* + * If owned by the Administrators group, and the + * current user is an administrator, we consider that + * okay, too. + */ + result = 1; else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; From 793c2bbc1ceac03ad785fc74a3a92db97e22b533 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 8 Jun 2022 11:56:03 +0200 Subject: [PATCH 3/3] mingw: be more informative when ownership check fails on FAT32 The FAT file system has no concept of ACLs. Therefore, it cannot store any ownership information anyway, and the `GetNamedSecurityInfoW()` call pretends that everything is owned "by the world". Let's special-case that scenario and tell the user what's going on, at least when they set `GIT_TEST_DEBUG_UNSAFE_DIRECTORIES`. This addresses https://github.com/git-for-windows/git/issues/3886 Signed-off-by: Johannes Schindelin --- compat/mingw.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index ec21c4dc43..5af585098b 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -2674,6 +2674,22 @@ static PSID get_current_user_sid(void) return result; } +static int acls_supported(const char *path) +{ + size_t offset = offset_1st_component(path); + WCHAR wroot[MAX_PATH]; + DWORD file_system_flags; + + if (offset && + xutftowcs_path_ex(wroot, path, MAX_PATH, offset, + MAX_PATH, 0) > 0 && + GetVolumeInformationW(wroot, NULL, 0, NULL, NULL, + &file_system_flags, NULL, 0)) + return !!(file_system_flags & FILE_PERSISTENT_ACLS); + + return 0; +} + int is_path_owned_by_current_sid(const char *path) { WCHAR wpath[MAX_PATH]; @@ -2731,7 +2747,14 @@ int is_path_owned_by_current_sid(const char *path) * okay, too. */ result = 1; - else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { + else if (IsWellKnownSid(sid, WinWorldSid) && + git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0) && + !acls_supported(path)) { + /* + * On FAT32 volumes, ownership is not actually recorded. + */ + warning("'%s' is on a file system that does not record ownership", path); + } else if (git_env_bool("GIT_TEST_DEBUG_UNSAFE_DIRECTORIES", 0)) { LPSTR str1, str2, to_free1 = NULL, to_free2 = NULL; if (ConvertSidToStringSidA(sid, &str1))