diff --git a/src/crypto/pem/pem_lib.c b/src/crypto/pem/pem_lib.c index 38c1d3f6a..6b272ccb6 100644 --- a/src/crypto/pem/pem_lib.c +++ b/src/crypto/pem/pem_lib.c @@ -312,12 +312,11 @@ int PEM_ASN1_write_bio(i2d_of_void *i2d, const char *name, BIO *bp, void *x, const unsigned iv_len = EVP_CIPHER_iv_length(enc); if (pass == NULL) { - pass_len = 0; if (!callback) { callback = PEM_def_callback; } pass_len = (*callback)(buf, PEM_BUFSIZE, 1, u); - if (pass_len <= 0) { + if (pass_len < 0) { OPENSSL_PUT_ERROR(PEM, PEM_R_READ_KEY); goto err; } @@ -393,7 +392,7 @@ int PEM_do_header(EVP_CIPHER_INFO *cipher, unsigned char *data, long *plen, callback = PEM_def_callback; } pass_len = callback(buf, PEM_BUFSIZE, 0, u); - if (pass_len <= 0) { + if (pass_len < 0) { OPENSSL_PUT_ERROR(PEM, PEM_R_BAD_PASSWORD_READ); return 0; } @@ -779,11 +778,11 @@ err: int PEM_def_callback(char *buf, int size, int rwflag, void *userdata) { if (!buf || !userdata || size < 0) { - return 0; + return -1; } size_t len = strlen((char *)userdata); if (len >= (size_t)size) { - return 0; + return -1; } OPENSSL_strlcpy(buf, userdata, (size_t)size); return (int)len; diff --git a/src/crypto/pem/pem_pk8.c b/src/crypto/pem/pem_pk8.c index 9c6419be4..2fc0673cd 100644 --- a/src/crypto/pem/pem_pk8.c +++ b/src/crypto/pem/pem_pk8.c @@ -113,12 +113,11 @@ static int do_pk8pkey(BIO *bp, const EVP_PKEY *x, int isder, int nid, } if (enc || (nid != -1)) { if (!pass) { - pass_len = 0; if (!cb) { cb = PEM_def_callback; } pass_len = cb(buf, PEM_BUFSIZE, 1, u); - if (pass_len <= 0) { + if (pass_len < 0) { OPENSSL_PUT_ERROR(PEM, PEM_R_READ_KEY); PKCS8_PRIV_KEY_INFO_free(p8inf); return 0; @@ -166,7 +165,7 @@ EVP_PKEY *d2i_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY **x, pem_password_cb *cb, cb = PEM_def_callback; } pass_len = cb(psbuf, PEM_BUFSIZE, 0, u); - if (pass_len <= 0) { + if (pass_len < 0) { OPENSSL_PUT_ERROR(PEM, PEM_R_BAD_PASSWORD_READ); X509_SIG_free(p8); return NULL; diff --git a/src/crypto/pem/pem_pkey.c b/src/crypto/pem/pem_pkey.c index 2fb450c9b..9349ac71e 100644 --- a/src/crypto/pem/pem_pkey.c +++ b/src/crypto/pem/pem_pkey.c @@ -110,7 +110,7 @@ EVP_PKEY *PEM_read_bio_PrivateKey(BIO *bp, EVP_PKEY **x, pem_password_cb *cb, cb = PEM_def_callback; } pass_len = cb(psbuf, PEM_BUFSIZE, 0, u); - if (pass_len <= 0) { + if (pass_len < 0) { OPENSSL_PUT_ERROR(PEM, PEM_R_BAD_PASSWORD_READ); X509_SIG_free(p8); goto err; diff --git a/src/crypto/pem/pem_test.cc b/src/crypto/pem/pem_test.cc index 117e00062..d3c8f14ca 100644 --- a/src/crypto/pem/pem_test.cc +++ b/src/crypto/pem/pem_test.cc @@ -14,9 +14,12 @@ #include +#include + #include #include +#include #include #include @@ -44,3 +47,269 @@ TEST(PEMTest, NoRC4) { EXPECT_TRUE( ErrorEquals(ERR_get_error(), ERR_LIB_PEM, PEM_R_UNSUPPORTED_ENCRYPTION)); } + +static std::vector DecodePEMBytes(const char *pem) { + bssl::UniquePtr bio(BIO_new_mem_buf(pem, -1)); + char *name, *header; + uint8_t *data; + long len; + if (bio == nullptr || + !PEM_read_bio(bio.get(), &name, &header, &data, &len)) { + return {}; + } + bssl::UniquePtr free_name(name), free_header(header); + bssl::UniquePtr free_data(data); + return std::vector(data, data + len); +} + +TEST(PEMTest, DecryptPassword) { + // A private key encrypted with the password "password", encrypted at the + // PKCS#8 level. + static const char kEncryptedPEM[] = R"( +-----BEGIN ENCRYPTED PRIVATE KEY----- +MIHeMEkGCSqGSIb3DQEFDTA8MBsGCSqGSIb3DQEFDDAOBAjnhMUlb9deeQICCAAw +HQYJYIZIAWUDBAECBBAO8j5GA5VK8wjvNrzp/iVhBIGQyQKFfFKlFhxiDkFfyhUc +nPLr0eboQOz8eIaTW1Rblo/qDkQwNtONyfYn909SoIP7iU8UehcBG1UQe41WvQpu +yRKYQteoWSzFl+yzktL2Y/25K7Uc+f2NScjdonYMZ+9/m1HGmEzKO+Hz28cAsJL7 +rH2gQ0lkxr1GtW77m2rfMKKuGYhpkgjWUbzJwP9v3iq+ +-----END ENCRYPTED PRIVATE KEY----- +)"; + // The same key and password, but encrypted at the PEM level. + static const char kEncryptedPEM2[] = R"( +-----BEGIN EC PRIVATE KEY----- +Proc-Type: 4,ENCRYPTED +DEK-Info: AES-128-CBC,B3B2988AECAE6EAB0D043105994C1123 + +RK7DUIGDHWTFh2rpTX+dR88hUyC1PyDlIULiNCkuWFwHrJbc1gM6hMVOKmU196XC +iITrIKmilFm9CPD6Tpfk/NhI/QPxyJlk1geIkxpvUZ2FCeMuYI1To14oYOUKv14q +wr6JtaX2G+pOmwcSPymZC4u2TncAP7KHgS8UGcMw8CE= +-----END EC PRIVATE KEY----- +)"; + + for (const char *pem : {kEncryptedPEM, kEncryptedPEM2}) { + SCOPED_TRACE(pem); + // Decrypt with the correct password. + { + bssl::UniquePtr bio(BIO_new_mem_buf(pem, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey(PEM_read_bio_PrivateKey( + bio.get(), nullptr, nullptr, const_cast("password"))); + EXPECT_TRUE(pkey); + } + + // Decrypt with the wrong password. + { + bssl::UniquePtr bio(BIO_new_mem_buf(pem, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey(PEM_read_bio_PrivateKey( + bio.get(), nullptr, nullptr, const_cast("wrong"))); + EXPECT_FALSE(pkey); + EXPECT_TRUE( + ErrorEquals(ERR_peek_error(), ERR_LIB_CIPHER, CIPHER_R_BAD_DECRYPT)); + ERR_clear_error(); + } + + // If the caller did not pass in a password, we should not proceed to try to + // decrypt. + { + bssl::UniquePtr bio(BIO_new_mem_buf(pem, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey( + PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr)); + EXPECT_FALSE(pkey); + EXPECT_TRUE( + ErrorEquals(ERR_peek_error(), ERR_LIB_PEM, PEM_R_BAD_PASSWORD_READ)); + ERR_clear_error(); + } + + // If the password, with a NUL terminator, does not fit in the internal + // buffer used by the PEM library, the PEM library should notice. + { + std::string too_long(PEM_BUFSIZE, 'a'); + bssl::UniquePtr bio(BIO_new_mem_buf(pem, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey(PEM_read_bio_PrivateKey( + bio.get(), nullptr, nullptr, const_cast(too_long.c_str()))); + EXPECT_FALSE(pkey); + EXPECT_TRUE( + ErrorEquals(ERR_peek_error(), ERR_LIB_PEM, PEM_R_BAD_PASSWORD_READ)); + ERR_clear_error(); + } + } + + // |d2i_PKCS8PrivateKey_bio| should also be able to manage the password + // callback correctly. + std::vector bytes = DecodePEMBytes(kEncryptedPEM); + ASSERT_FALSE(bytes.empty()); + { + bssl::UniquePtr bio(BIO_new_mem_buf(bytes.data(), bytes.size())); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey(d2i_PKCS8PrivateKey_bio( + bio.get(), nullptr, nullptr, const_cast("password"))); + EXPECT_TRUE(pkey); + } + + { + bssl::UniquePtr bio(BIO_new_mem_buf(bytes.data(), bytes.size())); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey( + d2i_PKCS8PrivateKey_bio(bio.get(), nullptr, nullptr, nullptr)); + EXPECT_FALSE(pkey); + EXPECT_TRUE( + ErrorEquals(ERR_peek_error(), ERR_LIB_PEM, PEM_R_BAD_PASSWORD_READ)); + ERR_clear_error(); + } + + { + std::string too_long(PEM_BUFSIZE, 'a'); + bssl::UniquePtr bio(BIO_new_mem_buf(bytes.data(), bytes.size())); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey(d2i_PKCS8PrivateKey_bio( + bio.get(), nullptr, nullptr, const_cast(too_long.c_str()))); + EXPECT_FALSE(pkey); + EXPECT_TRUE( + ErrorEquals(ERR_peek_error(), ERR_LIB_PEM, PEM_R_BAD_PASSWORD_READ)); + ERR_clear_error(); + } + + // A private key encrypted with the empty password, encrypted at the PKCS#8 + // level. + static const char kEncryptedPEMEmpty[] = R"( +-----BEGIN ENCRYPTED PRIVATE KEY----- +MIH0MF8GCSqGSIb3DQEFDTBSMDEGCSqGSIb3DQEFDDAkBBAXiHC8iDcjzF0I+D2g +zJOcAgIIADAMBggqhkiG9w0CCQUAMB0GCWCGSAFlAwQBAgQQwupOMi8DtEWiuXt5 +Odla9QSBkC37uJuG7HSCOyTVCEW76Kmf7GoH+Ou17bDAp6NGwm3KLxRfFoExki9g +hyLzdarBnhRbPqwMixhaQ2AtkpoSmjristGzZ9U7Y+TM3NnCA4+bu1TckdBn0g+Q +fvZI9eydS9buA0deGxCUytrMWrR3PxS1yoXBywMDJTom8u5hvvvkJ9WcNzUVRf0D +6z5NHHiXsQ== +-----END ENCRYPTED PRIVATE KEY----- +)"; + // THe same key and password, but encrypted at the PEM level. + static const char kEncryptedPEMEmpty2[] = R"( +-----BEGIN EC PRIVATE KEY----- +Proc-Type: 4,ENCRYPTED +DEK-Info: AES-128-CBC,A9505A7DD5C3B51D8AACED18F5758256 + +yfJKjep7Koj8hU/PtGC+NNXSNbItQ2zyeXDMVoazffraoDGMg6g1hFPPjg9reC+J +iQQIf9uACF27zi9fpWwbszszimrxl0u6n0ddBXizcK6xzkTvk3PZ67Vz1KYmotwC +XjgdgSEeixwKhDOuHKFdlFGP/7sw5GHlK3jPSpqi2gI= +-----END EC PRIVATE KEY----- +)"; + + for (const char *pem : {kEncryptedPEMEmpty, kEncryptedPEMEmpty2}) { + SCOPED_TRACE(pem); + + // The empty password should be correctly interpreted as a password. + { + bssl::UniquePtr bio(BIO_new_mem_buf(pem, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey(PEM_read_bio_PrivateKey( + bio.get(), nullptr, nullptr, const_cast(""))); + EXPECT_TRUE(pkey); + } + } + + // |d2i_PKCS8PrivateKey_bio| should also be able to manage the password + // callback correctly. + bytes = DecodePEMBytes(kEncryptedPEMEmpty); + { + ASSERT_FALSE(bytes.empty()); + bssl::UniquePtr bio(BIO_new_mem_buf(bytes.data(), bytes.size())); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey(d2i_PKCS8PrivateKey_bio( + bio.get(), nullptr, nullptr, const_cast(""))); + EXPECT_TRUE(pkey); + } +} + +TEST(PEMTest, EncryptPassword) { + static const char kKey[] = R"( +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgBw8IcnrUoEqc3VnJ +TYlodwi1b8ldMHcO6NHJzgqLtGqhRANCAATmK2niv2Wfl74vHg2UikzVl2u3qR4N +Rvvdqakendy6WgHn1peoChj5w8SjHlbifINI2xYaHPUdfvGULUvPciLB +-----END PRIVATE KEY----- +)"; + bssl::UniquePtr bio(BIO_new_mem_buf(kKey, -1)); + ASSERT_TRUE(bio); + bssl::UniquePtr pkey( + PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, nullptr)); + EXPECT_TRUE(pkey); + + // There are many ways to encrypt a PEM blob with a password. + struct PasswordMethod { + const char *name; + std::function func; + bool is_callback; + }; + const PasswordMethod kPasswordMethods[] = { + {"PKCS#8 encryption, password from param", + [&](BIO *out, const char *pass) -> bool { + return PEM_write_bio_PrivateKey( + out, pkey.get(), EVP_aes_128_cbc(), + reinterpret_cast(pass), + pass == nullptr ? 0 : strlen(pass), nullptr, nullptr); + }, + /*is_callback=*/false}, + {"PKCS#8 encryption, password from callback", + [&](BIO *out, const char *pass) -> bool { + return PEM_write_bio_PrivateKey(out, pkey.get(), EVP_aes_128_cbc(), + nullptr, 0, nullptr, + const_cast(pass)); + }, + /*is_callback=*/true}, + {"PEM-level encryption, password from param", + [&](BIO *out, const char *pass) -> bool { + return PEM_write_bio_ECPrivateKey( + out, EVP_PKEY_get0_EC_KEY(pkey.get()), EVP_aes_128_cbc(), nullptr, + 0, nullptr, const_cast(pass)); + }, + /*is_callback=*/false}, + {"PKCS#8 encryption, password from callback", + [&](BIO *out, const char *pass) -> bool { + return PEM_write_bio_ECPrivateKey( + out, EVP_PKEY_get0_EC_KEY(pkey.get()), EVP_aes_128_cbc(), nullptr, + 0, nullptr, const_cast(pass)); + }, + /*is_callback=*/true}, + }; + for (const auto &p : kPasswordMethods) { + SCOPED_TRACE(p.name); + + // Encrypting the private key with a password should work. + bio.reset(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); + ASSERT_TRUE(p.func(bio.get(), "password")); + + // Check we can decrypt it. + bssl::UniquePtr pkey2(PEM_read_bio_PrivateKey( + bio.get(), nullptr, nullptr, const_cast("password"))); + ASSERT_TRUE(pkey2); + + // The empty string is a valid password. + bio.reset(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); + ASSERT_TRUE(p.func(bio.get(), "")); + + // Check we can decrypt it. + pkey2.reset(PEM_read_bio_PrivateKey(bio.get(), nullptr, nullptr, + const_cast(""))); + ASSERT_TRUE(pkey2); + + // Check error-handling when the password is specified via the callback. + if (p.is_callback) { + bio.reset(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); + EXPECT_FALSE(p.func(bio.get(), nullptr)); + EXPECT_TRUE(ErrorEquals(ERR_peek_error(), ERR_LIB_PEM, PEM_R_READ_KEY)); + ERR_clear_error(); + + std::string too_long(PEM_BUFSIZE, 'a'); + bio.reset(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio); + EXPECT_FALSE(p.func(bio.get(), too_long.c_str())); + EXPECT_TRUE(ErrorEquals(ERR_peek_error(), ERR_LIB_PEM, PEM_R_READ_KEY)); + ERR_clear_error(); + } + } +} diff --git a/src/include/openssl/pem.h b/src/include/openssl/pem.h index 919cacdea..49ac6dc3e 100644 --- a/src/include/openssl/pem.h +++ b/src/include/openssl/pem.h @@ -385,10 +385,9 @@ OPENSSL_EXPORT int PEM_ASN1_write(i2d_of_void *i2d, const char *name, FILE *fp, pem_password_cb *callback, void *u); // PEM_def_callback treats |userdata| as a string and copies it into |buf|, -// assuming its |size| is sufficient. Returns the length of the string, or 0 -// if there is not enough room. If either |buf| or |userdata| is NULL, 0 is -// returned. Note that this is different from OpenSSL, which prompts for a -// password. +// assuming its |size| is sufficient. Returns the length of the string, or -1 on +// error. Error cases the buffer being too small, or |buf| and |userdata| being +// NULL. Note that this is different from OpenSSL, which prompts for a password. OPENSSL_EXPORT int PEM_def_callback(char *buf, int size, int rwflag, void *userdata);