Thread: 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@rtda.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@rtda.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.paquier@gmail.com>
wrote:

> On Wed, Nov 4, 2015 at 3:23 PM,  <breen@rtda.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@rtda.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.paquier@gmail.com> wrote:
> On Fri, Nov 6, 2015 at 1:00 AM, Breen Hagan <breen@rtda.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.paquier@gmail.com>
wrote:

> On Sat, Nov 7, 2015 at 4:09 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > On Fri, Nov 6, 2015 at 1:00 AM, Breen Hagan <breen@rtda.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@rtda.com> wrote:
>
>
> On Sat, Nov 7, 2015 at 1:36 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Sat, Nov 7, 2015 at 4:09 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>> > On Fri, Nov 6, 2015 at 1:00 AM, Breen Hagan <breen@rtda.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@2ndquadrant.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.paquier@gmail.com> wrote:
> On Tue, Apr 5, 2016 at 1:08 AM, Alvaro Herrera <alvherre@2ndquadrant.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@iki.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@rtda.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.paquier@gmail.com> wrote:
> On Sat, Sep 24, 2016 at 2:55 AM, Breen Hagan <breen@rtda.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