From 2608f2eaa778c9129a9c761b3d6ac6123fa20b8a Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 17 Sep 2019 23:48:39 +0200 Subject: [PATCH 1/2] fixup! Windows: add support for a Windows-wide configuration Let's refactor the `validate_system_file_ownership()` a bit, not only to clarify what is done, but also to prepare for maybe adding a slighty more expensive check: it is possible that the file `C:\ProgramData\Git\config` is owned by the local administrator (who has a different SID than the `Administrators` group). Signed-off-by: Johannes Schindelin --- compat/mingw.c | 104 ++++++++++++++++++++++++++----------------------- 1 file changed, 56 insertions(+), 48 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index e841b2bca8cbb8..4ba2a3b14378d5 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -3299,6 +3299,40 @@ int uname(struct utsname *buf) return 0; } +/* + * Determines whether the SID refers to an administrator or the current user. + * + * For convenience, the `info` parameter allows avoiding multiple calls to + * `OpenProcessToken()` if this function is called more than once. The initial + * value of `*info` is expected to be `NULL`, and it needs to be released via + * `free()` after the last call to this function. + * + * Returns 0 if the SID indicates a dubious owner of system files, otherwise 1. + */ +static int is_valid_system_file_owner(PSID sid, TOKEN_USER **info) +{ + HANDLE token; + DWORD len; + + if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) || + IsWellKnownSid(sid, WinLocalSystemSid)) + return 1; + + /* Obtain current user's SID */ + if (!*info && + OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &token)) { + if (!GetTokenInformation(token, TokenUser, NULL, 0, &len)) { + *info = xmalloc((size_t)len); + if (!GetTokenInformation(token, TokenUser, *info, len, + &len)) + FREE_AND_NULL(*info); + } + CloseHandle(token); + } + + return *info && EqualSid(sid, (*info)->User.Sid) ? 1 : 0; +} + /* * Verify that the file in question is owned by an administrator or system * account, or at least by the current user. @@ -3309,11 +3343,10 @@ int uname(struct utsname *buf) static int validate_system_file_ownership(const char *path) { WCHAR wpath[MAX_LONG_PATH]; - PSID owner_sid = NULL; + PSID owner_sid = NULL, problem_sid = NULL; PSECURITY_DESCRIPTOR descriptor = NULL; - HANDLE token; TOKEN_USER* info = NULL; - DWORD err, len; + DWORD err; int ret; if (xutftowcs_long_path(wpath, path) < 0) @@ -3325,63 +3358,37 @@ static int validate_system_file_ownership(const char *path) &owner_sid, NULL, NULL, NULL, &descriptor); /* if the file does not exist, it does not have a valid owner */ - if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) { + if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) ret = 0; - owner_sid = NULL; - goto finish_validation; - } - - if (err != ERROR_SUCCESS) { + else if (err != ERROR_SUCCESS) ret = error(_("failed to validate '%s' (%ld)"), path, err); - owner_sid = NULL; - goto finish_validation; - } - - if (!IsValidSid(owner_sid)) { + else if (!IsValidSid(owner_sid)) ret = error(_("invalid owner: '%s'"), path); - goto finish_validation; - } - - if (IsWellKnownSid(owner_sid, WinBuiltinAdministratorsSid) || - IsWellKnownSid(owner_sid, WinLocalSystemSid)) { + else if (is_valid_system_file_owner(owner_sid, &info)) ret = 1; - goto finish_validation; - } - - /* Obtain current user's SID */ - if (OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &token) && - !GetTokenInformation(token, TokenUser, NULL, 0, &len)) { - info = xmalloc((size_t)len); - if (!GetTokenInformation(token, TokenUser, info, len, &len)) - FREE_AND_NULL(info); - } - - if (!info) - ret = 0; else { - ret = EqualSid(owner_sid, info->User.Sid) ? 1 : 0; - free(info); + ret = 0; + problem_sid = owner_sid; } -finish_validation: - if (!ret && owner_sid) { + if (!ret && problem_sid) { #define MAX_NAME_OR_DOMAIN 256 - wchar_t owner_name[MAX_NAME_OR_DOMAIN]; - wchar_t owner_domain[MAX_NAME_OR_DOMAIN]; + wchar_t name[MAX_NAME_OR_DOMAIN]; + wchar_t domain[MAX_NAME_OR_DOMAIN]; wchar_t *p = NULL; DWORD size = MAX_NAME_OR_DOMAIN; SID_NAME_USE type; - char name[3 * MAX_NAME_OR_DOMAIN + 1]; + char utf[3 * MAX_NAME_OR_DOMAIN + 1]; - if (!LookupAccountSidW(NULL, owner_sid, owner_name, &size, - owner_domain, &size, &type) || - xwcstoutf(name, owner_name, ARRAY_SIZE(name)) < 0) { - if (!ConvertSidToStringSidW(owner_sid, &p)) - strlcpy(name, "(unknown)", ARRAY_SIZE(name)); + if (!LookupAccountSidW(NULL, problem_sid, name, &size, + domain, &size, &type) || + xwcstoutf(utf, name, ARRAY_SIZE(utf)) < 0) { + if (!ConvertSidToStringSidW(problem_sid, &p)) + strlcpy(utf, "(unknown)", ARRAY_SIZE(utf)); else { - if (xwcstoutf(name, p, ARRAY_SIZE(name)) < 0) - strlcpy(name, "(some user)", - ARRAY_SIZE(name)); + if (xwcstoutf(utf, p, ARRAY_SIZE(utf)) < 0) + strlcpy(utf, "(some user)", + ARRAY_SIZE(utf)); LocalFree(p); } } @@ -3390,11 +3397,12 @@ static int validate_system_file_ownership(const char *path) "For security reasons, it is therefore ignored.\n" "To fix this, please transfer ownership to an " "admininstrator."), - path, name); + path, utf); } if (descriptor) LocalFree(descriptor); + free(info); return ret; } From 8f8ffd9a00ad93981149d48d2f9966173587fa90 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 18 Sep 2019 10:56:04 +0200 Subject: [PATCH 2/2] config: treat any member of BUILTIN\Administrators as trusted We recently hardened Git for Windows by ignoring `C:\ProgramData\Git\config` files unless they are owned by the system account, by the administrators group, or by the account under which Git is running. Turns out that there are situations when that config file is owned by _an_ administrator, not by the entire administrators group, and that still is okay. This can happen very easily e.g. in Docker Containers. Let's add a fall back when the owner is none of the three currently expected ones, enumerating the members of the administrators group and comparing them to the file's owner. If a match is found, the owner is not dubious. Enumerating groups' members on Windows is not exactly a cheap operation, and it requires linking to the `netapi32.dll`. Since this is rarely needed, and since this is done at most a handful of times during any Git process' life cycle, it is okay that it is a bit expensive. To avoid the startup cost of linking to yet another DLL, we do this lazily instead: that way, the vast majority of Git for Windows' users will not feel any impact by this patch. This fixes https://github.com/git-for-windows/git/issues/2304. Signed-off-by: Johannes Schindelin --- compat/mingw.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/compat/mingw.c b/compat/mingw.c index 4ba2a3b14378d5..463dcc54856c7b 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -3313,6 +3313,7 @@ static int is_valid_system_file_owner(PSID sid, TOKEN_USER **info) { HANDLE token; DWORD len; + char builtin_administrators_sid[SECURITY_MAX_SID_SIZE]; if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) || IsWellKnownSid(sid, WinLocalSystemSid)) @@ -3330,7 +3331,63 @@ static int is_valid_system_file_owner(PSID sid, TOKEN_USER **info) CloseHandle(token); } - return *info && EqualSid(sid, (*info)->User.Sid) ? 1 : 0; + if (*info && EqualSid(sid, (*info)->User.Sid)) + return 1; + + /* Is the owner at least a member of BUILTIN\Administrators? */ + len = ARRAY_SIZE(builtin_administrators_sid); + if (CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, + builtin_administrators_sid, &len)) { + wchar_t name[256], domain[256]; + DWORD name_size = ARRAY_SIZE(name); + DWORD domain_size = ARRAY_SIZE(domain); + SID_NAME_USE type; + PSID *members; + DWORD dummy, i; + /* + * We avoid including the `lm.h` header and linking to + * `netapi32.dll` directly, in favor of lazy-loading that DLL + * when, and _only_ when, needed. + */ + DECLARE_PROC_ADDR(netapi32.dll, DWORD, + NetLocalGroupGetMembers, LPCWSTR, + LPCWSTR, DWORD, LPVOID, DWORD, + LPDWORD, LPDWORD, PDWORD_PTR); + DECLARE_PROC_ADDR(netapi32.dll, DWORD, + NetApiBufferFree, LPVOID); + + if (LookupAccountSidW(NULL, builtin_administrators_sid, + name, &name_size, domain, &domain_size, + &type) && + INIT_PROC_ADDR(NetLocalGroupGetMembers) && + /* + * Technically, `NetLocalGroupGetMembers()` wants to assign + * an array of type `LOCALGROUP_MEMBERS_INFO_0`, which + * however contains only one field of type `PSID`, + * therefore we can pretend that it is an array over the + * type `PSID`. + * + * Also, we simply ignore the condition where + * `ERROR_MORE_DATA` is returned; This should not happen + * anyway, as we are passing `-1` as `prefmaxlen` + * parameter, which is equivalent to the constant + * `MAX_PREFERRED_LENGTH`. + */ + !NetLocalGroupGetMembers(NULL, name, 0, &members, -1, + &len, &dummy, NULL)) { + for (i = 0; i < len; i++) + if (EqualSid(sid, members[i])) + break; + + if (INIT_PROC_ADDR(NetApiBufferFree)) + NetApiBufferFree(members); + + /* Did we find the `sid` in the members? */ + return i < len; + } + } + + return 0; } /*