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

On Tue, Nov 8, 2016 at 2:25 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> From: pgsql-hackers-owner@postgresql.org
>> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
>> Things are this way since b15f9b08 that introduced pgwin32_is_service().
>> Still, by considering what you say, you definitely have a point that if
>> postgres is started by another service running as Local System logs are
>> going where they should not. Let's remove the check for LocalSystem but
>> still check for SE_GROUP_ENABLED.
>> So, without any refactoring work, isn't the attached patch just but fine?
>> That seems to work properly for me.
>
> Just taking a look at the patch, I'm sure it will work.
>
> Committer (Heikki?),
> v5 is refactored for HEAD, and v6 is for previous releases without refactoring.  I'd like v5 to be applied to at
leastHEAD, as it removes a lot of unnecessary code.
 

+    if (!CheckTokenMembership(NULL, AdministratorsSid, &IsAdministrators) ||
+        !CheckTokenMembership(NULL, PowerUsersSid, &IsPowerUsers))    {
-        if ((EqualSid(AdministratorsSid, Groups->Groups[x].Sid) &&
-             (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)) ||
-            (EqualSid(PowerUsersSid, Groups->Groups[x].Sid) &&
-             (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)))
-        {
-            success = TRUE;
-            break;
-        }
+        log_error("could not check access token membership: error code %lu\n",
+                GetLastError());
+        exit(1);    }
I just looked more deeply at your refactoring patch, and I didn't know
about CheckTokenMembership()... The whole logic of your patch depends
on it. That's quite a cleanup that you have here. It looks that the
former implementation just had no knowledge of this routine or it
would just have been used.

+    if (IsAdministrators || IsPowerUsers)
+        return 1;
+    else
+        return 0;
I would remove the else here.
-- 
Michael