agent_pkdecrypt injects a trailing NUL byte in a couple places, which makes the memory buffer different from what the canonical S-expression would be. It is also dangerous to encourage other parts of the code to treat an S-expression as a C string, since it might contain NUL bytes mid-stream (e.g., when a high-entropy bytestring is present in the S-expression).
Description
Description
Revisions and Commits
Revisions and Commits
Related Objects
Related Objects
Event Timeline
Comment Actions
I've just pushed rG1ae16838660a to the dkg-fix-T4652 branch (i just adjusted it the commit message to include the GnuPG-bug-id)
Comment Actions
fwiw, this patch appears to cause gpgsm to fail its test suite:
Checking encryption
> plain-1 .gpgsm: NOTE: THIS IS A DEVELOPMENT VERSION!
gpgsm: It is only intended for test purposes and should NOT be
gpgsm: used in a production environment or with production keys!
gpgsm: WARNING: running with faked system time: 2001-12-13 11:00:00
gpgsm: DBG: recp 0 - issuer: 'CN=test cert 1,OU=Aegypten Project,O=g10 Code GmbH,L=Düsseldorf,C=DE'
gpgsm: DBG: recp 0 - serial: 00
gpgsm: error decrypting session key: Invalid S-expression
gpgsm: decrypting session key failed: Invalid S-expression
gpgsm: message decryption failed: Invalid S-expression <GpgSM>
: (() ((throw (:stderr result)) (call-popen cmd input)))
0: #<CLOSURE>
1: tests.scm:443: (apply throw error)
FAIL: tests/gpgsm/encrypt.scm
Checking bogus signature.
> #x2d #xca <
Checking that a valid signature is verified as such.
Checking that an invalid signature is verified as such.
PASS: tests/gpgsm/verify.scm
Checking decryption of supplied files.
> plain-1 gpgsm: NOTE: THIS IS A DEVELOPMENT VERSION!
gpgsm: It is only intended for test purposes and should NOT be
gpgsm: used in a production environment or with production keys!
gpgsm: WARNING: running with faked system time: 2001-12-13 11:00:00
gpgsm: DBG: recp 0 - issuer: 'CN=test cert 1,OU=Aegypten Project,O=g10 Code GmbH,L=Düsseldorf,C=DE'
gpgsm: DBG: recp 0 - serial: 00
gpgsm: error decrypting session key: Invalid S-expression
gpgsm: decrypting session key failed: Invalid S-expression
gpgsm: message decryption failed: Invalid S-expression <GpgSM>
: (() ((throw (:stderr result)) (call-popen cmd input)))
0: #<CLOSURE>
1: tests.scm:443: (apply throw error)
FAIL: tests/gpgsm/decrypt.scmThat suggests that gpsm might itself might fail if it decrypts (or encrypts) something with an embedded NUL bytes, but i haven't tracked it down in more detail yet.
Comment Actions
I've just posted rGb84feb0c82eb to the dkg-fix-T4652 branch, which solves the failure problems by making agent_pkdecrypt and gpgsm_agent_pkdecrypt more robust.
Comment Actions
I've just broken out my changes into two commits, one that makes gpg and gpgsm more robust. That should be applicable without any risk.
Then there is a subsequent commit which changes gpg-agent to drop its trailing NUL octet from the S-expression returned by pkdecrypt. We don't have a great way to do versioned/dependent upgrades, but it's possible that you want to delay making the agent more conservative until after you're convinced that the client programs are all fixed.
Comment Actions
Appending a nul byte is fail-safe programming and helps in debugging. It is on purpose and shall not be removed.
Comment Actions
Please see my explanation on gnupg-devel about why the trailing NUL is a source of pain and difficulty for would-be adopters.
If there are any plans for gpg-agent's PKDECRYPT and PKSIGN operations to be used by any project other than GnuPG, it would be good to document explicitly that they produce S-expressions with an additional trailing NUL byte which should be discarded before being fed to any standards-compliant S-expression parser.