[BUGS] BUG #5065: pg_ctl start fails as administrator, with "could not locate matching postgres executable"

2009-09-18 Thread Jesse Morris

The following bug has been logged online:

Bug reference:  5065
Logged by:  Jesse Morris
Email address:  jmor...@coverity.com
PostgreSQL version: 8.3.7, 8.4.1
Operating system:   Windows Server 2003 R2
Description:pg_ctl start fails as administrator, with "could not
locate matching postgres executable"
Details: 

I am logged in as domain\jmorris, a member of the local Administrators
group.

I am not running postgres as a service.

I can reproduce this with the zip file from enterprisedb for 8.3.7, or
8.4.1.   I also used msys/mingw to build my own (to instrument debugging
output) and that reproduces it as well.

>From cmd.exe:
initdb.exe works fine.
pg_ctl start complains "FATAL: postgres - could not locate matching postgres
executable" 

Instrumentation and investigation reveals that the failure is in
find_other_exec (exec.c) as the error text implies, but ultimately in
pipe_read_line; CreatePipe fails with error 5 (Access Denied).  

HOWEVER!
If I run pg_ctl from cygwin (or a cmd process that has cygwin or cygstart or
anything cygwinish as a parent) it works as expected.  

Almost certainly related:  AddUserToDacl (also in exec.c) has comments
explaining that what it does is necessary because otherwise on newer windows
we'll see access denied on CreatePipe.  However AddUserToDacl appears to be
working as written; none of the error paths are visited anyway. 

I can reproduce this on a number of separate win2k3 R2 SP2 boxes, including
one VM that is basically a fresh OS install.

So:  Two bugs:

* pg_ctl's manages to shoot itself in the foot when relinquishing
administrator rights (unless there's cygwin?)

* the error-reporting for this is extremely misleading.

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


[BUGS] Re: BUG #5065: pg_ctl start fails as administrator, with "could not locate matching postgres executable"

2009-10-15 Thread Jesse Morris
On Sep 18, 7:31 pm, jmor...@coverity.com ("Jesse Morris") wrote:
> The following bug has been logged online:
>
> Bug reference:      5065
> Logged by:          Jesse Morris
> Email address:      jmor...@coverity.com
> PostgreSQL version: 8.3.7, 8.4.1
> Operating system:   Windows Server 2003 R2
> Description:        pg_ctl start fails as administrator, with "could not
> locate matching postgres executable"
> Details:
>
> I am logged in as domain\jmorris, a member of the local Administrators
> group.
>
...
>
> From cmd.exe:
> initdb.exe works fine.
> pg_ctl start complains "FATAL: postgres - could not locate matching postgres
> executable"
>
> Instrumentation and investigation reveals that the failure is in
> find_other_exec (exec.c) as the error text implies, but ultimately in
> pipe_read_line; CreatePipe fails with error 5 (Access Denied).  
...

I went back to the version that supposedly initially fixed this issue,
but I couldn't get it to work either.  So I think the DACL adjustment
code was always broken.  The DACL stuff that both Cygwin and Active
Perl use to simulate *nix file permissions masks this error, so any
test framework that uses them would get false negatives on this bug.
Since these DACLs are inheritable, a workaround is to run pg_ctl as a
child process of Active Perl or Cygwin.   The comments indicated
pg_ctl & initdb were already trying to do the same thing themselves
(that is, add the current user to the DACLs) but it didn't actually
work on any of the systems I tried it on.

I think that a number of other people have seen this bug; search for
"FATAL: postgres - could not locate matching postgres executable."
But that message is so misleading is probably why it seems nobody has
properly diagnosed it as a permissions issue before.  I didn't do
anything to fix pg_ctl's error reporting.  :D

The patch:

--begin patch--

diff -rup unfixed/postgresql-8.4.1/src/bin/initdb/initdb.c fixed/
postgresql-8.4.1/src/bin/initdb/initdb.c
--- unfixed/postgresql-8.4.1/src/bin/initdb/initdb.c2009-06-11
07:49:07.0 -0700
+++ fixed/postgresql-8.4.1/src/bin/initdb/initdb.c  2009-10-15
16:31:12.651226900 -0700
@@ -2392,6 +2392,10 @@ CreateRestrictedProcess(char *cmd, PROCE
fprintf(stderr, "Failed to create restricted token: %lu\n",
GetLastError());
return 0;
}
+
+#ifndef __CYGWIN__
+AddUserToTokenDacl(restrictedToken);
+#endif

if (!CreateProcessAsUser(restrictedToken,
 NULL,
@@ -2409,11 +2413,7 @@ CreateRestrictedProcess(char *cmd, PROCE
fprintf(stderr, "CreateProcessAsUser failed: %lu\n", 
GetLastError
());
return 0;
}
-
-#ifndef __CYGWIN__
-   AddUserToDacl(processInfo->hProcess);
-#endif
-
+
return ResumeThread(processInfo->hThread);
 }
 #endif
Only in fixed/postgresql-8.4.1/src/bin/initdb: initdb.c.bak
diff -rup unfixed/postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c fixed/
postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c
--- unfixed/postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c2009-09-01
19:40:59.0 -0700
+++ fixed/postgresql-8.4.1/src/bin/pg_ctl/pg_ctl.c  2009-10-15
16:31:00.096971600 -0700
@@ -1389,7 +1389,10 @@ CreateRestrictedProcess(char *cmd, PROCE
write_stderr("Failed to create restricted token: %lu\n",
GetLastError());
return 0;
}
-
+#ifndef __CYGWIN__
+   AddUserToTokenDacl(restrictedToken);
+#endif
+
r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL,
TRUE, CREATE_SUSPENDED, NULL, NULL, &si, processInfo);

Kernel32Handle = LoadLibrary("KERNEL32.DLL");
@@ -1488,9 +1491,6 @@ CreateRestrictedProcess(char *cmd, PROCE
}
}

