Make Pageant use the same SID-selection logic as the Pageant client

code (as introduced in r9043), so that it uses the user SID rather
than the default SID.

This does change the access-control model, in that a Pageant running
with administrator privilege will now serve keys to an unprivileged
PuTTY running as the same user who started Pageant. Owen and I think
this isn't a problem (in particular, it will still not serve keys to a
_different_ user).

More importantly, making the Pageant client and server code work the
same way means that PuTTY and Pageant can still talk to each other
when UAC is turned off, which we've had several reports of r9043
having broken.

[originally from svn r9178]
[r9043 == 05f22632eb]

windows/winpgnt.c

@@ -19,6 +19,10 @@
 
 #ifndef NO_SECURITY
 #include <aclapi.h>
+#ifdef DEBUG_IPC
+#define _WIN32_WINNT 0x0500            /* for ConvertSidToStringSid */
+#include <sddl.h>
+#endif
 #endif
 
 #define IDI_MAINICON 200
@@ -113,7 +117,7 @@ static tree234 *rsakeys, *ssh2keys;
 
 static int has_security;
 #ifndef NO_SECURITY
-DECL_WINDOWS_FUNCTION(static, DWORD, GetSecurityInfo,
+DECL_WINDOWS_FUNCTION(extern, DWORD, GetSecurityInfo,
 		      (HANDLE, SE_OBJECT_TYPE, SECURITY_INFORMATION,
 		       PSID *, PSID *, PACL *, PACL *,
 		       PSECURITY_DESCRIPTOR *));
@@ -1817,8 +1821,7 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message,
 	    void *p;
 	    HANDLE filemap;
 #ifndef NO_SECURITY
-	    HANDLE proc;
+	    PSID mapowner, ourself;
-	    PSID mapowner, procowner;
 	    PSECURITY_DESCRIPTOR psd1 = NULL, psd2 = NULL;
 #endif
 	    int ret = 0;
@@ -1840,40 +1843,35 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message,
 #ifndef NO_SECURITY
 		int rc;
 		if (has_security) {
-		    if ((proc = OpenProcess(MAXIMUM_ALLOWED, FALSE,
+                    if ((ourself = get_user_sid()) == NULL) {
-					    GetCurrentProcessId())) ==
-			NULL) {
 #ifdef DEBUG_IPC
-			debug(("couldn't get handle for process\n"));
+			debug(("couldn't get user SID\n"));
 #endif
 			return 0;
-		    }
+                    }
-		    if (p_GetSecurityInfo(proc, SE_KERNEL_OBJECT,
+
-					  OWNER_SECURITY_INFORMATION,
-					  &procowner, NULL, NULL, NULL,
-					  &psd2) != ERROR_SUCCESS) {
-#ifdef DEBUG_IPC
-			debug(("couldn't get owner info for process\n"));
-#endif
-			CloseHandle(proc);
-			return 0;      /* unable to get security info */
-		    }
-		    CloseHandle(proc);
 		    if ((rc = p_GetSecurityInfo(filemap, SE_KERNEL_OBJECT,
 						OWNER_SECURITY_INFORMATION,
 						&mapowner, NULL, NULL, NULL,
 						&psd1) != ERROR_SUCCESS)) {
 #ifdef DEBUG_IPC
-			debug(
+			debug(("couldn't get owner info for filemap: %d\n",
-			      ("couldn't get owner info for filemap: %d\n",
+                               rc));
-			       rc));
 #endif
 			return 0;
 		    }
 #ifdef DEBUG_IPC
-		    debug(("got security stuff\n"));
+                    {
+                        LPTSTR ours, theirs;
+                        ConvertSidToStringSid(mapowner, &theirs);
+                        ConvertSidToStringSid(ourself, &ours);
+                        debug(("got both sids: ours=%s theirs=%s\n",
+                               ours, theirs));
+                        LocalFree(ours);
+                        LocalFree(theirs);
+                    }
 #endif
-		    if (!EqualSid(mapowner, procowner))
+		    if (!EqualSid(mapowner, ourself))
 			return 0;      /* security ID mismatch! */
 #ifdef DEBUG_IPC
 		    debug(("security stuff matched\n"));
@@ -1892,9 +1890,9 @@ static LRESULT CALLBACK WndProc(HWND hwnd, UINT message,
 		{
 		    int i;
 		    for (i = 0; i < 5; i++)
-			debug(
+			debug(("p[%d]=%02x\n", i,
-			      ("p[%d]=%02x\n", i,
+			       ((unsigned char *) p)[i]));
-			       ((unsigned char *) p)[i]));}
+                }
 #endif
 		answer_msg(p);
 		ret = 1;
@@ -1972,9 +1970,7 @@ int WINAPI WinMain(HINSTANCE inst, HINSTANCE prev, LPSTR cmdline, int show)
 	/*
 	 * Attempt to get the security API we need.
 	 */
-	advapi = load_system32_dll("advapi32.dll");
+        if (!init_advapi()) {
-	GET_WINDOWS_FUNCTION(advapi, GetSecurityInfo);
-	if (!p_GetSecurityInfo) {
 	    MessageBox(NULL,
 		       "Unable to access security APIs. Pageant will\n"
 		       "not run, in case it causes a security breach.",

windows/winpgntc.c

@@ -86,15 +86,70 @@ DECL_WINDOWS_FUNCTION(static, BOOL, InitializeSecurityDescriptor,
 		      (PSECURITY_DESCRIPTOR, DWORD));
 DECL_WINDOWS_FUNCTION(static, BOOL, SetSecurityDescriptorOwner,
 		      (PSECURITY_DESCRIPTOR, PSID, BOOL));
-static int init_advapi(void)
+DECL_WINDOWS_FUNCTION(, DWORD, GetSecurityInfo,
+		      (HANDLE, SE_OBJECT_TYPE, SECURITY_INFORMATION,
+		       PSID *, PSID *, PACL *, PACL *,
+		       PSECURITY_DESCRIPTOR *));
+int init_advapi(void)
 {
     advapi = load_system32_dll("advapi32.dll");
     return advapi &&
+	GET_WINDOWS_FUNCTION(advapi, GetSecurityInfo) &&
         GET_WINDOWS_FUNCTION(advapi, OpenProcessToken) &&
 	GET_WINDOWS_FUNCTION(advapi, GetTokenInformation) &&
 	GET_WINDOWS_FUNCTION(advapi, InitializeSecurityDescriptor) &&
 	GET_WINDOWS_FUNCTION(advapi, SetSecurityDescriptorOwner);
 }
+
+PSID get_user_sid(void)
+{
+    HANDLE proc = NULL, tok = NULL;
+    TOKEN_USER *user = NULL;
+    DWORD toklen, sidlen;
+    PSID sid = NULL, ret = NULL;
+
+    if ((proc = OpenProcess(MAXIMUM_ALLOWED, FALSE,
+                            GetCurrentProcessId())) == NULL)
+        goto cleanup;
+
+    if (!p_OpenProcessToken(proc, TOKEN_QUERY, &tok))
+        goto cleanup;
+
+    if (!p_GetTokenInformation(tok, TokenUser, NULL, 0, &toklen) &&
+        GetLastError() != ERROR_INSUFFICIENT_BUFFER)
+        goto cleanup;
+
+    if ((user = (TOKEN_USER *)LocalAlloc(LPTR, toklen)) == NULL)
+        goto cleanup;
+
+    if (!p_GetTokenInformation(tok, TokenUser, user, toklen, &toklen))
+        goto cleanup;
+
+    sidlen = GetLengthSid(user->User.Sid);
+
+    sid = (PSID)smalloc(sidlen);
+
+    if (!CopySid(sidlen, sid, user->User.Sid))
+        goto cleanup;
+
+    /* Success. Move sid into the return value slot, and null it out
+     * to stop the cleanup code freeing it. */
+    ret = sid;
+    sid = NULL;
+
+  cleanup:
+    if (proc != NULL)
+        CloseHandle(proc);
+    if (tok != NULL)
+        CloseHandle(tok);
+    if (user != NULL)
+        LocalFree(user);
+    if (sid != NULL)
+        sfree(sid);
+
+    return ret;
+}
+
 #endif
 
 int agent_query(void *in, int inlen, void **out, int *outlen,
@@ -108,8 +163,7 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
     COPYDATASTRUCT cds;
     SECURITY_ATTRIBUTES sa, *psa;
     PSECURITY_DESCRIPTOR psd = NULL;
-    HANDLE proc, tok;
+    PSID usersid = NULL;
-    TOKEN_USER *user = NULL;
 
     *out = NULL;
     *outlen = 0;
@@ -130,31 +184,16 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
          * run PSFTPs which refer back to the owning user's
          * unprivileged Pageant.
          */
-
+        usersid = get_user_sid();
-        if ((proc = OpenProcess(MAXIMUM_ALLOWED, FALSE,
-                                GetCurrentProcessId())) != NULL) {
-            if (p_OpenProcessToken(proc, TOKEN_QUERY, &tok)) {
-                DWORD retlen;
-                p_GetTokenInformation(tok, TokenUser, NULL, 0, &retlen);
-                user = (TOKEN_USER *)LocalAlloc(LPTR, retlen);
-                if (!p_GetTokenInformation(tok, TokenUser,
-                                           user, retlen, &retlen)) {
-                    LocalFree(user);
-                    user = NULL;
-                }
-                CloseHandle(tok);
-            }
-            CloseHandle(proc);
-        }
 
         psa = NULL;
-        if (user) {
+        if (usersid) {
             psd = (PSECURITY_DESCRIPTOR)
                 LocalAlloc(LPTR, SECURITY_DESCRIPTOR_MIN_LENGTH);
             if (psd) {
                 if (p_InitializeSecurityDescriptor
                     (psd, SECURITY_DESCRIPTOR_REVISION) &&
-                    p_SetSecurityDescriptorOwner(psd, user->User.Sid, FALSE)) {
+                    p_SetSecurityDescriptorOwner(psd, usersid, FALSE)) {
                     sa.nLength = sizeof(sa);
                     sa.bInheritHandle = TRUE;
                     sa.lpSecurityDescriptor = psd;
@@ -221,7 +260,6 @@ int agent_query(void *in, int inlen, void **out, int *outlen,
     CloseHandle(filemap);
     if (psd)
         LocalFree(psd);
-    if (user)
+    sfree(usersid);
-        LocalFree(user);
     return 1;
 }

windows/winstuff.h

@@ -497,7 +497,7 @@ int handle_backlog(struct handle *h);
 void *handle_get_privdata(struct handle *h);
 
 /*
- * pageantc.c needs to schedule callbacks for asynchronous agent
+ * winpgntc.c needs to schedule callbacks for asynchronous agent
  * requests. This has to be done differently in GUI and console, so
  * there's an exported function used for the purpose.
  * 
@@ -508,6 +508,14 @@ void agent_schedule_callback(void (*callback)(void *, void *, int),
 			     void *callback_ctx, void *data, int len);
 #define FLAG_SYNCAGENT 0x1000
 
+/*
+ * winpgntc.c also exports these two functions which are used by the
+ * server side of Pageant as well, to get the user SID for comparing
+ * with clients'.
+ */
+int init_advapi(void);  /* initialises everything needed by get_user_sid */
+PSID get_user_sid(void);
+
 /*
  * Exports from winser.c.
  */