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

The following bug has been logged on the website:

Bug reference: 13755
Logged by: Breen Hagan
Email address: breen(at)rtda(dot)com
PostgreSQL version: 9.4.4
Operating system: Windows 8.1
Description:

Short version: pgwin32_is_service checks the process token for
SECURITY_SERVICE_RID by doing an EqualSid check. This will match against a
SECURITY_SERVICE_RID that has been disabled ("use_for_deny_only"), causing
PG to think it's a service when it is not. This causes it to attempt to log
to the event log, but this doesn't work, and so there is no logging at all.

Long version: We ship PG with our own product, which may or may not be
installed as a service. When running PG, we run postgres.exe directly via a
Tcl-based wrapper script so that we can monitor the output in real time.
This works as expected when our product is not being run as a service.

When our product is installed as a service, we use CreateRestrictedToken to
disable all admin rights as well as the SECURITY_SERVICE_RID, and use the
returned token with CreateProcessAsUser, for which we also specify
CREATE_NEW_CONSOLE. This process then calls our wrapper script. Inside
this wrapper, I can call GetStdHandle (via Twapi) and get valid handles for
all 3: in, out, and err. Yet when the script calls postgres.exe, nothing is
received on the output. As mentioned above, nothing is logged in the event
log, either.

If you look at
https://msdn.microsoft.com/en-us/library/windows/desktop/aa379554(v=vs.85).aspx,
this code is very similar to pgwin32_is_service (except that it looks for
Admins), but also checks the attributes on the SID to see if it is enabled,
or used for deny only. I believe this check needs to be added to
pgwin32_is_service.

Thanks!

On Wed, Nov 4, 2015 at 3:23 PM, <breen(at)rtda(dot)com> wrote:
> Short version: pgwin32_is_service checks the process token for
> SECURITY_SERVICE_RID by doing an EqualSid check. This will match against a
> SECURITY_SERVICE_RID that has been disabled ("use_for_deny_only"), causing
> PG to think it's a service when it is not. This causes it to attempt to log
> to the event log, but this doesn't work, and so there is no logging at all.

OK. So if I am following correctly... If Postgres process uses a
SECURITY_SERVICE_RID SID that has SE_GROUP_USE_FOR_DENY_ONLY enabled
it will try to access to the event logs but will be denied as all
accesses are denied with this attribute, right?

What do you think about the patch attached then?
--
Michael

Michael,

I'm pretty sure your patch will fix my issue, but perhaps it should be a
positive check for SE_GROUP_ENABLED? I say "perhaps" because the last time
I did any serious Windows coding was 2005.

Thanks for the quick response!

Breen

On Thu, Nov 5, 2015 at 9:39 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Wed, Nov 4, 2015 at 3:23 PM, <breen(at)rtda(dot)com> wrote:
> > Short version: pgwin32_is_service checks the process token for
> > SECURITY_SERVICE_RID by doing an EqualSid check. This will match
> against a
> > SECURITY_SERVICE_RID that has been disabled ("use_for_deny_only"),
> causing
> > PG to think it's a service when it is not. This causes it to attempt to
> log
> > to the event log, but this doesn't work, and so there is no logging at
> all.
>
> OK. So if I am following correctly... If Postgres process uses a
> SECURITY_SERVICE_RID SID that has SE_GROUP_USE_FOR_DENY_ONLY enabled
> it will try to access to the event logs but will be denied as all
> accesses are denied with this attribute, right?
>
> What do you think about the patch attached then?
> --
> Michael
>

On Fri, Nov 6, 2015 at 1:00 AM, Breen Hagan <breen(at)rtda(dot)com> wrote:
> Michael,

(You should avoid top-posting, this breaks the logic of a thread).

> I'm pretty sure your patch will fix my issue, but perhaps it should be a
> positive check for SE_GROUP_ENABLED?

If we want to be completely consistent with pgwin32_is_admin, that
would be actually the opposite: Postgres should not start with an SID
that has administrator's rights for security reasons.

Btw, I think that you would be interested in this patch as well:
http://www.postgresql.org/message-id/CAB7nPqR=FsgqOsQL6qUC04XWbZ93Q9BC-qEmHu2Cvh8uMRNrNQ@mail.gmail.com
This makes pgwin32_is_service available for frontend applications as
well, hence you would not need to duplicate any upstream code and just
reuse it for your scripts. That's material for 9.6~ though. I am
actually planning to fix an old bug in pg_ctl handling of a service
using that.

> I say "perhaps" because the last time
> I did any serious Windows coding was 2005.

