Diffusion GnuPG fdd1567743cc

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

Authored by dkg on Jul 23 2019, 4:07 PM.

Description

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: T4652
  • Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>

Details

Committed
dkgJul 25 2019, 1:30 AM
Parents
rG7bfbb9fa7e76: gpg: A little clean up.
Branches
Unknown
Tags
Unknown
Tasks
T4652: avoid unnecessary trailing NUL byte in S-expressions
gniibe added a subscriber: gniibe.EditedJul 25 2019, 6:21 AM

As far as I know, usually, gpg/gpgsm can assume same version of gpg-agent.

Historically, old libgcrypt API returns binary data only (not S-expression).

For such a major change of format, it is robust for client (gpg/gpgsm) to support old behavior of gpg-agent.
I think that that's a reason why there is code for backward compatiblity.

/sm/call-agent.c
562

I think that by the removal of trailing NUL, this condition can be exact, like:

if (endp-p+n != len)

I'd like to push your change to master, if possible with exact check.
Do you intend to put your comment to the master repo? Or, it's for discussion?
It's OK for your topic branch, but, I feel that it would be too long to be included to master repo.

dkg added a comment.Jul 25 2019, 2:40 PM

I don't think there's a problem to have a long explanatory message in the main repository, as i think it makes it easier to understand, and space is not an issue.

But if you prefer to not have less of an explanation in the revision control system, i'm fine with whatever edits you prefer to make.

dkg added a comment.EditedJul 25 2019, 2:45 PM

fwiw, if the old gcrypt actually returned a radically different API, it should have a larger SONAME across that change, and NEED_LIBGCRYPT_VERSION should reflect a source version that forces it past that SONAME. I don't know what version of libgcrypt behaved differently -- is there a reference for that?

Currently, the GnuPG sources on STABLE-BRANCH-2-2 has NEED_LIBGCRYPT_VERSION=1.7.0. Does this mean we can clean out the extra codepath in sm/call-agent.c?

And if we can't clean it out, does g10/call-agent.c need to support the same flexibility?

/sm/call-agent.c
562

I think you're right, but i haven't tested it.

As far as I know, usually, gpg/gpgsm can assume same version of gpg-agent.

Due to socket forwarding we can have different versions of gpg-agent and gpg / gpgsm because they are on different machines and afaik we try to support it.

dkg added a comment.Jul 25 2019, 2:54 PM

@aheinecke

Due to socket forwarding we can have different versions of gpg-agent and gpg / gpgsm because they are on different machines and afaik we try to support it.

In that case, we definitely want this fix for robustness, because a reasonable gpg-agent replacement might return a raw S-expression without any spurious NUL-terminator.

However, I don't know how we will manage to apply rGefffd9907b7501323bae89ae515bc26312aaab15 safely, if there's no way to ensure that the agent is talking to a robust client, instead of one that is likely to choke if the spurious NUL-terminator is absent. :(

I'm going to push this change to master.

@dkg : We don't discuss by the git commits in master repo. It's not the right place/tool to record discussion. I think that we should use dev.gnupg.org or mailing list for that. On the other hand, using a topic branch with comment-ful commit log is useful.

Let me explain.
Comments in git log won't go into ChangeLog of the releases. It's only for developers with git. And, (in GNU coding standard,) when we need an explanation for code, it's not in ChangeLog (or git log), but it should be into the code itself as a comment, so that an explanation works effectively.

Please note that:
We use our own version of gitlog-to-changelog to generate ChangeLog (for releases), and it is our own feature to remove comments from git log when generating ChangeLog. The intention here is, it allows us to put some reference of change, etc., for developers with git, but not for releases.