gpg,gpgsm: Handle pkdecrypt responses with or without NUL terminators

* g10/call-agent.c (agent_pkdecrypt): accept but do not require
NUL-terminated data from the agent.
* sm/call-agent.c (gpgsm_agent_pkdecrypt): accept but do not require
NUL-terminated data from the agent.

--

The current code for both gpg and gpgsm assumes that gpg-agent will
return string terminated with a single NUL, even though the string
that it receives is also already length-delimited.  Since these tools
might be talking to an older version of gpg-agent, we want to continue
to make sense of such a response, but we really shouldn't depend on
it.  Rather, we can just strip off all trailing NULs and then treat
the remaining string as a proper S-expression.

We can't assume tha the S-expression itself is a NUL-terminated
string, because any of the canonically-represented objects could
contain a NUL byte internally.  But if it's a proper S-expression,
then it must actually terminate in a non-NUL ')' octet.

I note that gpgsm_agent_pkdecrypt() appears to try to work with older
versions of gpg-agent which might not return a full S-expression.
This makes it harder to reason about, since a maliciously-formed
return value could contain a string that could cause invalid memory
access when invoking strtoul (e.g. all numbers up to the end of the
buffer).  So we still have to manually NUL-terminate it before
continuing in that codepath.  This cleanup would be easier if we could
just assume that the agent will always return an S-expression.
Perhaps that could be a subsequent cleanup for gpgsm?  Do we expect
all versions of gpgsm to interoperate with all past versions of
gpg-agent?

gpg's agent_pkdecrypt() has no such qualms -- if the returned object
is not a full S-expression, then it rejects the response.  This makes
it much easier to reason about the pkdecrypt response without
modification, and allows us to strip any trailing NUL bytes knowing
that the response string will be properly terminated with a close
parenthesis.

GnuPG-bug-id: 4652
Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>

g10/call-agent.c

@@ -2323,25 +2323,28 @@ agent_pkdecrypt (ctrl_t ctrl, const char *keygrip, const char *desc,
       return err;
     }
 
-  put_membuf (&data, "", 1); /* Make sure it is 0 terminated.  */
   buf = get_membuf (&data, &len);
   if (!buf)
     return gpg_error_from_syserror ();
-  log_assert (len); /* (we forced Nul termination.)  */
 
-  if (*buf != '(')
+  if (len == 0 || *buf != '(')
     {
       xfree (buf);
       return gpg_error (GPG_ERR_INV_SEXP);
     }
 
-  if (len < 13 || memcmp (buf, "(5:value", 8) ) /* "(5:valueN:D)\0" */
+  if (len < 12 || memcmp (buf, "(5:value", 8) ) /* "(5:valueN:D)" */
     {
       xfree (buf);
       return gpg_error (GPG_ERR_INV_SEXP);
     }
-  len -= 10;   /* Count only the data of the second part. */
+  while (buf[len-1] == 0)
+    len--;
+  if (buf[len-1] != ')')
+    return gpg_error (GPG_ERR_INV_SEXP);
+  len--; /* Drop the final close-paren. */
   p = buf + 8; /* Skip leading parenthesis and the value tag. */
+  len -= 8;   /* Count only the data of the second part. */
 
   n = strtoul (p, &endp, 10);
   if (!n || *endp != ':')

sm/call-agent.c

@@ -477,7 +477,7 @@ gpgsm_agent_pkdecrypt (ctrl_t ctrl, const char *keygrip, const char *desc,
 {
   int rc;
   char line[ASSUAN_LINELENGTH];
-   membuf_t data;
+  membuf_t data;
   struct cipher_parm_s cipher_parm;
   size_t n, len;
   char *p, *buf, *endp;
@@ -528,7 +528,7 @@ gpgsm_agent_pkdecrypt (ctrl_t ctrl, const char *keygrip, const char *desc,
       return rc;
     }
 
-  put_membuf (&data, "", 1); /* Make sure it is 0 terminated. */
+  put_membuf (&data, "", 1); /* Make sure it is 0 terminated so we can invoke strtoul safely. */
   buf = get_membuf (&data, &len);
   if (!buf)
     return gpg_error (GPG_ERR_ENOMEM);
@@ -538,8 +538,14 @@ gpgsm_agent_pkdecrypt (ctrl_t ctrl, const char *keygrip, const char *desc,
     {
       if (len < 13 || memcmp (buf, "(5:value", 8) ) /* "(5:valueN:D)\0" */
         return gpg_error (GPG_ERR_INV_SEXP);
-      len -= 11;   /* Count only the data of the second part. */
+      /* Trim any spurious trailing Nuls: */
+      while (buf[len-1] == 0)
+        len--;
+      if (buf[len-1] != ')')
+        return gpg_error (GPG_ERR_INV_SEXP);
+      len--; /* Drop the final close-paren: */
       p = buf + 8; /* Skip leading parenthesis and the value tag. */
+      len -= 8; /* Count only the data of the second part. */
     }
   else
     {