From 03cae7a2b36285ddd6e7692964bb1b836d1d2572 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 24 Sep 2021 12:25:41 -0400 Subject: [PATCH 1/3] Keep EVP_CIPHER/EVP_MD lookup and do_all functions in sync Node seems uncommonly sensitive to this, so let's write these functions in a way that stays in sync and test this. See also https://boringssl-review.googlesource.com/c/boringssl/+/49585 This does incur a cost across all BoringSSL consumers that use these functions: as a result of Node indiscriminately exposing every cipher, we end up pulling more and more ciphers into these getters. But that ship sailed long ago, so, instead, document that EVP_get_cipherby* should not be used by size-conscious callers. EVP_get_digestby* probably should have the same warning, but I've left it alone for now because we don't quite have the same proliferation of digests as ciphers. (Though there are things in there, like MD4, that ought to be better disconnected.) Change-Id: I61ca406c146279bd05a52bed6c57200d1619c5da Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49625 Reviewed-by: Adam Langley Commit-Queue: Adam Langley --- crypto/cipher_extra/cipher_extra.c | 110 ++++++++++++----------------- crypto/cipher_extra/cipher_test.cc | 19 +++++ decrepit/CMakeLists.txt | 1 + decrepit/evp/evp_test.cc | 45 ++++++++++++ include/openssl/cipher.h | 10 ++- 5 files changed, 118 insertions(+), 67 deletions(-) create mode 100644 decrepit/evp/evp_test.cc diff --git a/crypto/cipher_extra/cipher_extra.c b/crypto/cipher_extra/cipher_extra.c index 786a5d5fb..62850ab6a 100644 --- a/crypto/cipher_extra/cipher_extra.c +++ b/crypto/cipher_extra/cipher_extra.c @@ -67,25 +67,42 @@ #include "../internal.h" +static const struct { + int nid; + const char *name; + const EVP_CIPHER *(*func)(void); +} kCiphers[] = { + {NID_aes_128_cbc, "aes-128-cbc", EVP_aes_128_cbc}, + {NID_aes_128_ctr, "aes-128-ctr", EVP_aes_128_ctr}, + {NID_aes_128_ecb, "aes-128-ecb", EVP_aes_128_ecb}, + {NID_aes_128_gcm, "aes-128-gcm", EVP_aes_128_gcm}, + {NID_aes_128_ofb128, "aes-128-ofb", EVP_aes_128_ofb}, + {NID_aes_192_cbc, "aes-192-cbc", EVP_aes_192_cbc}, + {NID_aes_192_ctr, "aes-192-ctr", EVP_aes_192_ctr}, + {NID_aes_192_ecb, "aes-192-ecb", EVP_aes_192_ecb}, + {NID_aes_192_gcm, "aes-192-gcm", EVP_aes_192_gcm}, + {NID_aes_192_ofb128, "aes-192-ofb", EVP_aes_192_ofb}, + {NID_aes_256_cbc, "aes-256-cbc", EVP_aes_256_cbc}, + {NID_aes_256_ctr, "aes-256-ctr", EVP_aes_256_ctr}, + {NID_aes_256_ecb, "aes-256-ecb", EVP_aes_256_ecb}, + {NID_aes_256_gcm, "aes-256-gcm", EVP_aes_256_gcm}, + {NID_aes_256_ofb128, "aes-256-ofb", EVP_aes_256_ofb}, + {NID_des_cbc, "des-cbc", EVP_des_cbc}, + {NID_des_ecb, "des-ecb", EVP_des_ecb}, + {NID_des_ede_cbc, "des-ede-cbc", EVP_des_ede_cbc}, + {NID_des_ede_ecb, "des-ede", EVP_des_ede}, + {NID_des_ede3_cbc, "des-ede3-cbc", EVP_des_ede3_cbc}, + {NID_rc2_cbc, "rc2-cbc", EVP_rc2_cbc}, + {NID_rc4, "rc4", EVP_rc4}, +}; + const EVP_CIPHER *EVP_get_cipherbynid(int nid) { - switch (nid) { - case NID_rc2_cbc: - return EVP_rc2_cbc(); - case NID_rc2_40_cbc: - return EVP_rc2_40_cbc(); - case NID_des_ede3_cbc: - return EVP_des_ede3_cbc(); - case NID_des_ede_cbc: - return EVP_des_cbc(); - case NID_aes_128_cbc: - return EVP_aes_128_cbc(); - case NID_aes_192_cbc: - return EVP_aes_192_cbc(); - case NID_aes_256_cbc: - return EVP_aes_256_cbc(); - default: - return NULL; + for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kCiphers); i++) { + if (kCiphers[i].nid == nid) { + return kCiphers[i].func(); + } } + return NULL; } const EVP_CIPHER *EVP_get_cipherbyname(const char *name) { @@ -93,54 +110,17 @@ const EVP_CIPHER *EVP_get_cipherbyname(const char *name) { return NULL; } - if (OPENSSL_strcasecmp(name, "rc4") == 0) { - return EVP_rc4(); - } else if (OPENSSL_strcasecmp(name, "des-cbc") == 0) { - return EVP_des_cbc(); - } else if (OPENSSL_strcasecmp(name, "des-ede3-cbc") == 0 || - // This is not a name used by OpenSSL, but tcpdump registers it - // with |EVP_add_cipher_alias|. Our |EVP_add_cipher_alias| is a - // no-op, so we support the name here. - OPENSSL_strcasecmp(name, "3des") == 0) { - return EVP_des_ede3_cbc(); - } else if (OPENSSL_strcasecmp(name, "aes-128-cbc") == 0) { - return EVP_aes_128_cbc(); - } else if (OPENSSL_strcasecmp(name, "aes-192-cbc") == 0) { - return EVP_aes_192_cbc(); - } else if (OPENSSL_strcasecmp(name, "aes-256-cbc") == 0) { - return EVP_aes_256_cbc(); - } else if (OPENSSL_strcasecmp(name, "aes-128-ctr") == 0) { - return EVP_aes_128_ctr(); - } else if (OPENSSL_strcasecmp(name, "aes-192-ctr") == 0) { - return EVP_aes_192_ctr(); - } else if (OPENSSL_strcasecmp(name, "aes-256-ctr") == 0) { - return EVP_aes_256_ctr(); - } else if (OPENSSL_strcasecmp(name, "aes-128-ecb") == 0) { - return EVP_aes_128_ecb(); - } else if (OPENSSL_strcasecmp(name, "aes-192-ecb") == 0) { - return EVP_aes_192_ecb(); - } else if (OPENSSL_strcasecmp(name, "aes-256-ecb") == 0) { - return EVP_aes_256_ecb(); - } else if (OPENSSL_strcasecmp(name, "aes-128-gcm") == 0) { - return EVP_aes_128_gcm(); - } else if (OPENSSL_strcasecmp(name, "aes-192-gcm") == 0) { - return EVP_aes_192_gcm(); - } else if (OPENSSL_strcasecmp(name, "aes-256-gcm") == 0) { - return EVP_aes_256_gcm(); - } else if (OPENSSL_strcasecmp(name, "aes-128-ofb") == 0) { - return EVP_aes_128_ofb(); - } else if (OPENSSL_strcasecmp(name, "aes-192-ofb") == 0) { - return EVP_aes_192_ofb(); - } else if (OPENSSL_strcasecmp(name, "aes-256-ofb") == 0) { - return EVP_aes_256_ofb(); - } else if (OPENSSL_strcasecmp(name, "des-ecb") == 0) { - return EVP_des_ecb(); - } else if (OPENSSL_strcasecmp(name, "des-ede") == 0) { - return EVP_des_ede(); - } else if (OPENSSL_strcasecmp(name, "des-ede-cbc") == 0) { - return EVP_des_ede_cbc(); - } else if (OPENSSL_strcasecmp(name, "rc2-cbc") == 0) { - return EVP_rc2_cbc(); + // This is not a name used by OpenSSL, but tcpdump registers it with + // |EVP_add_cipher_alias|. Our |EVP_add_cipher_alias| is a no-op, so we + // support the name here. + if (OPENSSL_strcasecmp(name, "3des") == 0) { + name = "des-ede3-cbc"; + } + + for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(kCiphers); i++) { + if (OPENSSL_strcasecmp(kCiphers[i].name, name) == 0) { + return kCiphers[i].func(); + } } return NULL; diff --git a/crypto/cipher_extra/cipher_test.cc b/crypto/cipher_extra/cipher_test.cc index 57fdc8ac1..ad939e249 100644 --- a/crypto/cipher_extra/cipher_test.cc +++ b/crypto/cipher_extra/cipher_test.cc @@ -524,3 +524,22 @@ TEST(CipherTest, SHA1WithSecretSuffix) { } } } + +TEST(CipherTest, GetCipher) { + const EVP_CIPHER *cipher = EVP_get_cipherbynid(NID_aes_128_gcm); + ASSERT_TRUE(cipher); + EXPECT_EQ(NID_aes_128_gcm, EVP_CIPHER_nid(cipher)); + + cipher = EVP_get_cipherbyname("aes-128-gcm"); + ASSERT_TRUE(cipher); + EXPECT_EQ(NID_aes_128_gcm, EVP_CIPHER_nid(cipher)); + + cipher = EVP_get_cipherbyname("AES-128-GCM"); + ASSERT_TRUE(cipher); + EXPECT_EQ(NID_aes_128_gcm, EVP_CIPHER_nid(cipher)); + + // We support a tcpdump-specific alias for 3DES. + cipher = EVP_get_cipherbyname("3des"); + ASSERT_TRUE(cipher); + EXPECT_EQ(NID_des_ede3_cbc, EVP_CIPHER_nid(cipher)); +} diff --git a/decrepit/CMakeLists.txt b/decrepit/CMakeLists.txt index ef95a6be0..16985da19 100644 --- a/decrepit/CMakeLists.txt +++ b/decrepit/CMakeLists.txt @@ -32,6 +32,7 @@ add_executable( blowfish/blowfish_test.cc cast/cast_test.cc cfb/cfb_test.cc + evp/evp_test.cc ripemd/ripemd_test.cc xts/xts_test.cc diff --git a/decrepit/evp/evp_test.cc b/decrepit/evp/evp_test.cc new file mode 100644 index 000000000..1f05ea5fa --- /dev/null +++ b/decrepit/evp/evp_test.cc @@ -0,0 +1,45 @@ +/* Copyright (c) 2021, Google Inc. + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY + * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION + * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ + +#include + +#include +#include +#include + + +// Node.js assumes every cipher in |EVP_CIPHER_do_all_sorted| is accessible via +// |EVP_get_cipherby*|. +TEST(EVPTest, CipherDoAll) { + EVP_CIPHER_do_all_sorted( + [](const EVP_CIPHER *cipher, const char *name, const char *unused, + void *arg) { + SCOPED_TRACE(name); + EXPECT_EQ(cipher, EVP_get_cipherbyname(name)); + EXPECT_EQ(cipher, EVP_get_cipherbynid(EVP_CIPHER_nid(cipher))); + }, + nullptr); +} + +// Node.js assumes every digest in |EVP_MD_do_all_sorted| is accessible via +// |EVP_get_digestby*|. +TEST(EVPTest, MDDoAll) { + EVP_MD_do_all_sorted( + [](const EVP_MD *md, const char *name, const char *unused, void *arg) { + SCOPED_TRACE(name); + EXPECT_EQ(md, EVP_get_digestbyname(name)); + EXPECT_EQ(md, EVP_get_digestbynid(EVP_MD_nid(md))); + }, + nullptr); +} diff --git a/include/openssl/cipher.h b/include/openssl/cipher.h index badd49629..09d72ec2c 100644 --- a/include/openssl/cipher.h +++ b/include/openssl/cipher.h @@ -106,7 +106,10 @@ OPENSSL_EXPORT const EVP_CIPHER *EVP_rc2_cbc(void); const EVP_CIPHER *EVP_rc2_40_cbc(void); // EVP_get_cipherbynid returns the cipher corresponding to the given NID, or -// NULL if no such cipher is known. +// NULL if no such cipher is known. Note using this function links almost every +// cipher implemented by BoringSSL into the binary, whether the caller uses them +// or not. Size-conscious callers, such as client software, should not use this +// function. OPENSSL_EXPORT const EVP_CIPHER *EVP_get_cipherbynid(int nid); @@ -409,7 +412,10 @@ OPENSSL_EXPORT int EVP_DecryptInit(EVP_CIPHER_CTX *ctx, OPENSSL_EXPORT int EVP_add_cipher_alias(const char *a, const char *b); // EVP_get_cipherbyname returns an |EVP_CIPHER| given a human readable name in -// |name|, or NULL if the name is unknown. +// |name|, or NULL if the name is unknown. Note using this function links almost +// every cipher implemented by BoringSSL into the binary, not just the ones the +// caller requests. Size-conscious callers, such as client software, should not +// use this function. OPENSSL_EXPORT const EVP_CIPHER *EVP_get_cipherbyname(const char *name); // These AEADs are deprecated AES-GCM implementations that set From 551ccd7e949c8fbf5acae44149ac1bbc9d79aa42 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 28 Sep 2021 11:55:10 -0400 Subject: [PATCH 2/3] Fix CRYPTO_malloc, etc., definitions. In upstream, these functions take file and line number arguments. Update ours to match. Guessing almost no one uses these, or we'd have caught this earlier. Change-Id: Ic09f8d8274065ac02efa78e70c215b87fa765b9f Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49665 Reviewed-by: Adam Langley Reviewed-by: Benjamin Brittain Commit-Queue: David Benjamin --- crypto/mem.c | 10 ++++++++++ include/openssl/mem.h | 12 +++++++++--- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/crypto/mem.c b/crypto/mem.c index 8988937ed..639de3224 100644 --- a/crypto/mem.c +++ b/crypto/mem.c @@ -380,3 +380,13 @@ void *OPENSSL_memdup(const void *data, size_t size) { OPENSSL_memcpy(ret, data, size); return ret; } + +void *CRYPTO_malloc(size_t size, const char *file, int line) { + return OPENSSL_malloc(size); +} + +void *CRYPTO_realloc(void *ptr, size_t new_size, const char *file, int line) { + return OPENSSL_realloc(ptr, new_size); +} + +void CRYPTO_free(void *ptr, const char *file, int line) { OPENSSL_free(ptr); } diff --git a/include/openssl/mem.h b/include/openssl/mem.h index 9906d5bfe..476299ae0 100644 --- a/include/openssl/mem.h +++ b/include/openssl/mem.h @@ -150,9 +150,15 @@ OPENSSL_EXPORT size_t OPENSSL_strlcat(char *dst, const char *src, // Deprecated functions. -#define CRYPTO_malloc OPENSSL_malloc -#define CRYPTO_realloc OPENSSL_realloc -#define CRYPTO_free OPENSSL_free +// CRYPTO_malloc calls |OPENSSL_malloc|. |file| and |line| are ignored. +OPENSSL_EXPORT void *CRYPTO_malloc(size_t size, const char *file, int line); + +// CRYPTO_realloc calls |OPENSSL_realloc|. |file| and |line| are ignored. +OPENSSL_EXPORT void *CRYPTO_realloc(void *ptr, size_t new_size, + const char *file, int line); + +// CRYPTO_free calls |OPENSSL_free|. |file| and |line| are ignored. +OPENSSL_EXPORT void CRYPTO_free(void *ptr, const char *file, int line); // OPENSSL_clear_free calls |OPENSSL_free|. BoringSSL automatically clears all // allocations on free, but we define |OPENSSL_clear_free| for compatibility. From cc509bdb7e4e0c6df228da71ae1bc86b1717ba3b Mon Sep 17 00:00:00 2001 From: Pete Bentley Date: Tue, 28 Sep 2021 20:41:03 +0100 Subject: [PATCH 3/3] Add log tag for Trusty. Trusty's TLOGE macro nowadays expects TLOG_TAG to be defined as the log tag to use. Change-Id: I18121287ba51698d354323027d5382c8406f0b99 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/49685 Commit-Queue: Pete Bentley Commit-Queue: David Benjamin Reviewed-by: David Benjamin --- util/fipstools/acvp/modulewrapper/modulewrapper.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/util/fipstools/acvp/modulewrapper/modulewrapper.cc b/util/fipstools/acvp/modulewrapper/modulewrapper.cc index 1974dce83..7188029ee 100644 --- a/util/fipstools/acvp/modulewrapper/modulewrapper.cc +++ b/util/fipstools/acvp/modulewrapper/modulewrapper.cc @@ -54,6 +54,7 @@ namespace acvp { #if defined(OPENSSL_TRUSTY) #include #define LOG_ERROR(...) TLOGE(__VA_ARGS__) +#define TLOG_TAG "modulewrapper" #else #define LOG_ERROR(...) fprintf(stderr, __VA_ARGS__) #endif // OPENSSL_TRUSTY