winpgnt.c: handle arbitrarily large file mappings.

I heard recently that at least one third-party client of Pageant
exists, and that it's used to generate signatures to use with TLS
client certificates. Apparently the signature scheme is compatible,
but TLS tends to need signatures over more data than will fit in
AGENT_MAX_MSGLEN.

Before the BinarySink refactor in commit b6cbad89f, this was OK
because the Windows Pageant IPC didn't check the size of the _input_
message against AGENT_MAX_MSGLEN, only the output one. But then we
started checking both, so that third-party TLS client started failing.

Now we use VirtualQuery to find out the actual size of the file
mapping we've been passed, and our only requirement is that the input
and output messages should both fit in _that_. So TLS should work
again, and also, other clients should be able to retrieve longer lists
of public keys if they pass a larger file mapping.

One side effect of this change is that Pageant's reply message is now
written directly into the shared-memory region. Previously, it was
written into a separate buffer and then memcpy()ed over after
pageant_handle_msg returned, but now the buffer is variable-size, it
seems to me to make more sense to avoid that extra not-entirely
controlled malloc. So I've done one very small reordering of
statements in the cross-platform pageant_handle_msg(), which fixes the
only case I could find where that function started writing its output
before it had finished using the contents of the input buffer.

pageant.c

@@ -339,11 +339,11 @@ void pageant_handle_msg(BinarySink *bs,
                 return;
             }
 
-            put_byte(bs, SSH2_AGENT_SIGN_RESPONSE);
-
             signature = strbuf_new();
             ssh_key_sign(key->key, sigdata.ptr, sigdata.len,
                          BinarySink_UPCAST(signature));
+
+            put_byte(bs, SSH2_AGENT_SIGN_RESPONSE);
             put_stringsb(bs, signature);
 
             plog(logctx, logfn, "reply: SSH2_AGENT_SIGN_RESPONSE");

windows/winpgnt.c

@@ -4,6 +4,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <stddef.h>
 #include <ctype.h>
 #include <assert.h>
 #include <tchar.h>
@@ -777,8 +778,9 @@ PSID get_default_sid(void)
 #endif
 
 struct PageantReply {
-    char buf[AGENT_MAX_MSGLEN - 4];
-    int len, overflowed;
+    char *buf;
+    size_t size, len;
+    int overflowed;
     BinarySink_IMPLEMENTATION;
 };
 
@@ -786,7 +788,7 @@ static void pageant_reply_BinarySink_write(
     BinarySink *bs, const void *data, size_t len)
 {
     struct PageantReply *rep = BinarySink_DOWNCAST(bs, struct PageantReply);
-    if (!rep->overflowed && len <= sizeof(rep->buf) - rep->len) {
+    if (!rep->overflowed && len <= rep->size - rep->len) {
         memcpy(rep->buf + rep->len, data, len);
         rep->len += len;
     } else {
@@ -799,7 +801,7 @@ static char *answer_filemapping_message(const char *mapname)
     HANDLE maphandle = INVALID_HANDLE_VALUE;
     void *mapaddr = NULL;
     char *err = NULL;
-    unsigned char *msg;
+    size_t mapsize;
     unsigned msglen;
     struct PageantReply reply;
 
@@ -810,6 +812,8 @@ static char *answer_filemapping_message(const char *mapname)
     PSECURITY_DESCRIPTOR psd = NULL;
 #endif
 
+    reply.buf = NULL;
+
 #ifdef DEBUG_IPC
     debug(("mapname = \"%s\"\n", mapname));
 #endif
@@ -886,6 +890,31 @@ static char *answer_filemapping_message(const char *mapname)
     debug(("mapped address = %p\n", mapaddr));
 #endif
 
+    {
+        MEMORY_BASIC_INFORMATION mbi;
+        size_t mbiSize = VirtualQuery(mapaddr, &mbi, sizeof(mbi));
+        if (mbiSize == 0) {
+            err = dupprintf("unable to query view of file mapping: %s",
+                            win_strerror(GetLastError()));
+            goto cleanup;
+        }
+        if (mbiSize < (offsetof(MEMORY_BASIC_INFORMATION, RegionSize) +
+                       sizeof(mbi.RegionSize))) {
+            err = dupstr("VirtualQuery returned too little data to get "
+                         "region size");
+            goto cleanup;
+        }
+
+        mapsize = mbi.RegionSize;
+    }
+#ifdef DEBUG_IPC
+    debug(("region size = %zd\n", mapsize));
+#endif
+    if (mapsize < 5) {
+        err = dupstr("mapping smaller than smallest possible request");
+        goto cleanup;
+    }
+
     msglen = GET_32BIT((unsigned char *)mapaddr);
 
 #ifdef DEBUG_IPC
@@ -893,17 +922,19 @@ static char *answer_filemapping_message(const char *mapname)
            msglen, (unsigned)((unsigned char *) mapaddr)[4]));
 #endif
 
+    reply.buf = (char *)mapaddr + 4;
+    reply.size = mapsize - 4;
     reply.len = 0;
     reply.overflowed = FALSE;
     BinarySink_INIT(&reply, pageant_reply_BinarySink_write);
 
-    if (msglen > AGENT_MAX_MSGLEN - 4) {
+    if (msglen > mapsize - 4) {
         pageant_failure_msg(BinarySink_UPCAST(&reply),
                             "incoming length field too large", NULL, NULL);
     } else {
         pageant_handle_msg(BinarySink_UPCAST(&reply),
-                           msg + 4, msglen, NULL, NULL);
-        if (reply.overflowed || reply.len > AGENT_MAX_MSGLEN - 4) {
+                           (unsigned char *)mapaddr + 4, msglen, NULL, NULL);
+        if (reply.overflowed) {
             reply.len = 0;
             reply.overflowed = FALSE;
             pageant_failure_msg(BinarySink_UPCAST(&reply),
@@ -912,19 +943,13 @@ static char *answer_filemapping_message(const char *mapname)
         }
     }
 
-    if (reply.len > AGENT_MAX_MSGLEN - 4) {
-        err = dupstr("even error-message output overflows buffer");
+    if (reply.overflowed) {
+        err = dupstr("even failure message overflows buffer");
         goto cleanup;
     }
 
-    /*
-     * Windows Pageant answers messages in place, by overwriting the
-     * input message buffer.
-     */
-    assert(4 + reply.len <= AGENT_MAX_MSGLEN);
-    PUT_32BIT(msg, reply.len);
-    memcpy(msg + 4, reply.buf, reply.len);
-    smemclr(reply.buf, sizeof(reply.buf));
+    /* Write in the initial length field, and we're done. */
+    PUT_32BIT(((unsigned char *)mapaddr), reply.len);
 
   cleanup:
     /* expectedsid has the lifetime of the program, so we don't free it */