That's short considering these day's life average expectancy.
--
Michael

On Sat, Nov 7, 2015 at 4:09 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Fri, Nov 6, 2015 at 1:00 AM, Breen Hagan <breen(at)rtda(dot)com> wrote:
>> Michael,
>
> (You should avoid top-posting, this breaks the logic of a thread).
>
>> I'm pretty sure your patch will fix my issue, but perhaps it should be a
>> positive check for SE_GROUP_ENABLED?
>
> If we want to be completely consistent with pgwin32_is_admin, that
> would be actually the opposite: Postgres should not start with an SID
> that has administrator's rights for security reasons.

SECURITY_SERVICE_RID and SECURITY_BUILTIN_DOMAIN_RID are completely
separated concepts... Please ignore that. Still, yeah, it seems that
you are right, we would want SE_GROUP_ENABLED to be enabled to check
if process can access the event logs. Thoughts from any Windows ninja
in the surroundings?
--
Michael

On Sat, Nov 7, 2015 at 1:36 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:

> On Sat, Nov 7, 2015 at 4:09 PM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
> > On Fri, Nov 6, 2015 at 1:00 AM, Breen Hagan <breen(at)rtda(dot)com> wrote:
> >> Michael,
> >
> > (You should avoid top-posting, this breaks the logic of a thread).
> >
> >> I'm pretty sure your patch will fix my issue, but perhaps it should be a
> >> positive check for SE_GROUP_ENABLED?
> >
> > If we want to be completely consistent with pgwin32_is_admin, that
> > would be actually the opposite: Postgres should not start with an SID
> > that has administrator's rights for security reasons.
>
> SECURITY_SERVICE_RID and SECURITY_BUILTIN_DOMAIN_RID are completely
> separated concepts... Please ignore that. Still, yeah, it seems that
> you are right, we would want SE_GROUP_ENABLED to be enabled to check
> if process can access the event logs. Thoughts from any Windows ninja
> in the surroundings?

--
> Michael
>

Sorry to bring back a very old thread, but I was wondering if this was ever
resolved? I saw
an item in the 9.4.6 release notes that seemed similar, but upon checking
the code, I see
that pgwin32_is_service() still checks just for the existence of these RIDs
without checking
to see if they are enabled.

Thanks,
Breen

On Wed, Mar 9, 2016 at 11:44 PM, Breen Hagan <breen(at)rtda(dot)com> wrote:
>
>
> On Sat, Nov 7, 2015 at 1:36 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
>>
>> On Sat, Nov 7, 2015 at 4:09 PM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> > On Fri, Nov 6, 2015 at 1:00 AM, Breen Hagan <breen(at)rtda(dot)com> wrote:
>> >> Michael,
>> >
>> > (You should avoid top-posting, this breaks the logic of a thread).
>> >
>> >> I'm pretty sure your patch will fix my issue, but perhaps it should be
>> >> a
>> >> positive check for SE_GROUP_ENABLED?
>> >
>> > If we want to be completely consistent with pgwin32_is_admin, that
>> > would be actually the opposite: Postgres should not start with an SID
>> > that has administrator's rights for security reasons.
>>
>> SECURITY_SERVICE_RID and SECURITY_BUILTIN_DOMAIN_RID are completely
>> separated concepts... Please ignore that. Still, yeah, it seems that
>> you are right, we would want SE_GROUP_ENABLED to be enabled to check
>> if process can access the event logs. Thoughts from any Windows ninja
>> in the surroundings?
>>
>> --
>> Michael
>
>
> Sorry to bring back a very old thread, but I was wondering if this was ever
> resolved? I saw
> an item in the 9.4.6 release notes that seemed similar, but upon checking
> the code, I see
> that pgwin32_is_service() still checks just for the existence of these RIDs
> without checking
> to see if they are enabled.

This is not resolved yet, this just fell from my radar and I recall
that I spent some time thinking about the consequences and whereabouts
of using either SE_GROUP_ENABLED or SE_GROUP_USE_FOR_DENY_ONLY,
without actually reaching a conclusion. I think that the patch would
be straight-forward. But it needs a bit of review from the author
(Hi!) and some extra input would be welcome. I guess I could try to
look at that again.. That won't be this week for sure though.
--
Michael

Michael Paquier wrote:

> This is not resolved yet, this just fell from my radar and I recall
> that I spent some time thinking about the consequences and whereabouts
> of using either SE_GROUP_ENABLED or SE_GROUP_USE_FOR_DENY_ONLY,
> without actually reaching a conclusion. I think that the patch would
> be straight-forward. But it needs a bit of review from the author
> (Hi!) and some extra input would be welcome. I guess I could try to
> look at that again.. That won't be this week for sure though.

