Don't allow RC4 in PEM. This fixes uninitialized memory read reported by Nick Mathewson in https://github.com/openssl/openssl/issues/6347. It imports the memset from upstream's 2c739f72e5236a8e0c351c00047c77083dcdb77f, but I believe that fix is incorrect and instead RC4 shouldn't be allowed in this context. See https://github.com/openssl/openssl/pull/6603#issuecomment-413066462 for details. Update-Note: Decoding a password-protected PEM block with RC4 will, rather than derive garbage from uninitialized memory, simply fail. Trying to encode a password-protect PEM block with an unsupported cipher will also fail, rather than output garbage (e.g. tag-less AES-GCM). Change-Id: Ib7e23dbf5514f0a523730926daad3c0bdb989417 Reviewed-on: https://boringssl-review.googlesource.com/31084 Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt index 5f6e327..2684750 100644 --- a/crypto/CMakeLists.txt +++ b/crypto/CMakeLists.txt
@@ -260,6 +260,7 @@ hmac_extra/hmac_test.cc lhash/lhash_test.cc obj/obj_test.cc + pem/pem_test.cc pkcs7/pkcs7_test.cc pkcs8/pkcs8_test.cc pkcs8/pkcs12_test.cc
diff --git a/crypto/pem/pem_lib.c b/crypto/pem/pem_lib.c index 8f89096..5180e55 100644 --- a/crypto/pem/pem_lib.c +++ b/crypto/pem/pem_lib.c
@@ -188,6 +188,26 @@ return 0; } +static const EVP_CIPHER *cipher_by_name(const char *name) +{ + /* This is similar to the (deprecated) function |EVP_get_cipherbyname|. Note + * the PEM code assumes that ciphers have at least 8 bytes of IV, at most 20 + * bytes of overhead and generally behave like CBC mode. */ + if (0 == strcmp(name, SN_des_cbc)) { + return EVP_des_cbc(); + } else if (0 == strcmp(name, SN_des_ede3_cbc)) { + return EVP_des_ede3_cbc(); + } else if (0 == strcmp(name, SN_aes_128_cbc)) { + return EVP_aes_128_cbc(); + } else if (0 == strcmp(name, SN_aes_192_cbc)) { + return EVP_aes_192_cbc(); + } else if (0 == strcmp(name, SN_aes_256_cbc)) { + return EVP_aes_256_cbc(); + } else { + return NULL; + } +} + int PEM_bytes_read_bio(unsigned char **pdata, long *plen, char **pnm, const char *name, BIO *bp, pem_password_cb *cb, void *u) @@ -265,7 +285,9 @@ if (enc != NULL) { objstr = OBJ_nid2sn(EVP_CIPHER_nid(enc)); - if (objstr == NULL || EVP_CIPHER_iv_length(enc) == 0) { + if (objstr == NULL || + cipher_by_name(objstr) == NULL || + EVP_CIPHER_iv_length(enc) < 8) { OPENSSL_PUT_ERROR(PEM, PEM_R_UNSUPPORTED_CIPHER); goto err; } @@ -393,26 +415,6 @@ return (1); } -static const EVP_CIPHER *cipher_by_name(const char *name) -{ - /* This is similar to the (deprecated) function |EVP_get_cipherbyname|. */ - if (0 == strcmp(name, SN_rc4)) { - return EVP_rc4(); - } else if (0 == strcmp(name, SN_des_cbc)) { - return EVP_des_cbc(); - } else if (0 == strcmp(name, SN_des_ede3_cbc)) { - return EVP_des_ede3_cbc(); - } else if (0 == strcmp(name, SN_aes_128_cbc)) { - return EVP_aes_128_cbc(); - } else if (0 == strcmp(name, SN_aes_192_cbc)) { - return EVP_aes_192_cbc(); - } else if (0 == strcmp(name, SN_aes_256_cbc)) { - return EVP_aes_256_cbc(); - } else { - return NULL; - } -} - int PEM_get_EVP_CIPHER_INFO(char *header, EVP_CIPHER_INFO *cipher) { const EVP_CIPHER *enc = NULL; @@ -420,6 +422,7 @@ char **header_pp = &header; cipher->cipher = NULL; + OPENSSL_memset(cipher->iv, 0, sizeof(cipher->iv)); if ((header == NULL) || (*header == '\0') || (*header == '\n')) return (1); if (strncmp(header, "Proc-Type: ", 11) != 0) { @@ -466,6 +469,13 @@ OPENSSL_PUT_ERROR(PEM, PEM_R_UNSUPPORTED_ENCRYPTION); return (0); } + // The IV parameter must be at least 8 bytes long to be used as the salt in + // the KDF. (This should not happen given |cipher_by_name|.) + if (EVP_CIPHER_iv_length(enc) < 8) { + assert(0); + OPENSSL_PUT_ERROR(PEM, PEM_R_UNSUPPORTED_ENCRYPTION); + return 0; + } if (!load_iv(header_pp, &(cipher->iv[0]), EVP_CIPHER_iv_length(enc))) return (0);
diff --git a/crypto/pem/pem_test.cc b/crypto/pem/pem_test.cc new file mode 100644 index 0000000..aed523c --- /dev/null +++ b/crypto/pem/pem_test.cc
@@ -0,0 +1,45 @@ +/* Copyright (c) 2018, 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 <openssl/pem.h> + +#include <gtest/gtest.h> + +#include <openssl/bio.h> +#include <openssl/err.h> +#include <openssl/rsa.h> + + +// Test that implausible ciphers, notably an IV-less RC4, aren't allowed in PEM. +// This is a regression test for https://github.com/openssl/openssl/issues/6347, +// though our fix differs from upstream. +TEST(PEMTest, NoRC4) { + static const char kPEM[] = + "-----BEGIN RSA PUBLIC KEY-----\n" + "Proc-Type: 4,ENCRYPTED\n" + "DEK-Info: RC4 -\n" + "extra-info\n" + "router-signature\n" + "\n" + "Z1w=\n" + "-----END RSA PUBLIC KEY-----\n"; + bssl::UniquePtr<BIO> bio(BIO_new_mem_buf(kPEM, sizeof(kPEM) - 1)); + ASSERT_TRUE(bio); + bssl::UniquePtr<RSA> rsa(PEM_read_bio_RSAPublicKey( + bio.get(), nullptr, nullptr, const_cast<char *>("password"))); + EXPECT_FALSE(rsa); + uint32_t err = ERR_get_error(); + EXPECT_EQ(ERR_LIB_PEM, ERR_GET_LIB(err)); + EXPECT_EQ(PEM_R_UNSUPPORTED_ENCRYPTION, ERR_GET_REASON(err)); +}