Skip to content

Commit ed78197

Browse files
committed
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 #2304. Signed-off-by: Johannes Schindelin <[email protected]>
1 parent 581faa0 commit ed78197

File tree

1 file changed

+58
-1
lines changed

1 file changed

+58
-1
lines changed

compat/mingw.c

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3308,6 +3308,7 @@ static int is_valid_system_file_owner(PSID sid, TOKEN_USER **info)
33083308
{
33093309
HANDLE token;
33103310
DWORD len;
3311+
char builtin_administrators_sid[SECURITY_MAX_SID_SIZE];
33113312

33123313
if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) ||
33133314
IsWellKnownSid(sid, WinLocalSystemSid))
@@ -3325,7 +3326,63 @@ static int is_valid_system_file_owner(PSID sid, TOKEN_USER **info)
33253326
CloseHandle(token);
33263327
}
33273328

3328-
return *info && EqualSid(sid, (*info)->User.Sid) ? 1 : 0;
3329+
if (*info && EqualSid(sid, (*info)->User.Sid))
3330+
return 1;
3331+
3332+
/* Is the owner at least a member of BUILTIN\Administrators? */
3333+
len = ARRAY_SIZE(builtin_administrators_sid);
3334+
if (CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL,
3335+
builtin_administrators_sid, &len)) {
3336+
wchar_t name[256], domain[256];
3337+
DWORD name_size = ARRAY_SIZE(name);
3338+
DWORD domain_size = ARRAY_SIZE(domain);
3339+
SID_NAME_USE type;
3340+
PSID *members;
3341+
DWORD dummy, i;
3342+
/*
3343+
* We avoid including the `lm.h` header and linking to
3344+
* `netapi32.dll` directly, in favor of lazy-loading that DLL
3345+
* when, and _only_ when, needed.
3346+
*/
3347+
DECLARE_PROC_ADDR(netapi32.dll, DWORD,
3348+
NetLocalGroupGetMembers, LPCWSTR,
3349+
LPCWSTR, DWORD, LPVOID, DWORD,
3350+
LPDWORD, LPDWORD, PDWORD_PTR);
3351+
DECLARE_PROC_ADDR(netapi32.dll, DWORD,
3352+
NetApiBufferFree, LPVOID);
3353+
3354+
if (LookupAccountSidW(NULL, builtin_administrators_sid,
3355+
name, &name_size, domain, &domain_size,
3356+
&type) &&
3357+
INIT_PROC_ADDR(NetLocalGroupGetMembers) &&
3358+
/*
3359+
* Technically, `NetLocalGroupGetMembers()` wants to assign
3360+
* an array of type `LOCALGROUP_MEMBERS_INFO_0`, which
3361+
* however contains only one field of type `PSID`,
3362+
* therefore we can pretend that it is an array over the
3363+
* type `PSID`.
3364+
*
3365+
* Also, we simply ignore the condition where
3366+
* `ERROR_MORE_DATA` is returned; This should not happen
3367+
* anyway, as we are passing `-1` as `prefmaxlen`
3368+
* parameter, which is equivalent to the constant
3369+
* `MAX_PREFERRED_LENGTH`.
3370+
*/
3371+
!NetLocalGroupGetMembers(NULL, name, 0, &members, -1,
3372+
&len, &dummy, NULL)) {
3373+
for (i = 0; i < len; i++)
3374+
if (EqualSid(sid, members[i]))
3375+
break;
3376+
3377+
if (INIT_PROC_ADDR(NetApiBufferFree))
3378+
NetApiBufferFree(members);
3379+
3380+
/* Did we find the `sid` in the members? */
3381+
return i < len;
3382+
}
3383+
}
3384+
3385+
return 0;
33293386
}
33303387

33313388
/*

0 commit comments

Comments
 (0)