-#ifndef __CYGWIN__
-   AddUserToDacl(processInfo->hProcess);
-#endif

CloseHandle(restrictedToken);

Only in fixed/postgresql-8.4.1/src/bin/pg_ctl: pg_ctl.c.bak
diff -rup unfixed/postgresql-8.4.1/src/include/port.h fixed/
postgresql-8.4.1/src/include/port.h
--- unfixed/postgresql-8.4.1/src/include/port.h 2009-06-11
07:49:08.0 -0700
+++ fixed/postgresql-8.4.1/src/include/port.h   2009-10-15
14:02:36.860635900 -0700
@@ -81,7 +81,7 @@ extern int find_other_exec(const char *a

 /* Windows security token manipulation (in exec.c) */
 #ifdef WIN32
-extern BOOL AddUserToDacl(HANDLE hProcess);
+extern BOOL AddUserToTokenDacl(HANDLE hToken);
 #endif


diff -rup unfixed/postgresql-8.4.1/src/port/exec.c fixed/
postgresql-8.4.1/src/port/exec.c
--- unfixed/postgresql-8.4.1/src/port/exec.c2009-06-11
07:49:15.0 -0700
+++ fixed/postgresql-8.4.1/src/port/exec.c  2009-10-15
16:02:04.352805300 -0700
@@ -664,11 +664,10 @@ set_pglocale_pgservice(const char *argv0
 #ifdef WIN32

 /*
- * AddUserToDacl(HANDLE hProces

Re: [BUGS] Re: BUG #5065: pg_ctl start fails as administrator, with "could not locate matching postgres executable"

2009-10-16 Thread Jesse Morris
> -Original Message-
> From: Dave Page [mailto:dp...@pgadmin.org]
> Sent: Friday, October 16, 2009 2:14 AM
> To: Jesse Morris
> Cc: pgsql-bugs@postgresql.org
> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
administrator,
> with "could not locate matching postgres executable"
>
> > The patch:
> >
> > --begin patch--
> 
> :-(. Unfortunately inlining the patch in the email has munged it
> beyond usability. Can you resend it as an attachment please?

Oops!  Re-sent, as an attachment.  
 


postgres-bug-5065.patch
Description: postgres-bug-5065.patch

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


Re: [BUGS] Re: BUG #5065: pg_ctl start fails as administrator, with "could not locate matching postgres executable"

2009-10-19 Thread Jesse Morris
Andrew,

First the really important stuff:  I (correctly) changed the comment
from "it's" to "its" in one place, but didn't notice the other.  

As to "is doing this safe?" I'm not sure whether you are asking about my
patch, or about the general approach that I made the slight tweaks to,
but it's probably worth talking about both.  I'm going to start with
describing how I understand what's going on. If my understanding of the
windows security model is wrong, then my patch might be flawed.  If my
understanding of the windows security model is right, then the general
approach which I made minor changes to might be flawed.  
 
How a thread / process interacts with the system is determined by its
security token.  This token belongs to a user owner and a list of
groups.  The token also has an list of rights  (e.g.,
"SeTakeOwnershipPrivilege"), and a default DACL.  The default DACL is
like a umask; new objects the process creates will have that DACL.

Postmaster (that's what I'm going to call postgres.exe) checks that its
token does not belong to the Administrators or Power Users groups, and
refuses to run if it is.  

pg_ctl/initdb both run postmaster with a restricted token that disables
the Administrators and Power User groups.  Postmaster is satisfied with
this.

However, by default on Win2k3 (and possibly elsewhere?), Administrators'
processes' default DACLs only grants access to {System, Administrators}.
That's the whole list.  Postmaster spawns a child postmaster, and they
create a pipe to communicate.  But the postmaster is not owned by System
and pg_ctl disabled its Administrators membership, so that pipe is not
accessible: Access denied. 

On the Vista and Windows 7 machines I've looked at there's also a Login
SID (S-1-5-5-0-) in both the token's group membership and in
the default DACL.  In that case both parent and child process can access
the created pipe and there are no problems.  Cygwin, ActivePerl, and
msys all add the user that owns the process to the default DACL, so if
you run pg_ctl with one of those above you in the process tree, it will
work fine.  

The code I changed was trying to add the user to the DACL.  It doesn't
grant the postmaster processes any additional inherent rights, it just
says "your children processes are all accessible by the creating user
(which also has full access to the database cluster on disk anyway).  I
didn't write it though; I just changed how it was doing that.  I was
never able to get the original code to work at all, though Dave Page
says he can't even reproduce the failure, so I don't know what we're
doing differently.  MSDN says there's a "SeAssignPrimaryTokenPrivilege"
which is required to replace a process-level token, so maybe that's why
it's wasn't working for me - but I wasn't seeing any win32 calls fail,
so I dunno.  

Now:  To answer your question of "What if the Administrator user is
granted additional permissions?"  

The answer is the same with and without my patch.  If postgres is run as
a user that has additional permissions, postgres will have additional
permissions.  It is unusual to do this though; usually that is all
managed through group membership.  "Administrator" is not special in
this regard; I can give the "guest" account full control over everything
if I want. 

If you do want to defend against that (personally I'm indifferent), it
wouldn't be difficult to drop unnecessary permissions from the
restricted token that the daemon is already being run with.  Postmaster
can verify that anything "dangerous" is not allowed from the backend
startup check.  Actually, I'm not sure if *any* of these are necessary:
http://msdn.microsoft.com/en-us/library/bb530716%28VS.85%29.aspx

This wouldn't be a substitute for disabling the Administrators and Power
Users group membership; those still affect a lot of important files.   

I personally don't need postgres to be more defensive, I just need it to
start successfully.  So even if the above IS a good idea, someone else
will have to implement it.  

And by all means, include whatever of this is correct/helpful in code
comments.  I don't have to submit a separate patch for that, do I?

-Jesse

> -Original Message-
> From: Andrew Dunstan [mailto:and...@dunslane.net]
> Sent: Monday, October 19, 2009 11:03 AM
> To: Dave Page
> Cc: Jesse Morris; pgsql-bugs@postgresql.org; Magnus Hagander
> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
administrator,
> with "could not locate matching postgres executable"
> 
> It looks OK to me (modulo the incorrect changing of "its" to "it's" in
> a comment - whoever did that was trying to make it consistent, but
> unfortunately made it consistently

Re: [BUGS] Re: BUG #5065: pg_ctl start fails as administrator, with "could not locate matching postgres executable"

2009-10-20 Thread Jesse Morris
I've seen it happen on a wide range of win2k3 servers.  From a fresh
image with nothing but VMWare tools under Add/Remove Programs, to a
heavily used shared dev server with MSVC, Cygwin, and tons of other
stuff.  Some of our beta testers have also seen the issue.  In fact I
can't really think of any win2k3 machine where it *had* worked.  I
suspect the main reason reports of this failure are relatively rare is
that most people use the one-click installer which sets it up to run as
a non-administrator service account.  (That's superior in many ways, but
it is not always an option for me.)

It looks like something that happens with Win2k3 only; XP had the user
instead of Administrators in the DACL, and Vista appears to have the
Session.  I guess that's the only place that "AddUserToDacl" was even
necessary in the first place?  I dunno.

> -Original Message-
> From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> Sent: Tuesday, October 20, 2009 9:45 AM
> To: Dave Page
> Cc: Andrew Dunstan; Jesse Morris; pgsql-bugs@postgresql.org; Magnus
> Hagander
> Subject: Re: [BUGS] Re: BUG #5065: pg_ctl start fails as
administrator,
> with "could not locate matching postgres executable"
> 
> Dave Page  writes:
> > On Tue, Oct 20, 2009 at 3:48 PM, Tom Lane  wrote:
> >> Do we have any idea why?
> 
> > Honestly? No. I have a vague hand-wavy idea about there being
> > something preventing us properly modifying the token of an existing
> > process in some configurations, but nothing even remotely
jello-like,
> > let alone concrete.
> 
> After re-reading the thread I am struck by Jesse's comment that the
> current coding never worked at all for him.  It seems like that must
> indicate an environment difference or installed-software difference
> compared to the setups where it does work.  (Antivirus maybe?)
> 
> Seems like it would be worth the trouble to identify exactly what the
> critical difference is.
> 
>   regards, tom lane

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