Bump.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Apr 5, 2016 at 1:08 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> Michael Paquier wrote:
>> This is not resolved yet, this just fell from my radar and I recall
>> that I spent some time thinking about the consequences and whereabouts
>> of using either SE_GROUP_ENABLED or SE_GROUP_USE_FOR_DENY_ONLY,
>> without actually reaching a conclusion. I think that the patch would
>> be straight-forward. But it needs a bit of review from the author
>> (Hi!) and some extra input would be welcome. I guess I could try to
>> look at that again.. That won't be this week for sure though.
>
> Bump.

Don't worry. This has not fallen from my radar yet..
--
Michael

On Tue, Apr 5, 2016 at 12:58 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Apr 5, 2016 at 1:08 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>> Michael Paquier wrote:
>>> This is not resolved yet, this just fell from my radar and I recall
>>> that I spent some time thinking about the consequences and whereabouts
>>> of using either SE_GROUP_ENABLED or SE_GROUP_USE_FOR_DENY_ONLY,
>>> without actually reaching a conclusion. I think that the patch would
>>> be straight-forward. But it needs a bit of review from the author
>>> (Hi!) and some extra input would be welcome. I guess I could try to
>>> look at that again.. That won't be this week for sure though.
>>
>> Bump.
>
> Don't worry. This has not fallen from my radar yet..

So I have been looking at this issue again and finished with the patch
attached. I think that it makes the most sense to browse the whole
list of groups, and choose if Postgres is running as a service if
service SID matches with one of the group SIDs listed, on top of which
this group SID should be enabled via SE_GROUP_ENABLED. Checking for
SE_GROUP_USE_FOR_DENY_ONLY would not make much sense, because it would
mean that SE_GROUP_ENABLED is not set, and that's what we are
interested in. That was in short the point of Breen, and it looks to
be the saner way to go.

What do others think?
--
Michael

On 04/08/2016 09:48 AM, Michael Paquier wrote:
> So I have been looking at this issue again and finished with the patch
> attached. I think that it makes the most sense to browse the whole
> list of groups, and choose if Postgres is running as a service if
> service SID matches with one of the group SIDs listed, on top of which
> this group SID should be enabled via SE_GROUP_ENABLED. Checking for
> SE_GROUP_USE_FOR_DENY_ONLY would not make much sense, because it would
> mean that SE_GROUP_ENABLED is not set, and that's what we are
> interested in. That was in short the point of Breen, and it looks to
> be the saner way to go.

Yeah, seems like the right way. pgwin32_is_admin() also checks for
SE_GROUP_ENABLED.

I think this is ready to be committed, except that I don't have an easy
way to reproduce the original problem to test this. I suppose I could
write a test program to call CreateRestrictedToken() and
CreateProcessAsUser(), but would rather avoid the work. Breen, if I push
a fix for this, can you build from sources and verify that it fixes your
original problem? Or alternatively, can you provide a test program that
I can use to verify it?

- Heikki

Hi,

Sorry for the delay in response. We don't presently build postgres for
Windows (we do for linux and macos), but I'm willing to give it a shot if
there is a solid doc on setting up the build. That would probably be
easier than doing a test program.

Breen

On Wed, Sep 21, 2016 at 7:50 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:

> On 04/08/2016 09:48 AM, Michael Paquier wrote:
>
>> So I have been looking at this issue again and finished with the patch
>> attached. I think that it makes the most sense to browse the whole
>> list of groups, and choose if Postgres is running as a service if
>> service SID matches with one of the group SIDs listed, on top of which
>> this group SID should be enabled via SE_GROUP_ENABLED. Checking for
>> SE_GROUP_USE_FOR_DENY_ONLY would not make much sense, because it would
>> mean that SE_GROUP_ENABLED is not set, and that's what we are
>> interested in. That was in short the point of Breen, and it looks to
>> be the saner way to go.
>>
>
> Yeah, seems like the right way. pgwin32_is_admin() also checks for
> SE_GROUP_ENABLED.
>
> I think this is ready to be committed, except that I don't have an easy
> way to reproduce the original problem to test this. I suppose I could write
> a test program to call CreateRestrictedToken() and CreateProcessAsUser(),
> but would rather avoid the work. Breen, if I push a fix for this, can you
> build from sources and verify that it fixes your original problem? Or
> alternatively, can you provide a test program that I can use to verify it?
>
> - Heikki
>
>

