Обсуждение: Patch to add Windows 7 support
The attached patch adds support for the Windows 7 beta which we've had a few reports of incompatibility with. When we startup using pg_ctl on Windows, we create a job object (a logical grouping of processes on Windows) to which we apply various security options. One of these (JOB_OBJECT_UILIMIT_HANDLES) is used to prevent our processes seeing handles belonging to processes outside of our job, however, when we run under the service control manager, this causes the postmaster to exit immeditately for no apparent reason. I'm not entirely sure what has change in the SCM to cause this yet (Windows 7 documentation is somewhat thin on the ground at the moment), but the patch avoids theporblem by only setting JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. Tested on CVS head, but should probably be backpatched to 8.3 to avoid more bug reports. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Вложения
Dave Page wrote: > The attached patch adds support for the Windows 7 beta which we've had > a few reports of incompatibility with. When we startup using pg_ctl on > Windows, we create a job object (a logical grouping of processes on > Windows) to which we apply various security options. One of these > (JOB_OBJECT_UILIMIT_HANDLES) is used to prevent our processes seeing > handles belonging to processes outside of our job, however, when we > run under the service control manager, this causes the postmaster to > exit immeditately for no apparent reason. > > I'm not entirely sure what has change in the SCM to cause this yet > (Windows 7 documentation is somewhat thin on the ground at the > moment), but the patch avoids theporblem by only setting > JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. > > Tested on CVS head, but should probably be backpatched to 8.3 to avoid > more bug reports. No objections here, but seems we should revisit this after Windows 7 is released, and revert if they've fixed the underlying problem during the beta period. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On Tue, Jan 27, 2009 at 11:04 AM, Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> wrote: > Dave Page wrote: >> >> The attached patch adds support for the Windows 7 beta which we've had >> a few reports of incompatibility with. When we startup using pg_ctl on >> Windows, we create a job object (a logical grouping of processes on >> Windows) to which we apply various security options. One of these >> (JOB_OBJECT_UILIMIT_HANDLES) is used to prevent our processes seeing >> handles belonging to processes outside of our job, however, when we >> run under the service control manager, this causes the postmaster to >> exit immeditately for no apparent reason. >> >> I'm not entirely sure what has change in the SCM to cause this yet >> (Windows 7 documentation is somewhat thin on the ground at the >> moment), but the patch avoids theporblem by only setting >> JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. >> >> Tested on CVS head, but should probably be backpatched to 8.3 to avoid >> more bug reports. > > No objections here, but seems we should revisit this after Windows 7 is > released, and revert if they've fixed the underlying problem during the beta > period. Agreed. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Tuesday 27 January 2009 12:34:56 Dave Page wrote: > I'm not entirely sure what has change in the SCM to cause this yet > (Windows 7 documentation is somewhat thin on the ground at the > moment), but the patch avoids theporblem by only setting > JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. Doesn't this effectively mean, we relax the security settings because we don't understand why we are getting errors? Sounds fishy.
Peter Eisentraut wrote: > On Tuesday 27 January 2009 12:34:56 Dave Page wrote: >> I'm not entirely sure what has change in the SCM to cause this yet >> (Windows 7 documentation is somewhat thin on the ground at the >> moment), but the patch avoids theporblem by only setting >> JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. Does the SCM just terminate the process, or does it at some point give us an error code? > Doesn't this effectively mean, we relax the security settings because we don't > understand why we are getting errors? Sounds fishy. In theory, yes. However, it only affects USER handles, which is basically windows, buttons etc. So what it basically means is that a hacked PostgreSQL process can be used to send messages, for example, to other windows running in the same session. Which only affects things if pg is started from the commandline, and not as a service. I don't think it's enough that we need to care about it really. I'm thinking we could perhaps even just never set that, and not bother with the version check... But perhaps we should set it only when launching as a service, and not when running from the commandline? //Magnus
On Tue, Jan 27, 2009 at 11:26 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On Tuesday 27 January 2009 12:34:56 Dave Page wrote: >> I'm not entirely sure what has change in the SCM to cause this yet >> (Windows 7 documentation is somewhat thin on the ground at the >> moment), but the patch avoids theporblem by only setting >> JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. > > Doesn't this effectively mean, we relax the security settings because we don't > understand why we are getting errors? Sounds fishy. Yes, essentially. I have a suspicion that Microsoft have tightened the security of that option, such that if we use it we can no longer see the handle to the service control manager (which it owns of course), but I have no way to prove that. However; - We only use job objects on >= XP. On Windows 2000/NT4, we don't use them at all so we don't set any of the related security options on those platforms. - I don't believe this option gives us much additional security. It doesn't secure PostgreSQL in any way, it prevents PostgreSQL from seeing the user handles owned by other jobs in the same session. To make any use of those, the PostgreSQL installation would have to be severely compromised anyway, which would give other, easier paths into the system, besides which, when running as a service we're in our own session anyway. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
On Tue, Jan 27, 2009 at 11:38 AM, Magnus Hagander <magnus@hagander.net> wrote: > Peter Eisentraut wrote: >> On Tuesday 27 January 2009 12:34:56 Dave Page wrote: >>> I'm not entirely sure what has change in the SCM to cause this yet >>> (Windows 7 documentation is somewhat thin on the ground at the >>> moment), but the patch avoids theporblem by only setting >>> JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. > > Does the SCM just terminate the process, or does it at some point give > us an error code? Just the ubiquitous DLL initialization error. > I don't think it's enough that we need to care about it really. I'm > thinking we could perhaps even just never set that, and not bother with > the version check... That was how I originally coded it, but figured we might as well set it if we can - it's not like it's expensive to do. > But perhaps we should set it only when launching as a service, and not > when running from the commandline? We could. I'm not sure there's a great deal of need - most people will run as a service, and it won't make any difference for those that start the postmaster directly. -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page <dpage@pgadmin.org> writes:
> The attached patch adds support for the Windows 7 beta which we've had
> a few reports of incompatibility with. When we startup using pg_ctl on
> Windows, we create a job object (a logical grouping of processes on
> Windows) to which we apply various security options. One of these
> (JOB_OBJECT_UILIMIT_HANDLES) is used to prevent our processes seeing
> handles belonging to processes outside of our job, however, when we
> run under the service control manager, this causes the postmaster to
> exit immeditately for no apparent reason.
> I'm not entirely sure what has change in the SCM to cause this yet
> (Windows 7 documentation is somewhat thin on the ground at the
> moment), but the patch avoids theporblem by only setting
> JOB_OBJECT_UILIMIT_HANDLES on earlier OSs.
It would be good to understand what the problem actually is and what are
the risks of running without this flag. I assume we put it in there
for a reason.
regards, tom lane
Tom Lane wrote: > Dave Page <dpage@pgadmin.org> writes: >> The attached patch adds support for the Windows 7 beta which we've had >> a few reports of incompatibility with. When we startup using pg_ctl on >> Windows, we create a job object (a logical grouping of processes on >> Windows) to which we apply various security options. One of these >> (JOB_OBJECT_UILIMIT_HANDLES) is used to prevent our processes seeing >> handles belonging to processes outside of our job, however, when we >> run under the service control manager, this causes the postmaster to >> exit immeditately for no apparent reason. > >> I'm not entirely sure what has change in the SCM to cause this yet >> (Windows 7 documentation is somewhat thin on the ground at the >> moment), but the patch avoids theporblem by only setting >> JOB_OBJECT_UILIMIT_HANDLES on earlier OSs. > > It would be good to understand what the problem actually is and what are > the risks of running without this flag. I assume we put it in there > for a reason. Honestly, I think the reason was simply "enable all flags that we don't know will break things for us". There was no closer analysis than that. //Magnus
On Tue, Jan 27, 2009 at 3:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > It would be good to understand what the problem actually is and what are > the risks of running without this flag. I assume we put it in there > for a reason. The risks are pretty low imho. Not having the flag means that the server has access to the handles of objects in other jobs in the same session. When running as a service, that's basically nothing as the service runs in it's own session and is isolated through other means. When run from the command line, a hacked binary could send messages to the users UI (buttons and other controls for example). It would be a difficult attack to pull off, and very hard to gain from it. It's easy enough to leave the flag in console mode however - that does work on Windows 7. As for what the problem actually is - without more info from Microsoft, I suspect we won't find out (and even then, I wouldn't hold my breath for something like this). -- Dave Page EnterpriseDB UK: http://www.enterprisedb.com
Dave Page <dpage@pgadmin.org> writes:
> The risks are pretty low imho. Not having the flag means that the
> server has access to the handles of objects in other jobs in the same
> session. When running as a service, that's basically nothing as the
> service runs in it's own session and is isolated through other means.
> When run from the command line, a hacked binary could send messages to
> the users UI (buttons and other controls for example). It would be a
> difficult attack to pull off, and very hard to gain from it. It's easy
> enough to leave the flag in console mode however - that does work on
> Windows 7.
Okay, but +1 for leaving it on in console mode.
regards, tom lane
Dave Page wrote:
>> I don't think it's enough that we need to care about it really. I'm
>> thinking we could perhaps even just never set that, and not bother with
>> the version check...
>
> That was how I originally coded it, but figured we might as well set
> it if we can - it's not like it's expensive to do.
>
>> But perhaps we should set it only when launching as a service, and not
>> when running from the commandline?
>
> We could. I'm not sure there's a great deal of need - most people will
> run as a service, and it won't make any difference for those that
> start the postmaster directly.
I have applied this updated patch. It simplifies the if branches a bit
(imho, that is), and also adds the switch to only make the change when
starting as a service.
//Magnus
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
***************
*** 121,127 **** static void pgwin32_SetServiceStatus(DWORD);
static void WINAPI pgwin32_ServiceHandler(DWORD);
static void WINAPI pgwin32_ServiceMain(DWORD, LPTSTR *);
static void pgwin32_doRunAsService(void);
! static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION * processInfo);
static SERVICE_STATUS status;
static SERVICE_STATUS_HANDLE hStatus = (SERVICE_STATUS_HANDLE) 0;
--- 121,127 ----
static void WINAPI pgwin32_ServiceHandler(DWORD);
static void WINAPI pgwin32_ServiceMain(DWORD, LPTSTR *);
static void pgwin32_doRunAsService(void);
! static int CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION * processInfo, bool as_service);
static SERVICE_STATUS status;
static SERVICE_STATUS_HANDLE hStatus = (SERVICE_STATUS_HANDLE) 0;
***************
*** 385,391 **** start_postmaster(void)
snprintf(cmd, MAXPGPATH, "CMD /C " SYSTEMQUOTE "\"%s\" %s%s < \"%s\" 2>&1" SYSTEMQUOTE,
postgres_path, pgdata_opt, post_opts, DEVNULL);
! if (!CreateRestrictedProcess(cmd, &pi))
return GetLastError();
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
--- 385,391 ----
snprintf(cmd, MAXPGPATH, "CMD /C " SYSTEMQUOTE "\"%s\" %s%s < \"%s\" 2>&1" SYSTEMQUOTE,
postgres_path, pgdata_opt, post_opts, DEVNULL);
! if (!CreateRestrictedProcess(cmd, &pi, false))
return GetLastError();
CloseHandle(pi.hProcess);
CloseHandle(pi.hThread);
***************
*** 1210,1216 **** pgwin32_ServiceMain(DWORD argc, LPTSTR * argv)
/* Start the postmaster */
pgwin32_SetServiceStatus(SERVICE_START_PENDING);
! if (!CreateRestrictedProcess(pgwin32_CommandLine(false), &pi))
{
pgwin32_SetServiceStatus(SERVICE_STOPPED);
return;
--- 1210,1216 ----
/* Start the postmaster */
pgwin32_SetServiceStatus(SERVICE_START_PENDING);
! if (!CreateRestrictedProcess(pgwin32_CommandLine(false), &pi, true))
{
pgwin32_SetServiceStatus(SERVICE_STOPPED);
return;
***************
*** 1313,1319 **** typedef BOOL(WINAPI * __QueryInformationJobObject) (HANDLE, JOBOBJECTINFOCLASS,
* automatically destroyed when pg_ctl exits.
*/
static int
! CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION * processInfo)
{
int r;
BOOL b;
--- 1313,1319 ----
* automatically destroyed when pg_ctl exits.
*/
static int
! CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION * processInfo, bool as_service)
{
int r;
BOOL b;
***************
*** 1449,1454 **** CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION * processInfo)
--- 1449,1455 ----
JOBOBJECT_BASIC_LIMIT_INFORMATION basicLimit;
JOBOBJECT_BASIC_UI_RESTRICTIONS uiRestrictions;
JOBOBJECT_SECURITY_LIMIT_INFORMATION securityLimit;
+ OSVERSIONINFO osv;
ZeroMemory(&basicLimit, sizeof(basicLimit));
ZeroMemory(&uiRestrictions, sizeof(uiRestrictions));
***************
*** 1459,1466 **** CreateRestrictedProcess(char *cmd, PROCESS_INFORMATION * processInfo)
_SetInformationJobObject(job, JobObjectBasicLimitInformation, &basicLimit, sizeof(basicLimit));
uiRestrictions.UIRestrictionsClass = JOB_OBJECT_UILIMIT_DESKTOP |
JOB_OBJECT_UILIMIT_DISPLAYSETTINGS|
! JOB_OBJECT_UILIMIT_EXITWINDOWS | JOB_OBJECT_UILIMIT_HANDLES |
JOB_OBJECT_UILIMIT_READCLIPBOARD|
JOB_OBJECT_UILIMIT_SYSTEMPARAMETERS | JOB_OBJECT_UILIMIT_WRITECLIPBOARD;
_SetInformationJobObject(job, JobObjectBasicUIRestrictions, &uiRestrictions,
sizeof(uiRestrictions));
securityLimit.SecurityLimitFlags = JOB_OBJECT_SECURITY_NO_ADMIN | JOB_OBJECT_SECURITY_ONLY_TOKEN;
--- 1460,1482 ----
_SetInformationJobObject(job, JobObjectBasicLimitInformation, &basicLimit, sizeof(basicLimit));
uiRestrictions.UIRestrictionsClass = JOB_OBJECT_UILIMIT_DESKTOP |
JOB_OBJECT_UILIMIT_DISPLAYSETTINGS|
! JOB_OBJECT_UILIMIT_EXITWINDOWS | JOB_OBJECT_UILIMIT_READCLIPBOARD |
JOB_OBJECT_UILIMIT_SYSTEMPARAMETERS | JOB_OBJECT_UILIMIT_WRITECLIPBOARD;
+
+ if (as_service)
+ {
+ osv.dwOSVersionInfoSize = sizeof(osv);
+ if (!GetVersionEx(&osv) ||
+ osv.dwMajorVersion < 6 ||
+ (osv.dwMajorVersion == 6 && osv.dwMinorVersion == 0))
+ {
+ /*
+ * On Windows 7 (and presumably later), JOB_OBJECT_UILIMIT_HANDLES prevents us from
+ * starting as a service. So we only enable it on Vista and earlier (version <= 6.0)
+ */
+ uiRestrictions.UIRestrictionsClass |= JOB_OBJECT_UILIMIT_HANDLES;
+ }
+ }
_SetInformationJobObject(job, JobObjectBasicUIRestrictions, &uiRestrictions,
sizeof(uiRestrictions));
securityLimit.SecurityLimitFlags = JOB_OBJECT_SECURITY_NO_ADMIN | JOB_OBJECT_SECURITY_ONLY_TOKEN;