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

From: Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>
Cc: MauMau <maumau307(at)gmail(dot)com>, Breen Hagan <breen(at)rtda(dot)com>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
Date: 2017-03-16 08:40:11
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-bugs pgsql-hackers

On 11/08/2016 07:56 AM, Michael Paquier wrote:
> On Tue, Nov 8, 2016 at 2:25 PM, Tsunakawa, Takayuki
> <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:
>> From: pgsql-hackers-owner(at)postgresql(dot)org
>>> [mailto:pgsql-hackers-owner(at)postgresql(dot)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.

I did some testing on patch v5, on my Windows 8 VM. I launched cmd as
Administrator, and registered the service with:

pg_ctl register -D data

I.e. without specifying a user. When I start the service with "net
start", it refuses to start, and there are no messages in the event log.
It refuses to start because "data" is not a valid directory, so that's
correct. But the error message about that is lost.

Added some debugging messages to win32_is_service(), and it confirms
that with this patch (v5), win32_is_service() incorrectly returns false,
while unmodified git master returns true, and writes the error message
to the event log.

So, I think we still need the check for Local System.

>>> 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 least HEAD, 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.

Yeah, CheckTokenMembership() seems really handy. Let's switch to that,
but without removing the checks for Local System.

- Heikki

In response to


Browse pgsql-bugs by date

  From Date Subject
Next Message Sandeep Thakkar 2017-03-16 12:57:38 Re: Error floating-point exception on postgresql installer
Previous Message Andrew Gierth 2017-03-16 06:55:54 Re: BUG #14057: vacuum setting reltuples=0 for tables with >0 tuples

Browse pgsql-hackers by date

  From Date Subject
Next Message Petr Jelinek 2017-03-16 08:40:48 Re: logical replication launcher crash on buildfarm
Previous Message Michael Paquier 2017-03-16 07:48:40 Re: exposing wait events for non-backends (was: Tracking wait event for latches)