On Sat, Sep 24, 2016 at 2:55 AM, Breen Hagan <breen(at)rtda(dot)com> wrote:
> Sorry for the delay in response. We don't presently build postgres for
> Windows (we do for linux and macos), but I'm willing to give it a shot if
> there is a solid doc on setting up the build. That would probably be easier
> than doing a test program.

There is a whole chapter in the docs in the matter:
https://www.postgresql.org/docs/9.0/static/install-windows.html
--
Michael

On Sat, Sep 24, 2016 at 8:52 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Sat, Sep 24, 2016 at 2:55 AM, Breen Hagan <breen(at)rtda(dot)com> wrote:
>> Sorry for the delay in response. We don't presently build postgres for
>> Windows (we do for linux and macos), but I'm willing to give it a shot if
>> there is a solid doc on setting up the build. That would probably be easier
>> than doing a test program.
>
> There is a whole chapter in the docs in the matter:
> https://www.postgresql.org/docs/9.0/static/install-windows.html

(Moved to next CF, with same status "Ready for committer").
--
Michael

Hello,

From: Michael Paquier
(Moved to next CF, with same status "Ready for committer").

I reviewed and tested this patch after simplifying it like the
attached one. The file could be reduced by about 110 lines. Please
review and/or test it. Though I kept the status "ready for
committer", feel free to change it back based on the result.

I tested as follows. First, I confirmed that pg_is_admin() still
works by running postgres.exe from the Administrator command line:

--------------------------------------------------
G:\>postgres
Execution of PostgreSQL by a user with administrative permissions is
not
permitted.
The server must be started under an unprivileged user ID to prevent
possible system security compromises. See the documentation for
more information on how to properly start the server.

G:\>
--------------------------------------------------

Then, I added the following two elog() calls in postmaster.c so that
pg_is_admin() and pg_is_service() works fine.

--------------------------------------------------
maybe_start_bgworker();

elog(LOG, "pgwin32_is_admin = %d", pgwin32_is_admin());
elog(LOG, "pgwin32_is_service = %d", pgwin32_is_service());

status = ServerLoop();
--------------------------------------------------

To reproduce the OP's problem, I modified pg_ctl.c to disable
SECURITY_SERVICE_RID when spawning postgres.exe. Without the patch,
starting the Windows service emit the following log, showing that
pg_is_service() misjudged that postgres is running as a Windows
service:

LOG: pgwin32_is_admin = 0
LOG: pgwin32_is_service = 1

With the patch, the log became correct:

LOG: pgwin32_is_admin = 0
LOG: pgwin32_is_service = 0

Regards
Takayuki Tsunakawa

Hello,

Sorry, I may have had to send this to pgsql-hackers. I just replied
to all, which did not include pgsql-hackers but pgsql-bugs because
this discussion was on pgsql-bugs. CommitFest app doesn't seem to
reflect the mails on pgsql-bugs, so I'm re-submitting this here on
pgsql-hackers.

From: Michael Paquier
(Moved to next CF, with same status "Ready for committer").

I reviewed and tested this patch after simplifying it like the
attached one. The file could be reduced by about 110 lines. Please
review and/or test it. Though I kept the status "ready for
committer", feel free to change it back based on the result.

I tested as follows. First, I confirmed that pg_is_admin() still
works by running postgres.exe from the Administrator command line:

--------------------------------------------------
G:\>postgres
Execution of PostgreSQL by a user with administrative permissions is
not
permitted.
The server must be started under an unprivileged user ID to prevent
possible system security compromises. See the documentation for
more information on how to properly start the server.

G:\>
--------------------------------------------------

Then, I added the following two elog() calls in postmaster.c so that
pg_is_admin() and pg_is_service() works fine.

--------------------------------------------------
maybe_start_bgworker();

elog(LOG, "pgwin32_is_admin = %d", pgwin32_is_admin());
elog(LOG, "pgwin32_is_service = %d", pgwin32_is_service());

status = ServerLoop();
--------------------------------------------------

To reproduce the OP's problem, I modified pg_ctl.c to disable
SECURITY_SERVICE_RID when spawning postgres.exe. Without the patch,
starting the Windows service emit the following log, showing that
pg_is_service() misjudged that postgres is running as a Windows
service:

LOG: pgwin32_is_admin = 0
LOG: pgwin32_is_service = 1

With the patch, the log became correct:

LOG: pgwin32_is_admin = 0
LOG: pgwin32_is_service = 0

Regards
Takayuki Tsunakawa

