Skip to content

Commit f2568a3

Browse files
dschoGit for Windows Build Agent
authored and
Git for Windows Build Agent
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 <johannes.schindelin@gmx.de>
1 parent 1b935bc commit f2568a3

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
@@ -3313,6 +3313,7 @@ static int is_valid_system_file_owner(PSID sid, TOKEN_USER **info)
33133313
{
33143314
HANDLE token;
33153315
DWORD len;
3316+
char builtin_administrators_sid[SECURITY_MAX_SID_SIZE];
33163317

33173318
if (IsWellKnownSid(sid, WinBuiltinAdministratorsSid) ||
33183319
IsWellKnownSid(sid, WinLocalSystemSid))
@@ -3330,7 +3331,63 @@ static int is_valid_system_file_owner(PSID sid, TOKEN_USER **info)
33303331
CloseHandle(token);
33313332
}
33323333

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

33363393
/*

0 commit comments

Comments
 (0)