BoringSSL: Don't support parameterless DSA keys in SPKIs AND Set an EVP_PKEY's algorithm and data together#3057
BoringSSL: Don't support parameterless DSA keys in SPKIs AND Set an EVP_PKEY's algorithm and data together#3057
Conversation
| @@ -236,7 +236,11 @@ int EVP_read_pw_string_min(char *buf, int min_length, int length, | |||
| int EVP_PKEY_copy_parameters(EVP_PKEY *to, const EVP_PKEY *from) { | |||
| SET_DIT_AUTO_RESET; | |||
There was a problem hiding this comment.
warning: use of undeclared identifier 'SET_DIT_AUTO_RESET' [clang-diagnostic-error]
SET_DIT_AUTO_RESET;
^| @@ -441,11 +447,13 @@ int EVP_PKEY_set1_RSA(EVP_PKEY *pkey, RSA *key) { | |||
|
|
|||
| int EVP_PKEY_assign_RSA(EVP_PKEY *pkey, RSA *key) { | |||
| SET_DIT_AUTO_RESET; | |||
There was a problem hiding this comment.
warning: use of undeclared identifier 'SET_DIT_AUTO_RESET' [clang-diagnostic-error]
SET_DIT_AUTO_RESET;
^| @@ -477,11 +485,13 @@ int EVP_PKEY_set1_DSA(EVP_PKEY *pkey, DSA *key) { | |||
|
|
|||
| int EVP_PKEY_assign_DSA(EVP_PKEY *pkey, DSA *key) { | |||
| SET_DIT_AUTO_RESET; | |||
There was a problem hiding this comment.
warning: use of undeclared identifier 'SET_DIT_AUTO_RESET' [clang-diagnostic-error]
SET_DIT_AUTO_RESET;
^| key->has_private = 1; | ||
| OPENSSL_free(pkey->pkey.ptr); | ||
| pkey->pkey.ptr = key; | ||
| evp_pkey_set0(pkey, &ed25519_asn1_meth, key); |
There was a problem hiding this comment.
warning: call to undeclared function 'evp_pkey_set0'; ISO C99 and later do not support implicit function declarations [clang-diagnostic-implicit-function-declaration]
evp_pkey_set0(pkey, &ed25519_asn1_meth, key);
^| key->has_private = 1; | ||
| OPENSSL_free(pkey->pkey.ptr); | ||
| pkey->pkey.ptr = key; | ||
| evp_pkey_set0(pkey, &ed25519_asn1_meth, key); |
There was a problem hiding this comment.
warning: use of undeclared identifier 'ed25519_asn1_meth' [clang-diagnostic-error]
evp_pkey_set0(pkey, &ed25519_asn1_meth, key);
^
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3057 +/- ##
=======================================
Coverage 78.00% 78.01%
=======================================
Files 689 689
Lines 122405 122395 -10
Branches 17083 17082 -1
=======================================
+ Hits 95482 95486 +4
+ Misses 26026 26012 -14
Partials 897 897 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d8c6c0e to
b09ec22
Compare
| OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR); | ||
| goto err; | ||
| } | ||
| // See RFC 3279, section 2.3.2. Although RFC 3279 allows DSA parameters to be |
There was a problem hiding this comment.
OK, but note it might cause test to fail in some packages if they include such keys. Maybe this could flush them out.
| key->has_private = 1; | ||
| OPENSSL_free(pkey->pkey.ptr); | ||
| pkey->pkey.ptr = key; | ||
| evp_pkey_set0(pkey, &ed25519_asn1_meth, key); |
There was a problem hiding this comment.
Need to include the proper header files declaring both functions.
crypto/evp_extra/p_dh_asn1.c
Outdated
| evp_pkey_set_method(pkey, &dh_asn1_meth); | ||
| pkey->pkey.dh = key; | ||
| return key != NULL; | ||
| if (key == NULL) { |
There was a problem hiding this comment.
These could use GUARD_PTR
a9e2811 to
7322bf2
Compare
Cherry-picked from BoringSSL 5b7171f2da10d5dad52d0a2e4426fbc71d4ff146.
EVP_PKEY_{assign,set1}_FOO would check for NULL, return an error, but
still leave the EVP_PKEY assigned to that type on failure. Check for
NULL first, so that we don't leave it in that state.
Adapted for AWS-LC:
- RSA/DSA/EC_KEY assign functions are in crypto/fipsmodule/evp/evp.c
(not split out into separate p_*_asn1.cc files as in BoringSSL)
- DH assign function is in crypto/evp_extra/p_dh_asn1.c
- Added NoHalfEmptyKeys test (AWS-LC did not have the BoringSSL
equivalent)
Cherry-picked from BoringSSL 1bc58a303ef577f4dc5c5ed0612e9766c85dce8c. RFC 3279 allows DSA parameters to be omitted, in which case they are picked up from the issuer in an X.509 chain (which we do not support), or implicitly from some unspecified out-of-band source. Parameterless DSA keys in SubjectPublicKeyInfo will no longer parse. This does not impact TLS, where we have never supported DSA.
Cherry-picked from BoringSSL b0ef87e5e3ba9a4f4b5bba4434f6f2e630507b48. Replace evp_pkey_set_method with evp_pkey_set0, which sets both the method and key data together, avoiding half-initialized EVP_PKEY states. Adapted for AWS-LC: - RSA/DSA/EC_KEY/DH assign functions and EVP_PKEY_new_raw_* are in different files than BoringSSL (FIPS module vs evp_extra) - AWS-LC has a pkey.type field that must also be set (BoringSSL derives type from ameth) - ed25519/x25519 set_priv_raw/set_pub_raw callbacks are shared with ed25519ph (AWS-LC specific), so callers (EVP_PKEY_new_raw_*, evp_asn1 decode) set the method via evp_pkey_set0 before invoking the callback, rather than having the callback set it - KEM and PQDSA (AWS-LC specific) updated to create key first, then call evp_pkey_set0 with method+data together
7322bf2 to
c33ff43
Compare
Note: to be rebased on #3056.
Description of changes:
BoringSSL commit google/boringssl@1bc58a3: "Don't support parameterless DSA keys in SPKIs"
Changes (2 files):
CBS_len(params) == 0branch indsa_pub_decodethat created an emptyDSA_new()without parameters. Parameterless DSA keys inSubjectPublicKeyInfowill no longer parse. This does not impact TLS, where we have never supported DSA.DECODE_ERRORinstead of successful parsing.BoringSSL commit google/boringssl@b0ef87e: "Set an EVP_PKEY's algorithm and data together"
Changes (8 files):
evp_pkey_set_methodwithevp_pkey_set0(pkey, method, pkey_data)which setsmethod,type, anddataatomically. Removedfree_itfrompkey_set_type(usesevp_pkey_set0(pkey, NULL, NULL)instead).EVP_PKEY_new_raw_*functions setmethodviaevp_pkey_set0before calling callbacks.evp_pkey_set0implementation: BoringSSL doesn't have thetypefield — it derives thetypefromamethat query time viaEVP_PKEY_id(). AWS-LC stores type as a separate fieldthat must be kept in sync.
pub_decode/priv_decodeNULL checks earlier; set method viaevp_pkey_set0before calling decode callbacksevp_pkey_set0in keygenevp_pkey_set0(pkey, method, key)togetherAdaptations for AWS-LC:
ed25519/x25519 set_priv_raw/set_pub_raw callbacks are shared with ed25519ph (AWS-LC specific), so the method must be set by the caller before invoking the callback, not by the
callback itself.
Details
crypto/fipsmodule/evp/evp.c:
EVP_PKEY_new_raw_private_key— callsevp_pkey_set0(ret, method, NULL)thenmethod->set_priv_raw(...)EVP_PKEY_new_raw_public_key— callsevp_pkey_set0(ret, method, NULL)thenmethod->set_pub_raw(...)crypto/evp_extra/evp_asn1.c:
EVP_parse_public_key— callsevp_pkey_set0(ret, method, NULL)thenmethod->pub_decode(...)EVP_parse_private_key— callsevp_pkey_set0(ret, method, NULL)thenmethod->priv_decode(...)In BoringSSL's version, all four of these just called
method->callback(ret, ...)directly (nomethodpre-setting), and the callbacks themselves called evp_pkey_set0. We couldn't do that because the ed25519 callbacks are shared with ed25519ph.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.