On Sun, Nov 6, 2016 at 6:30 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> Sorry, I may have had to send this to pgsql-hackers. I just replied
> to all, which did not include pgsql-hackers but pgsql-bugs because
> this discussion was on pgsql-bugs. CommitFest app doesn't seem to
> reflect the mails on pgsql-bugs, so I'm re-submitting this here on
> pgsql-hackers.

No problem, I still see a unique thread so that's not an issue seen from here.

> I reviewed and tested this patch after simplifying it like the
> attached one. The file could be reduced by about 110 lines. Please
> review and/or test it. Though I kept the status "ready for
> committer", feel free to change it back based on the result.

So you see the same behavior with the patch I sent and your
refactoring, right? If yes, backpatching the one-liner is the safest
bet to me. We could keep the refactoring for HEAD if it makes sense.

Something is wrong with the format of your patch by the way. My
Windows and even OSX environments recognize it as a binary file,
though I can read it in any editor and I cannot apply it cleanly with
a simple patch command. Could you send it again and double-check?

> To reproduce the OP's problem, I modified pg_ctl.c to disable
> SECURITY_SERVICE_RID when spawning postgres.exe.

So basically you allocated a SID to drop via AllocateAndInitializeSid,
called _CreateRestrictedToken and let the process being spawned? I
think that this is the patch attached
(win32-disable-service-rid.patch). Could you confirm? I want to be
sure that we are testing the same things.
--
Michael

From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Michael Paquier
> On Sun, Nov 6, 2016 at 6:30 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> > Sorry, I may have had to send this to pgsql-hackers. I just replied
> > to all, which did not include pgsql-hackers but pgsql-bugs because
> > this discussion was on pgsql-bugs. CommitFest app doesn't seem to
> > reflect the mails on pgsql-bugs, so I'm re-submitting this here on
> > pgsql-hackers.
>
> No problem, I still see a unique thread so that's not an issue seen from
> here.

You are right. A while after I sent the second mail, I noticed the CommitFest app collected both of my mails. I was just impatient.

> So you see the same behavior with the patch I sent and your refactoring,
> right? If yes, backpatching the one-liner is the safest bet to me. We could
> keep the refactoring for HEAD if it makes sense.

Yes. And It's fine to me that your patch will be applied to previous releases and my patch to HEAD only. This is a good (rare?) chance to reduce the Windows-specific code, so I want to take advantage of it.

> Something is wrong with the format of your patch by the way. My Windows
> and even OSX environments recognize it as a binary file, though I can read
> it in any editor and I cannot apply it cleanly with a simple patch command.
> Could you send it again and double-check?

Ouch, the Git shell included in GitHub Desktop for Windows produced the diff in UTF-16 and CR/LF line terminators. I haven't found how to fix it, so I generated the attached patch on Linux. Please check it.

> > To reproduce the OP's problem, I modified pg_ctl.c to disable
> > SECURITY_SERVICE_RID when spawning postgres.exe.
>
> So basically you allocated a SID to drop via AllocateAndInitializeSid,
> called _CreateRestrictedToken and let the process being spawned? I think
> that this is the patch attached (win32-disable-service-rid.patch). Could
> you confirm? I want to be sure that we are testing the same things.

Yes, I did the same.

Regards
Takayuki Tsunakawa

On Mon, Nov 7, 2016 at 9:49 AM, 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
>> On Sun, Nov 6, 2016 at 6:30 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
>> So you see the same behavior with the patch I sent and your refactoring,
>> right? If yes, backpatching the one-liner is the safest bet to me. We could
>> keep the refactoring for HEAD if it makes sense.
>
> Yes. And It's fine to me that your patch will be applied to previous releases and my patch to HEAD only. This is a good (rare?) chance to reduce the Windows-specific code, so I want to take advantage of it.

Yes, I can follow that argument.

>> Something is wrong with the format of your patch by the way. My Windows
>> and even OSX environments recognize it as a binary file, though I can read
>> it in any editor and I cannot apply it cleanly with a simple patch command.
>> Could you send it again and double-check?
>
> Ouch, the Git shell included in GitHub Desktop for Windows produced the diff in UTF-16 and CR/LF line terminators. I haven't found how to fix it, so I generated the attached patch on Linux. Please check it.

And the patch got twice smaller in size. Thanks.

