[BUGS] BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled

[b***@rtda.com]

The following bug has been logged on the website:

Bug reference: 13755
Logged by: Breen Hagan
Email address: ***@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!

[Michael Paquier]

Post by b***@rtda.com
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?

[Breen Hagan]

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

Post by b***@rtda.com

Post by b***@rtda.com
Short version: pgwin32_is_service checks the process token for
SECURITY_SERVICE_RID by doing an EqualSid check. This will match

against a

Post by b***@rtda.com
SECURITY_SERVICE_RID that has been disabled ("use_for_deny_only"),

causing

Post by b***@rtda.com
PG to think it's a service when it is not. This causes it to attempt to

log

Post by b***@rtda.com
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 Paquier]

Post by Breen Hagan
Michael,

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

Post by Breen Hagan
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-***@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.

Post by Breen Hagan
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 Paquier]

On Sat, Nov 7, 2015 at 4:09 PM, Michael Paquier

Post by Michael Paquier

Post by Breen Hagan
Michael,

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

Post by Breen Hagan
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?