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

On Sep 18, 7:31 pm, jmor(dot)(dot)(dot)(at)coverity(dot)com ("Jesse Morris") wrote:
> The following bug has been logged online:
>
> Bug reference:      5065
> Logged by:          Jesse Morris
> Email address:      jmor(dot)(dot)(dot)(at)coverity(dot)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.c 2009-06-11
07:49:07.000000000 -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.c 2009-09-01
19:40:59.000000000 -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.000000000 -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.c 2009-06-11
07:49:15.000000000 -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 hProcess)
+ * AddUserToTokenDacl(HANDLE hToken)
*
- * This function adds the current user account to the default DACL
- * which gets attached to the restricted token used when we create
- * a restricted process.
+ * This function adds the current user account to the restricted
+ * token used when we create a restricted process.
*
* This is required because of some security changes in Windows
* that appeared in patches to XP/2K3 and in Vista/2008.
@@ -681,13 +680,13 @@ set_pglocale_pgservice(const char *argv0
* and CreateProcess() calls when running as Administrator.
*
* This function fixes this problem by modifying the DACL of the
- * specified process and explicitly re-adding the current user
account.
- * This is still secure because the Administrator account inherits
it's
- * privileges from the Administrators group - it doesn't have any of
- * it's own.
+ * token the process will use, and explicitly re-adding the current
+ * user account. This is still secure because the Administrator
account
+ * inherits it's privileges from the Administrators group - it
doesn't
+ * have any of its own.
*/
BOOL
-AddUserToDacl(HANDLE hProcess)
+AddUserToTokenDacl(HANDLE hToken)
{
int i;
ACL_SIZE_INFORMATION asi;
@@ -695,7 +694,6 @@ AddUserToDacl(HANDLE hProcess)
DWORD dwNewAclSize;
DWORD dwSize = 0;
DWORD dwTokenInfoLength = 0;
- HANDLE hToken = NULL;
PACL pacl = NULL;
PSID psidUser = NULL;
TOKEN_DEFAULT_DACL tddNew;
@@ -703,13 +701,6 @@ AddUserToDacl(HANDLE hProcess)
TOKEN_INFORMATION_CLASS tic = TokenDefaultDacl;
BOOL ret = FALSE;

- /* Get the token for the process */
- if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_DEFAULT,
&hToken))
- {
- log_error("could not open process token: %lu", GetLastError());
- goto cleanup;
- }
-
/* Figure out the buffer size for the DACL info */
if (!GetTokenInformation(hToken, tic, (LPVOID) NULL,
dwTokenInfoLength, &dwSize))
{
@@ -785,8 +776,8 @@ AddUserToDacl(HANDLE hProcess)
}

/* Add the new ACE for the current user */
- if (!AddAccessAllowedAce(pacl, ACL_REVISION, GENERIC_ALL, psidUser))
- {
+ if (!AddAccessAllowedAceEx(pacl, ACL_REVISION, OBJECT_INHERIT_ACE,
GENERIC_ALL, psidUser))
+ {
log_error("could not add access allowed ACE: %lu", GetLastError());
goto cleanup;
}
@@ -812,9 +803,6 @@ cleanup:
if (ptdd)
LocalFree((HLOCAL) ptdd);

- if (hToken)
- CloseHandle(hToken);
-
return ret;
}

diff -rup unfixed/postgresql-8.4.1/src/test/regress/pg_regress.c fixed/
postgresql-8.4.1/src/test/regress/pg_regress.c
--- unfixed/postgresql-8.4.1/src/test/regress/pg_regress.c 2009-06-11
07:49:15.000000000 -0700
+++ fixed/postgresql-8.4.1/src/test/regress/pg_regress.c 2009-10-15
14:03:51.609110000 -0700
@@ -1021,6 +1021,10 @@ spawn_process(const char *cmdline)
cmdline2 = malloc(strlen(cmdline) + 8);
sprintf(cmdline2, "cmd /c %s", cmdline);

+#ifndef __CYGWIN__
+ AddUserToTokenDacl(restrictedToken);
+#endif
+
if (!CreateProcessAsUser(restrictedToken,
NULL,
cmdline2,
@@ -1038,10 +1042,6 @@ spawn_process(const char *cmdline)
exit_nicely(2);
}

-#ifndef __CYGWIN__
- AddUserToDacl(pi.hProcess);
-#endif
-
free(cmdline2);

ResumeThread(pi.hThread);
------end patch------