>> > To reproduce the OP's problem, I modified pg_ctl.c to disable
>> > SECURITY_SERVICE_RID when spawning postgres.exe.
>>
>> So basically you allocated a SID to drop via AllocateAndInitializeSid,
>> called _CreateRestrictedToken and let the process being spawned? I think
>> that this is the patch attached (win32-disable-service-rid.patch). Could
>> you confirm? I want to be sure that we are testing the same things.
>
> Yes, I did the same.

Hm.. I have just tested HEAD, my patch and your patch using my patch
test on pg_ctl.c, but I am always getting pgwin32_is_service set to 0
when running pg_ctl start from a terminal, and set it to 1 when
running pg_ctl service to register the service startup. Could you
precise in which ways you started the Postgres instance and could you
post the patch of pg_ctl you used? I am afraid that I am taking it
incorrectly because I am not able to see any differences.

Also, did you test the patch I posted and were you able to see the
same differences as with your patch? I still think that my short patch
is logically correct but if the tests are not we are in a no-go
position for any fix posted on this thread.
--
Michael

From: Michael Paquier
Hm.. I have just tested HEAD, my patch and your patch using my patch
test on pg_ctl.c, but I am always getting pgwin32_is_service set to 0
when running pg_ctl start from a terminal, and set it to 1 when
running pg_ctl service to register the service startup. Could you
precise in which ways you started the Postgres instance and could you
post the patch of pg_ctl you used? I am afraid that I am taking it
incorrectly because I am not able to see any differences.

Also, did you test the patch I posted and were you able to see the
same differences as with your patch? I still think that my short patch
is logically correct but if the tests are not we are in a no-go
position for any fix posted on this thread.

Yes, I tested both your patch and mine. I used the attached pg_ctl.c.
It adds -z option which disables SECURITY_SERVICE_RID.

I registered the service with "pg_ctl register -N pg -D
datadir -w -z -S demand -U myuser -P mypass", then started the service
with "net start pg". The following messages were output in the server
log:

LOG: pgwin32_is_admin = 0
LOG: pgwin32_is_service = 0
LOG: database system was shut down at 2016-11-07 22:04:46 JST
LOG: MultiXact member wraparound protections are now enabled
LOG: database system is ready to accept connections
LOG: autovacuum launcher started

Without -z, the message becomes "pgwin32_is_service = 1". And without
the win32security.c patch, "pgwin32_is_service = 1" is output.

I guess you registered the service without specifying the service
account with -U. Then the service runs as the Local System account,
whence pgwin32_is_service() returns 1.

Regards
Takayuki Tsunakawa

Hi, Michael

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.

Regards
Takayuki Tsunakawa

On Mon, Nov 7, 2016 at 10:31 PM, MauMau <maumau307(at)gmail(dot)com> wrote:
> Yes, I tested both your patch and mine. I used the attached pg_ctl.c.
> It adds -z option which disables SECURITY_SERVICE_RID.

Okay, so you did exactly what I did except that you wrapped with an option...

> I guess you registered the service without specifying the service
> account with -U. Then the service runs as the Local System account,
> whence pgwin32_is_service() returns 1.

Thank you, that's as you said. I was just using the local user account
which is why I did not see the difference. And now I can. I finished
by not using your version of pg_ctl but mine still the result is the
same. Hm, now that we are two folks able to test the difference, I'd
suggest that a committer pops up and pushes the one-liner patch I sent
upthread and back-patches it.

For the refactoring, I guess that we could sort that later on, and I
promise to look at soon. The issue reported on this thread has been
here for 1 year, I am glad that someone finally came up an easy way to
test things.
--
Michael

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

From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Michael Paquier
> 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.
> Not relying on the fact that local system accounts are only used by services
> looks bad to me.

I believe v5 is correct for two reasons:

(1)
SECURITY_SERVICE_RID is enough to check, because the process gets SECURITY_SERVICE_RID when it runs as a service.

https://msdn.microsoft.com/ja-jp/library/windows/desktop/aa379649(v=vs.85).aspx

SECURITY_SERVICE_RID
Accounts authorized to log on as a service. This is a group identifier added to the token of a process when it was logged as a service. The corresponding logon type is LOGON32_LOGON_SERVICE.

I saw descriptions that LocalSystem is used by the SCM, but didn't find a statement that LocalSystem is used only by SCM and services. In addition, if the check for LocalSystem is really necessary, LocalService and NetworkService also need to be checked.

https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs.85).aspx

(Japanese article)
http://www.atmarkit.co.jp/ait/articles/0905/08/news095.html

(2)
The OP wants to explicitly run postgres.exe outside the service even when his app runs as a service, so that the app can read postgres's messages from its stdout/stderr. So, he disabled SECURITY_SERVICE_RID when starting postgres.exe. His users may run his app as a service under LocalSystem.

