From d2242407bb382f403677f30f697eed36e9d7ee6f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 26 Dec 2016 00:18:49 -0500 Subject: [PATCH] Don't accept signature OIDs in EVP_get_digestby{nid,obj}. This is a remnant of signature EVP_MDs. Detach them from EVP_get_digestby{nid,obj}. Nothing appears to rely on this for those two functions. Alas, Node.js appears to rely on it for EVP_get_digestbyname, so keep that working. This avoids causing every consumer's parsing to be unintentionally lax. It also means fewer OIDs to transcribe when detaching the last of libcrypto from the legacy ASN.1 stack and its giant OID table. Note this is an externally visible change. There was one consumer I had to fix, but otherwise everything handled things incorrectly due to this quirk, so it seemed better to just fix the API rather than fork off a second set. Change-Id: I705e073bc05d946e71cd1c38acfa5e3c6b0a22b4 Reviewed-on: https://boringssl-review.googlesource.com/13058 Commit-Queue: David Benjamin Reviewed-by: Adam Langley --- crypto/digest/digest_test.cc | 15 +++++++-- crypto/digest/digests.c | 63 +++++++++++++++++++----------------- 2 files changed, 45 insertions(+), 33 deletions(-) diff --git a/crypto/digest/digest_test.cc b/crypto/digest/digest_test.cc index 0d3f16e5..8b29236e 100644 --- a/crypto/digest/digest_test.cc +++ b/crypto/digest/digest_test.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include "../internal.h" @@ -235,9 +236,17 @@ static int TestDigest(const TestVector *test) { } static int TestGetters() { - if (EVP_get_digestbyname("RSA-SHA512") == NULL || - EVP_get_digestbyname("sha512WithRSAEncryption") == NULL || - EVP_get_digestbyname("nonsense") != NULL) { + if (EVP_get_digestbyname("RSA-SHA512") != EVP_sha512() || + EVP_get_digestbyname("sha512WithRSAEncryption") != EVP_sha512() || + EVP_get_digestbyname("nonsense") != NULL || + EVP_get_digestbyname("SHA512") != EVP_sha512() || + EVP_get_digestbyname("sha512") != EVP_sha512()) { + return false; + } + + if (EVP_get_digestbynid(NID_sha512) != EVP_sha512() || + EVP_get_digestbynid(NID_sha512WithRSAEncryption) != NULL || + EVP_get_digestbynid(NID_undef) != NULL) { return false; } diff --git a/crypto/digest/digests.c b/crypto/digest/digests.c index 3307f265..351e031f 100644 --- a/crypto/digest/digests.c +++ b/crypto/digest/digests.c @@ -65,6 +65,7 @@ #include #include "internal.h" +#include "../internal.h" #if defined(NDEBUG) #define CHECK(x) (void) (x) @@ -262,36 +263,41 @@ struct nid_to_digest { }; static const struct nid_to_digest nid_to_digest_mapping[] = { - { NID_md4, EVP_md4, SN_md4, LN_md4 }, - { NID_md5, EVP_md5, SN_md5, LN_md5 }, - { NID_sha1, EVP_sha1, SN_sha1, LN_sha1 }, - { NID_sha224, EVP_sha224, SN_sha224, LN_sha224 }, - { NID_sha256, EVP_sha256, SN_sha256, LN_sha256 }, - { NID_sha384, EVP_sha384, SN_sha384, LN_sha384 }, - { NID_sha512, EVP_sha512, SN_sha512, LN_sha512 }, - { NID_md5_sha1, EVP_md5_sha1, SN_md5_sha1, LN_md5_sha1 }, - { NID_dsaWithSHA, EVP_sha1, SN_dsaWithSHA, LN_dsaWithSHA }, - { NID_dsaWithSHA1, EVP_sha1, SN_dsaWithSHA1, LN_dsaWithSHA1 }, - { NID_ecdsa_with_SHA1, EVP_sha1, SN_ecdsa_with_SHA1, NULL }, - { NID_md5WithRSAEncryption, EVP_md5, SN_md5WithRSAEncryption, - LN_md5WithRSAEncryption }, - { NID_sha1WithRSAEncryption, EVP_sha1, SN_sha1WithRSAEncryption, - LN_sha1WithRSAEncryption }, - { NID_sha224WithRSAEncryption, EVP_sha224, SN_sha224WithRSAEncryption, - LN_sha224WithRSAEncryption }, - { NID_sha256WithRSAEncryption, EVP_sha256, SN_sha256WithRSAEncryption, - LN_sha256WithRSAEncryption }, - { NID_sha384WithRSAEncryption, EVP_sha384, SN_sha384WithRSAEncryption, - LN_sha384WithRSAEncryption }, - { NID_sha512WithRSAEncryption, EVP_sha512, SN_sha512WithRSAEncryption, - LN_sha512WithRSAEncryption }, + {NID_md4, EVP_md4, SN_md4, LN_md4}, + {NID_md5, EVP_md5, SN_md5, LN_md5}, + {NID_sha1, EVP_sha1, SN_sha1, LN_sha1}, + {NID_sha224, EVP_sha224, SN_sha224, LN_sha224}, + {NID_sha256, EVP_sha256, SN_sha256, LN_sha256}, + {NID_sha384, EVP_sha384, SN_sha384, LN_sha384}, + {NID_sha512, EVP_sha512, SN_sha512, LN_sha512}, + {NID_md5_sha1, EVP_md5_sha1, SN_md5_sha1, LN_md5_sha1}, + /* As a remnant of signing |EVP_MD|s, OpenSSL returned the corresponding + * hash function when given a signature OID. To avoid unintended lax parsing + * of hash OIDs, this is no longer supported for lookup by OID or NID. + * Node.js, however, exposes |EVP_get_digestbyname|'s full behavior to + * consumers so we retain it there. */ + {NID_undef, EVP_sha1, SN_dsaWithSHA, LN_dsaWithSHA}, + {NID_undef, EVP_sha1, SN_dsaWithSHA1, LN_dsaWithSHA1}, + {NID_undef, EVP_sha1, SN_ecdsa_with_SHA1, NULL}, + {NID_undef, EVP_md5, SN_md5WithRSAEncryption, LN_md5WithRSAEncryption}, + {NID_undef, EVP_sha1, SN_sha1WithRSAEncryption, LN_sha1WithRSAEncryption}, + {NID_undef, EVP_sha224, SN_sha224WithRSAEncryption, + LN_sha224WithRSAEncryption}, + {NID_undef, EVP_sha256, SN_sha256WithRSAEncryption, + LN_sha256WithRSAEncryption}, + {NID_undef, EVP_sha384, SN_sha384WithRSAEncryption, + LN_sha384WithRSAEncryption}, + {NID_undef, EVP_sha512, SN_sha512WithRSAEncryption, + LN_sha512WithRSAEncryption}, }; const EVP_MD* EVP_get_digestbynid(int nid) { - unsigned i; + if (nid == NID_undef) { + /* Skip the |NID_undef| entries in |nid_to_digest_mapping|. */ + return NULL; + } - for (i = 0; i < sizeof(nid_to_digest_mapping) / sizeof(struct nid_to_digest); - i++) { + for (unsigned i = 0; i < OPENSSL_ARRAY_SIZE(nid_to_digest_mapping); i++) { if (nid_to_digest_mapping[i].nid == nid) { return nid_to_digest_mapping[i].md_func(); } @@ -305,10 +311,7 @@ const EVP_MD* EVP_get_digestbyobj(const ASN1_OBJECT *obj) { } const EVP_MD *EVP_get_digestbyname(const char *name) { - unsigned i; - - for (i = 0; i < sizeof(nid_to_digest_mapping) / sizeof(struct nid_to_digest); - i++) { + for (unsigned i = 0; i < OPENSSL_ARRAY_SIZE(nid_to_digest_mapping); i++) { const char *short_name = nid_to_digest_mapping[i].short_name; const char *long_name = nid_to_digest_mapping[i].long_name; if ((short_name && strcmp(short_name, name) == 0) ||