Optionally pad OpenPGP EdDSA signature parameters #340
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add special crafted test signature, using GnuPG/MacGPG2 v2.2.24, where the DATA and TIMESTAMP to be hashed and signed are chosen such that the S value of the resulting signature starts with a 0-byte and is thus omitted as per the RFC 4880 MPI encoding, making the overall signature one byte shorter than required as per RFC 8032. Kudos to @jku for providing the following reproducer: ``` DATA="hello" TIMESTAMP=1613479847 HOMEDIR="tests/gpg_keyrings/eddsa/" echo -n $DATA | gpg --faked-system-time $TIMESTAMP \ --homedir $HOMEDIR \ --digest-algo SHA256 \ --local-user 4E630F84838BF6F7447B830B22692F5FEA9E2DD2 \ --detach-sign > short.sig gpg --list-packets --verbose short.sig echo -n $DATA | gpg --homedir $HOMEDIR --verify short.sig - ```
EdDSA signatures must be 64 bytes long as per RFC 8032 5.1.6. (6). This commit adds a constant for this length which may be used to to pad shorter signatures.
Closed
jku
approved these changes
Feb 25, 2021
securesystemslib/gpg/eddsa.py
Outdated
Comment on lines
148
to
149
| r = r.rjust(int(ED25519_SIG_LENGTH / 2), b"\x00") | ||
| s = s.rjust(int(ED25519_SIG_LENGTH / 2), b"\x00") |
There was a problem hiding this comment.
not a big deal but I think this should work in all pythons:
Suggested change
| r = r.rjust(int(ED25519_SIG_LENGTH / 2), b"\x00") | |
| s = s.rjust(int(ED25519_SIG_LENGTH / 2), b"\x00") | |
| r = r.rjust(ED25519_SIG_LENGTH // 2, b"\x00") | |
| s = s.rjust(ED25519_SIG_LENGTH // 2, b"\x00") |
There was a problem hiding this comment.
Nice use of the floor division operator. Will update.
|
|
||
| # Check that the signature is padded upon parsing | ||
| signature = parse_signature_packet(signature_data) | ||
| self.assertTrue(len(signature["signature"]) == (ED25519_SIG_LENGTH * 2)) |
There was a problem hiding this comment.
So this is double length here because signature is already a hex string at this point?
There was a problem hiding this comment.
Exactly. I'll add a comment.
Left-zero-pad 'r' and 's' values that are shorter than required by RFC 8032 (5.1.6.), to make up for omitted leading zeros in RFC 4880 (3.2.) MPIs. This is especially important for 's', which is little-endian. Note that GnuPG seems to correctly parse EdDSA signatures with MPIs that omit leading zeros prior to validation. But in order to validate these signatures outside the context of RFC 4880, e.g. with pyca/cryptography's OpenSSL backend, we need to guarantee the correct length of the parsed signatures ourselves.
Test correct padding of OpenPGP EdDSA signature upon parsing, using a special-crafted short signature pre-generated with GnuPG.
lukpueh
force-pushed
the
pad-pgp-eddsa-sig
branch
from
3f83988
to
b55fa42
Compare
|
Addressed your comments and force-pushed. Thanks for the review, @jku! |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #334
Description of the changes being introduced by the pull request:
Left-zero-pad 'r' and 's' values that are shorter than required by RFC 8032 (5.1.6.), to make up for omitted leading zeros in RFC 4880 (3.2.) MPIs. This is especially important for 's', which is little-endian.
Note that GnuPG seems to correctly parse EdDSA signatures with MPIs that omit leading zeros prior to validation. But in order to validate these signatures outside the context of RFC 4880, e.g. with pyca/cryptography's OpenSSL backend, we need to guarantee the correct length of the parsed signatures ourselves.
This PR also adds a test using a special-crafted short signature.
See commit messages and #334 for more details.
Kudos to @jku and @SantiagoTorres for helping to troubleshoot.
Please verify and check that the pull request fulfils the following requirements: