From fdd1567743cc1d526f92e63162d899a24c42cdf4 Mon Sep 17 00:00:00 2001 From: Daniel Kahn Gillmor Date: Tue, 23 Jul 2019 10:07:17 -0400 Subject: [PATCH] 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 --- g10/call-agent.c | 13 ++++++++----- sm/call-agent.c | 12 +++++++++--- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/g10/call-agent.c b/g10/call-agent.c index d55238311..62568fc76 100644 --- a/g10/call-agent.c +++ b/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 != ':') diff --git a/sm/call-agent.c b/sm/call-agent.c index b37c2e53d..f9069a3b1 100644 --- a/sm/call-agent.c +++ b/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 {