[Excerpt]
--------------------------------------------------
We ship PG with our own product, which may or may not be
installed as a service. When running PG, we run postgres.exe directly via a
Tcl-based wrapper script so that we can monitor the output in real time.

When our product is installed as a service, we use CreateRestrictedToken to
disable all admin rights as well as the SECURITY_SERVICE_RID, and use the
returned token with CreateProcessAsUser, for which we also specify
CREATE_NEW_CONSOLE. This process then calls our wrapper script. Inside
this wrapper, I can call GetStdHandle (via Twapi) and get valid handles for
all 3: in, out, and err. Yet when the script calls postgres.exe, nothing is
received on the output. As mentioned above, nothing is logged in the event
log, either.
--------------------------------------------------

Regards
Takayuki Tsunakawa

On Tue, Nov 8, 2016 at 11:36 AM, Tsunakawa, Takayuki
<tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:
> SECURITY_SERVICE_RID
> Accounts authorized to log on as a service. This is a group identifier added to the token of a process when it was logged as a service. The corresponding logon type is LOGON32_LOGON_SERVICE.
>
> I saw descriptions that LocalSystem is used by the SCM, but didn't find a statement that LocalSystem is used only by SCM and services. In addition, if the check for LocalSystem is really necessary, LocalService and NetworkService also need to be checked.
>
> https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs.85).aspx

That's what I looked at as well :) And this part is what caught my
attention, meaning that it is not used by anything else than the SCM:
"The LocalSystem account is a predefined local account used by the
service control manager."
And this implies, at least it seems to me, that trying to run Postgres
as this user is actually not something you'd want to do.

> (2)
> The OP wants to explicitly run postgres.exe outside the service even when his app runs as a service, so that the app can read postgres's messages from its stdout/stderr. So, he disabled SECURITY_SERVICE_RID when starting postgres.exe. His users may run his app as a service under LocalSystem.

Good question, and I don't know how this is used.
--
Michael

From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Michael Paquier
> https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs
> > .85).aspx
>
> That's what I looked at as well :) And this part is what caught my attention,
> meaning that it is not used by anything else than the SCM:
> "The LocalSystem account is a predefined local account used by the service
> control manager."

The same thing is said about other two special accounts, so they need to be checked if we really believe we need to check for LocalSystem.

"The LocalService account is a predefined local account used by the service control manager."
"The NetworkService account is a predefined local account used by the service control manager."

But, in practice, SECURITY_SERVICE_RID has turned out to be enough.

> And this implies, at least it seems to me, that trying to run Postgres as
> this user is actually not something you'd want to do.

Yes, I think people should avoid using LocalSystem for user services like PostgreSQL for security reasons. But the Services applet in the Control Panel allows to select LocalSystem, and pg_ctl register creates a service with LocalSystem account when -U is omitted.

Regards
Takayuki Tsunakawa

On Tue, Nov 8, 2016 at 12:16 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
>> https://msdn.microsoft.com/ja-jp/library/windows/desktop/ms684190(v=vs
>> > .85).aspx
>>
>> That's what I looked at as well :) And this part is what caught my attention,
>> meaning that it is not used by anything else than the SCM:
>> "The LocalSystem account is a predefined local account used by the service
>> control manager."
>
> The same thing is said about other two special accounts, so they need to be checked if we really believe we need to check for LocalSystem.
>
> "The LocalService account is a predefined local account used by the service control manager."
> "The NetworkService account is a predefined local account used by the service control manager."
>
> But, in practice, SECURITY_SERVICE_RID has turned out to be enough.

Hm... See here:
http://stackoverflow.com/questions/6084547/how-to-check-whether-a-process-is-running-as-a-windows-service
And particularly this quote:
"No, that is not reliable because if a service is started from command
line for example it will not have this token. "
--
Michael

From: pgsql-hackers-owner(at)postgresql(dot)org
> [mailto:pgsql-hackers-owner(at)postgresql(dot)org] On Behalf Of Michael Paquier
> Hm... See here:
> http://stackoverflow.com/questions/6084547/how-to-check-whether-a-proc
> ess-is-running-as-a-windows-service
> And particularly this quote:
> "No, that is not reliable because if a service is started from command line
> for example it will not have this token. "

Is there any Microsoft document that states this? I don't think the above comment is correct, because SECURITY_SERVICE_RID was present when I started the service from command line with "net start".

Regards
Takayuki Tsunakawa

