reject non-zero unused bits in public key bit strings#15064
Conversation
|
Is there some spec that dictates this behavior? Do other libraries enforce this? |
|
there isn't a single clause that says "reject a non-zero count" outright. it falls out of the per-algorithm content definitions: for the key types we load the subjectPublicKey always holds a whole number of octets, so the bit length is a multiple of eight and the unused-bits count can only be zero. for the edwards/montgomery keys rfc 8410 carries the raw key bytes, ec carries the SEC1 point octet string (rfc 5480), and the rsa case wraps a der-encoded RSAPublicKey. a non-zero count contradicts that, and x.690 11.2 also requires the trailing unused bits themselves be zero under der. honestly the strongest argument is internal consistency rather than a survey of other libraries. parse_spki_for_data already rejects exactly this for the same field, so right now load_der_public_key and that helper disagree on whether the same bytes are a valid public key, and the ec private key loader reads the point bit string the same loose way. i haven't done a broad survey across other libraries, so if you'd rather not tighten this at parse time i'm happy to drop it. |
alex
left a comment
alex
left a comment
There was a problem hiding this comment.
Confirmed that BoringSSL does these checks, so we should be good once the tests are fixed up. Sorry for the slow review.
| idx = data.rfind(b"\x03\x21\x00") | ||
| assert idx != -1 | ||
| data[idx + 2] = 0x01 |
There was a problem hiding this comment.
The right way to do these is to add new vectors, not this manipulation approach.
There was a problem hiding this comment.
switched both to committed vectors. added asymmetric/Ed25519/ed25519-pub-non-zero-unused-bits.der and asymmetric/EC/ec_private_key_non_zero_unused_bits.der, each a valid key with the unused-bits byte bumped to 1, and the tests just load and expect ValueError now. dropped the generate-and-mangle loops.
| data = bytearray(der) | ||
| data[idx + 2] = 0x01 |
There was a problem hiding this comment.
done, this one now loads the EC vector too.
| @@ -0,0 +1,4 @@ | |||
| 0w �2ST��� | |||
There was a problem hiding this comment.
please document in test-vectors.rst
There was a problem hiding this comment.
done, documented both vectors in test-vectors.rst alongside the EC and Ed25519 entries.
alex
left a comment
alex
left a comment
There was a problem hiding this comment.
Confirmed that BoringSSL does these checks.
|
Thank you. |
2 participants
load_der_public_key and the EC private key loader read the subjectPublicKey and EC publicKey BIT STRINGs with as_bytes() but never look at the unused-bits count, so DER that declares a non-zero number of unused bits in those octet-aligned key bit strings is accepted even though it is malformed. parse_spki_for_data already rejects exactly this for the same field, so the two paths disagree on what a valid public key encoding is. This adds the same padding_bits check to both loaders, with regression tests that craft an Ed25519 SPKI and an EC private key carrying a bogus unused-bits count.