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@gmail.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