On Tue, Nov 8, 2016 at 1:36 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
>> Hm... See here:
>> http://stackoverflow.com/questions/6084547/how-to-check-whether-a-proc
>> ess-is-running-as-a-windows-service
>> And particularly this quote:
>> "No, that is not reliable because if a service is started from command line
>> for example it will not have this token. "
>
> Is there any Microsoft document that states this? I don't think the above comment is correct, because SECURITY_SERVICE_RID was present when I started the service from command line with "net start".

Not that I can see of... So maybe I'm just confused by this comment as
it is added by the SCM itself, right?

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.
--
Michael

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

Regards
Takayuki Tsunakawa

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

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

From: Michael Paquier [mailto:michael(dot)paquier(at)gmail(dot)com]
> 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.

Yes, Microsoft recommends GetTokenMembership() because it's simpler.

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

IIRC, I sometimes saw this style of code in PostgreSQL (or in psqlODBC possibly...) I'd like to leave the style choice to the committer.

Regards
Takayuki Tsunakawa

On 8 November 2016 at 14:31, Tsunakawa, Takayuki
<tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:
> From: Michael Paquier [mailto:michael(dot)paquier(at)gmail(dot)com]
>> 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.
>
> Yes, Microsoft recommends GetTokenMembership() because it's simpler.

You meant CheckTokenMembership().

Relevant references:

* https://msdn.microsoft.com/en-us/library/windows/desktop/aa376389(v=vs.85).aspx

* https://blogs.msdn.microsoft.com/junfeng/2007/01/26/how-to-tell-if-the-current-user-is-in-administrators-group-programmatically/

The docs say it's supported in WinXP and Win2k3, so it's fine to use.

The blog above notes that it "won't work" on Vista+, but if you read
it you'll notice that what it means is that you can't tell if the
running user has the right to elevate to Administrator rights. I don't
think PostgreSQL cares about that, it only cares if it has admin
rights *right now*, not whether the running user can gain such rights
using a UAC elevation prompt. In fact it'd be super-annoying if you
couldn't run postgres as a user with admin elevation rights so this
behaviour seems to be what we want.

The proposed patch does need to be checked with:

* WinXP, non-admin
* WinXP, admin, should refuse to run
* WinVista / Win7, local admin, UAC on => should run
* WinVista / Win7, local admin, UAC off => should refuse to run
* WinVista / Win7, run cmd.exe using "run as admin" => should refuse to run
* WinVista / Win7, not local admin => should run

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

From: Craig Ringer [mailto:craig(at)2ndquadrant(dot)com]
> You meant CheckTokenMembership().

Yes, my typo in the mail.

> The proposed patch does need to be checked with:

I understood you meant by "refuse to run" that postgres.exe fails to start below. Yes, I checked it on Win10. I don't have access to WinXP/2003 - Microsoft ended their support.

if (pgwin32_is_admin())
{
write_stderr("Execution of PostgreSQL by a user with administrative permissions is not\n"
"permitted.\n"
"The server must be started under an unprivileged user ID to prevent\n"
"possible system security compromises. See the documentation for\n"
"more information on how to properly start the server.\n");
exit(1);
}

Regards
Takayuki Tsunakawa

On Tue, Nov 22, 2016 at 1:58 PM, Tsunakawa, Takayuki
<tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com> wrote:
> From: Craig Ringer [mailto:craig(at)2ndquadrant(dot)com]
>> You meant CheckTokenMembership().
>
> Yes, my typo in the mail.
>
>> The proposed patch does need to be checked with:
>
> I understood you meant by "refuse to run" that postgres.exe fails to start below. Yes, I checked it on Win10. I don't have access to WinXP/2003 - Microsoft ended their support.
>
> if (pgwin32_is_admin())
> {
> write_stderr("Execution of PostgreSQL by a user with administrative permissions is not\n"
> "permitted.\n"
> "The server must be started under an unprivileged user ID to prevent\n"
> "possible system security compromises. See the documentation for\n"
> "more information on how to properly start the server.\n");
> exit(1);
> }

I have moved that to next CF. The refactoring patch needs more testing
but the basic fix patch could be applied.
--
Michael

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

From: Heikki Linnakangas
So, I think we still need the check for Local System.

Thanks, fixed and confirmed that the error message is output in the
event log.

Regards
MauMau

On 03/17/2017 12:21 AM, MauMau wrote:
> From: Heikki Linnakangas
> So, I think we still need the check for Local System.
>
> Thanks, fixed and confirmed that the error message is output in the
> event log.

Committed, thanks!

- Heikki