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));
+}
