Skip to content

Commit

Permalink
config: treat any member of BUILTIN\Administrators as trusted
Browse files Browse the repository at this point in the history
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 #2304.

Signed-off-by: Johannes Schindelin <[email protected]>
  • Loading branch information
dscho committed Sep 18, 2019
1 parent 2608f2e commit 8f8ffd9
Showing 1 changed file with 58 additions and 1 deletion.
59 changes: 58 additions & 1 deletion compat/mingw.c
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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;
}

/*
Expand Down

0 comments on commit 8f8ffd9

Please sign in to comment.