From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Michael Paquier <michael.paquier@gmail.com>
To: Tsunakawa, Takayuki <tsunakawa.takay@jp.fujitsu.com>
Cc: MauMau <maumau307@gmail.com>
Cc: Breen Hagan <breen@rtda.com>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
Date: Thu, 16 Mar 2017 10:40:11 +0200
Message-ID: <f20d0a60-e9cc-beb6-93d9-ecdda829b1e4@iki.fi> (raw)
In-Reply-To: <CAB7nPqQ-A1vgh3aRk08LSu-F9HE-tw9DRk850YnUMgqQf5XrXA@mail.gmail.com>
References: <CAB7nPqTAiB6+z=Cbqzt4KNNkynhP6D7_r_KaZBkmpt9mX7STuQ@mail.gmail.com>
<952A88FFCBDA419EB6D6556B6A8F29C1@tunaPC>
<CAB7nPqTRtATjX5JwYwE41X4TzzVLnykmXiTxNsn3wziMHp43Hg@mail.gmail.com>
<0A3221C70F24FB45833433255569204D1F63BD65@G01JPEXMBYT05>
<CAB7nPqQzLmq=LT-4+S-svHob85KNdtCQdq4NLczde98LXTFsnQ@mail.gmail.com>
<8230C8E316B544FC92813D96FA139F2E@tunaPC>
<CAB7nPqRTaifmRyZojGJF1AV9_72QwjBgdUe1HhS3Gx2qYAJcNA@mail.gmail.com>
<0A3221C70F24FB45833433255569204D1F63C649@G01JPEXMBYT05>
<CAB7nPqQnCB2zrSq0iTEtc=oYPcc_Oh_HxP=3Vy9ZnFd-m8ZXKw@mail.gmail.com>
<0A3221C70F24FB45833433255569204D1F63C6C9@G01JPEXMBYT05>
<CAB7nPqSxV744oC6ai5og566=5=9+pT10WkN5KdpjyrjAx+=wNA@mail.gmail.com>
<0A3221C70F24FB45833433255569204D1F63C7A6@G01JPEXMBYT05>
<CAB7nPqREw6B-qQAOrbLZTTQ-Ug3NKX2O59WeAiP5sSWSKuJuMQ@mail.gmail.com>
<0A3221C70F24FB45833433255569204D1F63C85A@G01JPEXMBYT05>
<CAB7nPqQ-A1vgh3aRk08LSu-F9HE-tw9DRk850YnUMgqQf5XrXA@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
On 11/08/2016 07:56 AM, Michael Paquier wrote:
> 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.
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: hlinnaka@iki.fi, michael.paquier@gmail.com, tsunakawa.takay@jp.fujitsu.com, maumau307@gmail.com, breen@rtda.com, alvherre@2ndquadrant.com, pgsql-hackers@postgresql.org
Subject: Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
In-Reply-To: <f20d0a60-e9cc-beb6-93d9-ecdda829b1e4@iki.fi>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox