[HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On 11/08/2016 07:56 AM, Michael Paquier wrote:
On Tue, Nov 8, 2016 at 2:25 PM, Tsunakawa, Takayuki
wrote:
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
Things are this way since b15f9b08 that introduced pgwin32_is_service().
Still, by considering what you say, you definitely have a point that if
postgres is started by another service running as Local System logs are
going where they should not. Let's remove the check for LocalSystem but
still check for SE_GROUP_ENABLED.
I did some testing on patch v5, on my Windows 8 VM. I launched cmd as
Administrator, and registered the service with:
pg_ctl register -D data
I.e. without specifying a user. When I start the service with "net
start", it refuses to start, and there are no messages in the event log.
It refuses to start because "data" is not a valid directory, so that's
correct. But the error message about that is lost.
Added some debugging messages to win32_is_service(), and it confirms
that with this patch (v5), win32_is_service() incorrectly returns false,
while unmodified git master returns true, and writes the error message
to the event log.
So, I think we still need the check for Local System.
So, without any refactoring work, isn't the attached patch just but fine?
That seems to work properly for me.
Just taking a look at the patch, I'm sure it will work.
Committer (Heikki?),
v5 is refactored for HEAD, and v6 is for previous releases without refactoring.
I'd like v5 to be applied to at least HEAD, as it removes a lot of unnecessary
code.
+if (!CheckTokenMembership(NULL, AdministratorsSid, &IsAdministrators) ||
+!CheckTokenMembership(NULL, PowerUsersSid, &IsPowerUsers))
{
-if ((EqualSid(AdministratorsSid, Groups->Groups[x].Sid) &&
- (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)) ||
-(EqualSid(PowerUsersSid, Groups->Groups[x].Sid) &&
- (Groups->Groups[x].Attributes & SE_GROUP_ENABLED)))
-{
-success = TRUE;
-break;
-}
+log_error("could not check access token membership: error code %lu\n",
+GetLastError());
+exit(1);
}
I just looked more deeply at your refactoring patch, and I didn't know
about CheckTokenMembership()... The whole logic of your patch depends
on it. That's quite a cleanup that you have here. It looks that the
former implementation just had no knowledge of this routine or it
would just have been used.
Yeah, CheckTokenMembership() seems really handy. Let's switch to that,
but without removing the checks for Local System.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Tue, Nov 22, 2016 at 1:58 PM, Tsunakawa, Takayuki
wrote:
> From: Craig Ringer [mailto:cr...@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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
From: Craig Ringer [mailto:cr...@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 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On 8 November 2016 at 14:31, Tsunakawa, Takayuki wrote: > From: Michael Paquier [mailto:michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
From: Michael Paquier [mailto:michael.paqu...@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 to the committer. Regards Takayuki Tsunakawa -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Tue, Nov 8, 2016 at 2:25 PM, Tsunakawa, Takayuki
wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@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.
+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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Tue, Nov 8, 2016 at 1:36 PM, Tsunakawa, Takayuki
wrote:
> From: pgsql-hackers-ow...@postgresql.org
>> [mailto:pgsql-hackers-ow...@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_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
diff --git a/src/port/win32security.c b/src/port/win32security.c
index 2c9ca15..e329eb0 100644
--- a/src/port/win32security.c
+++ b/src/port/win32security.c
@@ -122,12 +122,9 @@ pgwin32_is_admin(void)
}
/*
- * 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
- * process token by the SCM when starting a service)
+ * We consider ourselves running as a service if our token contains
+ * SECURITY_SERVICE_RID, which is automatically added to the process token
+ * by the SCM when starting a service.
*
* Return values:
* 0 = Not service
@@ -147,9 +144,7 @@ pgwin32_is_service(void)
char *InfoBuffer = NULL;
char errbuf[256];
PTOKEN_GROUPS Groups;
- PTOKEN_USER User;
PSID ServiceSid;
- PSID LocalSystemSid;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
UINT x;
@@ -164,37 +159,6 @@ pgwin32_is_service(void)
return -1;
}
- /* First check for local system */
- if (!pgwin32_get_dynamic_tokeninfo(AccessToken, TokenUser, &InfoBuffer,
- errbuf, sizeof(errbuf)))
- {
- fprintf(stderr, "%s", errbuf);
- return -1;
- }
-
- User = (PTOKEN_USER) InfoBuffer;
-
- 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\n");
- CloseHandle(AccessToken);
- return -1;
- }
-
- if (EqualSid(LocalSystemSid, User->User.Sid))
- {
- FreeSid(LocalSystemSid);
- free(InfoBuffer);
- CloseHandle(AccessToken);
- _is_service = 1;
- return _is_service;
- }
-
- FreeSid(LocalSystemSid);
- free(InfoBuffer);
-
/* Now check for group SID */
if (!pgwin32_get_dynamic_tokeninfo(AccessToken, TokenGroups, &InfoBuffer,
errbuf, sizeof(errbuf)))
@@ -218,7 +182,8 @@ pgwin32_is_service(void)
_is_service = 0;
for (x = 0; x < Groups->GroupCount; x++)
{
- if (EqualSid(ServiceSid, Groups->Groups[x].Sid))
+ if (EqualSid(ServiceSid, Groups->Groups[x].Sid) &&
+ (Groups->Groups[x].Attributes & SE_GROUP_ENABLED))
{
_is_service = 1;
break;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@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_RID was present when I started the service from command line with "net start". Regards Takayuki Tsunakawa -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Tue, Nov 8, 2016 at 12:16 PM, Tsunakawa, Takayuki wrote: > From: pgsql-hackers-ow...@postgresql.org >> [mailto:pgsql-hackers-ow...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Tue, Nov 8, 2016 at 11:36 AM, Tsunakawa, Takayuki 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@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 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Tue, Nov 8, 2016 at 6:47 AM, MauMau 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;
BOOLIsMember;
PSIDServiceSid;
- PSIDLocalSystemSid;
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Mon, Nov 7, 2016 at 10:31 PM, MauMau 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
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 win32-security-service-v5.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
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
/*
-
*
* pg_ctl --- start/stops/restarts the PostgreSQL server
*
* Portions Copyright (c) 1996-2016, PostgreSQL Global Development
Group
*
* src/bin/pg_ctl/pg_ctl.c
*
*
-
*/
#ifdef WIN32
/*
* Need this to get defines for restricted tokens and jobs. And it
* has to be set before any header from the Win32 API is loaded.
*/
#define _WIN32_WINNT 0x0501
#endif
#include "postgres_fe.h"
#include "libpq-fe.h"
#include "pqexpbuffer.h"
#include
#include
#include
#include
#include
#include
#include
#include
#ifdef HAVE_SYS_RESOURCE_H
#include
#include
#endif
#include "getopt_long.h"
#include "miscadmin.h"
/* PID can be negative for standalone backend */
typedef long pgpid_t;
typedef enum
{
SMART_MODE,
FAST_MODE,
IMMEDIATE_MODE
} ShutdownMode;
typedef enum
{
NO_COMMAND = 0,
INIT_COMMAND,
START_COMMAND,
STOP_COMMAND,
RESTART_COMMAND,
RELOAD_COMMAND,
STATUS_COMMAND,
PROMOTE_COMMAND,
KILL_COMMAND,
REGISTER_COMMAND,
UNREGISTER_COMMAND,
RUN_AS_SERVICE_COMMAND
} CtlCommand;
#define DEFAULT_WAIT60
static bool do_wait = false;
static bool del_service_rid = false;
static bool wait_set = false;
static int wait_seconds = DEFAULT_WAIT;
static bool wait_seconds_arg = false;
static bool silent_mode = false;
static ShutdownMode shutdown_mode = FAST_MODE;
static int sig = SIGINT; /* default */
static CtlCommand ctl_command = NO_COMMAND;
static char *pg_data = NULL;
static char *pg_config = NULL;
static char *pgdata_opt = NULL;
static char *post_opts = NULL;
static const char *progname;
static char *log_file = NULL;
static char *exec_path = NULL;
static char *event_source = NULL;
static char *register_servicename = "PostgreSQL"; /* FIXME: +
version ID? */
static char *register_username = NULL;
static char *register_password = NULL;
static char *argv0 = NULL;
static bool allow_core_files = false;
static time_t start_time;
static char postopts_file[MAXPGPATH];
static char version_file[MAXPGPATH];
static char pid_file[MAXPGPATH];
static char backup_file[MAXPGPATH];
static char recovery_file[MAXPGPATH];
static char promote_file[MAXPGPATH];
#ifdef WIN32
static DWORD pgctl_start_type = SERVICE_AUTO_START;
static SERVICE_STATUS status;
static SERVICE_STATUS_HANDLE hStatus = (SERVICE_STATUS_HANDLE) 0;
static HANDLE shutdownHandles[2];
static pid_t postmasterPID = -1;
#define shutdownEvent shutdownHandles[0]
#define postmasterProcess shutdownHandles[1]
#endif
static void write_stderr(const char *fmt,...) pg_attribute_printf(1,
2);
static void do_advice(void);
static void do_help(void);
static void set_mode(char *modeopt);
static void set_sig(char *signame);
static void do_init(void);
static void do_start(void);
static void do_stop(void);
static void do_restart(void);
static void do_reload(void);
static void do_status(void);
static void do_promote(void);
static void do_kill(pgpid_t pid);
static void print_msg(const char *msg);
static void adjust_data_dir(void);
#ifdef WIN32
#if (_MSC_VER >= 1800)
#include
#else
static bool IsWindowsXPOrGreater(void);
static bool
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Mon, Nov 7, 2016 at 9:49 AM, Tsunakawa, Takayuki wrote: > From: pgsql-hackers-ow...@postgresql.org >> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier >> On Sun, Nov 6, 2016 at 6:30 PM, MauMau 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
From: pgsql-hackers-ow...@postgresql.org > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier > On Sun, Nov 6, 2016 at 6:30 PM, MauMau 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 win32-security-service-v4.patch Description: win32-security-service-v4.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: BUG #13755: pgwin32_is_service not checking if SECURITY_SERVICE_SID is disabled
On Sun, Nov 6, 2016 at 6:30 PM, MauMau 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
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
index 4b47602..56c7f2e 100644
--- a/src/bin/pg_ctl/pg_ctl.c
+++ b/src/bin/pg_ctl/pg_ctl.c
@@ -1738,7 +1738,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
HANDLE origToken;
HANDLE restrictedToken;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
- SID_AND_ATTRIBUTES dropSids[2];
+ SID_AND_ATTRIBUTES dropSids[3];
/* Functions loaded dynamically */
__CreateRestrictedToken _CreateRestrictedToken = NULL;
@@ -1790,7 +1790,10 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
0, &dropSids[0].Sid) ||
!AllocateAndInitializeSid(&NtAuthority, 2,
SECURITY_BUILTIN_DOMAIN_RID, DOMAIN_ALIAS_RID_POWER_USERS, 0, 0, 0, 0, 0,
- 0, &dropSids[1].Sid))
+ 0, &dropSids[1].Sid) ||
+ !AllocateAndInitializeSid(&NtAuthority, 1,
+ SECURITY_SERVICE_RID, 0, 0, 0, 0, 0, 0,
+ 0, &dropSids[2].Sid))
{
write_stderr(_("%s: could not allocate SIDs: error code %lu\n"),
progname, (unsigned long) GetLastError());
@@ -1805,6 +1808,7 @@ CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION *processInfo, bool as_ser
0, NULL,
&restrictedToken);
+ FreeSid(dropSids[2].Sid);
FreeSid(dropSids[1].Sid);
FreeSid(dropSids[0].Sid);
CloseHandle(origToken);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
