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

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@gmail.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@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
> On Sun, Nov 6, 2016 at 6:30 PM, MauMau <maumau307@gmail.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
justimpatient.
 



> 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'tfound 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.takay@jp.fujitsu.com> wrote:
> From: pgsql-hackers-owner@postgresql.org
>> [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Michael Paquier
>> On Sun, Nov 6, 2016 at 6:30 PM, MauMau <maumau307@gmail.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'tfound 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@gmail.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@gmail.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@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.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
loggedas 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
SCMand services.  In addition, if the check for LocalSystem is really necessary, LocalService and NetworkService also
needto 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
canread postgres's messages from its stdout/stderr.  So, he disabled SECURITY_SERVICE_RID when starting postgres.exe.
Hisusers 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.takay@jp.fujitsu.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
loggedas 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
SCMand services.  In addition, if the check for LocalSystem is really necessary, LocalService and NetworkService also
needto 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
canread postgres's messages from its stdout/stderr.  So, he disabled SECURITY_SERVICE_RID when starting postgres.exe.
Hisusers 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@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.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
checkfor 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
Servicesapplet in the Control Panel allows to select LocalSystem, and pg_ctl register creates a service with
LocalSystemaccount when -U is omitted.
 

Regards
Takayuki Tsunakawa

On Tue, Nov 8, 2016 at 12:16 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
>> 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
checkfor 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@postgresql.org
> [mailto:pgsql-hackers-owner@postgresql.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_RIDwas 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.takay@jp.fujitsu.com> wrote:
> From: pgsql-hackers-owner@postgresql.org
>> [mailto:pgsql-hackers-owner@postgresql.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_RIDwas 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@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.
> 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.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.
>> 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
leastHEAD, 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.paquier@gmail.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
tothe committer.
 

Regards
Takayuki Tsunakawa

On 8 November 2016 at 14:31, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> From: Michael Paquier [mailto:michael.paquier@gmail.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@2ndquadrant.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
haveaccess 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"
"possiblesystem security compromises.  See the documentation for\n"              "more information on how to properly
startthe server.\n");    exit(1);}
 

Regards
Takayuki Tsunakawa

On Tue, Nov 22, 2016 at 1:58 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:
> From: Craig Ringer [mailto:craig@2ndquadrant.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'thave 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.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
leastHEAD, 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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