Thread: Patch to add Windows 7 support

Patch to add Windows 7 support

From
Dave Page
Date:
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

Attachment

Re: Patch to add Windows 7 support

From
Heikki Linnakangas
Date:
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


Re: Patch to add Windows 7 support

From
Dave Page
Date:
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


Re: Patch to add Windows 7 support

From
Peter Eisentraut
Date:
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.


Re: Patch to add Windows 7 support

From
Magnus Hagander
Date:
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


Re: Patch to add Windows 7 support

From
Dave Page
Date:
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


Re: Patch to add Windows 7 support

From
Dave Page
Date:
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


Re: Patch to add Windows 7 support

From
Tom Lane
Date:
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


Re: Patch to add Windows 7 support

From
Magnus Hagander
Date:
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



Re: Patch to add Windows 7 support

From
Dave Page
Date:
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


Re: Patch to add Windows 7 support

From
Tom Lane
Date:
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


Re: Patch to add Windows 7 support

From
Magnus Hagander
Date:
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;