avoid unnecessary trailing NUL byte in S-expressions
Open, LowPublic

Description

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).

dkg created this task.Jul 23 2019, 5:37 PM
dkg added a comment.EditedJul 23 2019, 5:40 PM

I've just pushed rG1ae16838660a to the dkg-fix-T4652 branch (i just adjusted it the commit message to include the GnuPG-bug-id)

dkg added a comment.Tue, Jul 23, 6:46 PM

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.scm

That 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.

dkg added a comment.Wed, Jul 24, 6:24 AM

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.

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.

werner added a subscriber: werner.Wed, Jul 31, 8:49 AM

Appending a nul byte is fail-safe programming and helps in debugging. It is on purpose and shall not be removed.

werner triaged this task as Low priority.Wed, Jul 31, 12:34 PM
dkg added a comment.Wed, Jul 31, 4:45 PM

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.