Re: Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

On Tue, Nov 8, 2016 at 6:47 AM, MauMau <maumau307(at)gmail(dot)com> wrote:
> As I guessed in the previous mail, both our patches cause
> pgwin32_is_service() to return 1 even when SECURITY_SERVICE_RID is
> disabled, if the service is running as a Local System. The existing
> logic of checking for Local System should be removed. The attached
> patch fixes this problem.

Meh. Local System accounts are used only by services (see comments of
pgwin32_is_service), so I'd expect pgwin32_is_service() to return true
in this case, contrary to what your v5 is doing. v4 is doing it better
I think at quick glance.

Here are the diffs between your v4 and v5 of this refactoring btw for
people wondering:
/*
- * We consider ourselves running as a service if one of the following is
- * true:
- *
- * 1) We are running as Local System (only used by services)
- * 2) Our token contains SECURITY_SERVICE_RID (automatically added to the
+ * We consider ourselves running as a service if
+ * our token contains SECURITY_SERVICE_RID (automatically added to the
* process token by the SCM when starting a service)
*
* Return values:
@@ -115,39 +112,13 @@ pgwin32_is_service(void)
static int _is_service = -1;
BOOL IsMember;
PSID ServiceSid;
- PSID LocalSystemSid;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};

/* Only check the first time */
if (_is_service != -1)
return _is_service;

- /* First check for Local System */
- if (!AllocateAndInitializeSid(&NtAuthority, 1,
- SECURITY_LOCAL_SYSTEM_RID, 0, 0, 0, 0, 0, 0, 0,
- &LocalSystemSid))
- {
- fprintf(stderr, "could not get SID for Local System account:
error code %lu\n",
- GetLastError());
- return -1;
- }
-
- if (!CheckTokenMembership(NULL, LocalSystemSid, &IsMember))
- {
- fprintf(stderr, "could not check access token membership:
error code %lu\n",
- GetLastError());
- FreeSid(LocalSystemSid);
- return -1;
- }
- FreeSid(LocalSystemSid);
-
- if (IsMember)
- {
- _is_service = 1;
- return _is_service;
- }
-
- /* Next check for service group */
+ /* Check for service group membership */

Not relying on the fact that local system accounts are only used by
services looks bad to me.
--
Michael