Handle ASN.1 type detection errors #3855
Conversation
lrstewart
force-pushed
the
fix
branch
3 times, most recently
from
7ed39da to
a75fa27
Compare
| @@ -129,14 +129,29 @@ int s2n_pkey_free(struct s2n_pkey *key) | |||
| return S2N_SUCCESS; | |||
| } | |||
|
|
|||
| int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der) | |||
| int s2n_asn1der_to_private_key(struct s2n_pkey *priv_key, struct s2n_blob *asn1der, int type_hint) | |||
There was a problem hiding this comment.
bummer that they didn't use an enum here...
There was a problem hiding this comment.
Yeah. I considered using an internal one, and that would have avoided the CBMC issues. But we'd just have to immediately convert back to the int to use the enum.
| @@ -0,0 +1,13 @@ | |||
| -----BEGIN CERTIFICATE----- | |||
There was a problem hiding this comment.
Not blocking but it would be nice to have these cert generators scripted and reproducible. I started one in pems/sni but it would be good to add one to the root.
There was a problem hiding this comment.
That'd be pretty difficult for this one. I had to manually modify it to remove the last entry. If we scripted it, the script would need to parse the base64, parse the ASN.1, remove the last field of the ASN.1, update the overall length, and then re-encode in base64.
There was a problem hiding this comment.
hopefully anyone in the future will find this comment if they need to reconstruct it, I guess 😄
tests/cbmc/proofs/s2n_stuffer_private_key_from_pem/s2n_stuffer_private_key_from_pem_harness.c
Outdated
Show resolved
Hide resolved
Description of changes:
s2n-tls is rejecting a customer's private ECDSA key because it doesn't set the publicKey field of the ASN.1 structure, which is marked as OPTIONAL in both RFC5915 and SECG's SEC 1. This missing field causes d2i_AutoPrivateKey to incorrectly identify the type of the private key.
libcrypto also offers PEM_read_bio_PrivateKey, which is able to parse the customer's key. However, s2n-tls prefers its own strict PEM parsing.
So I see two options:
I initially went with the second option, and it worked. But it seemed like overkill, and I worried it might be more permissive than our current logic. So I switched to the first option, which also worked and seemed smaller / cleaner.
Call-outs:
Testing:
I generated an EC key pair and then ripped the publicKey field out of the private key. The pem test fails without this fix, but passes with it.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.