Skip to content

Commit e5747bd

Browse files
Allow zero-length PEM passwords in callback paths (#3073)
### Description of changes: Four sites incorrectly rejected empty passwords by checking pass_len <= 0 instead of pass_len < 0 after invoking the password callback. The callback returns negative on error and 0 for a valid empty password, so the check should be < 0. ### Testing: How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer? CI By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
1 parent 1b40140 commit e5747bd

3 files changed

Lines changed: 62 additions & 4 deletions

File tree

crypto/pem/pem_lib.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ int PEM_ASN1_write_bio(i2d_of_void *i2d, const char *name, BIO *bp, void *x,
322322
callback = PEM_def_callback;
323323
}
324324
pass_len = (*callback)(buf, PEM_BUFSIZE, 1, u);
325-
if (pass_len <= 0) {
325+
if (pass_len < 0) {
326326
OPENSSL_PUT_ERROR(PEM, PEM_R_READ_KEY);
327327
goto err;
328328
}
@@ -397,7 +397,7 @@ int PEM_do_header(EVP_CIPHER_INFO *cipher, unsigned char *data, long *plen,
397397
callback = PEM_def_callback;
398398
}
399399
pass_len = callback(buf, PEM_BUFSIZE, 0, u);
400-
if (pass_len <= 0) {
400+
if (pass_len < 0) {
401401
OPENSSL_PUT_ERROR(PEM, PEM_R_BAD_PASSWORD_READ);
402402
return 0;
403403
}

crypto/pem/pem_pk8.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ static int do_pk8pkey(BIO *bp, const EVP_PKEY *x, int isder, int nid,
117117
cb = PEM_def_callback;
118118
}
119119
pass_len = cb(buf, PEM_BUFSIZE, 1, u);
120-
if (pass_len <= 0) {
120+
if (pass_len < 0) {
121121
OPENSSL_PUT_ERROR(PEM, PEM_R_READ_KEY);
122122
PKCS8_PRIV_KEY_INFO_free(p8inf);
123123
return 0;
@@ -164,7 +164,7 @@ EVP_PKEY *d2i_PKCS8PrivateKey_bio(BIO *bp, EVP_PKEY **x, pem_password_cb *cb,
164164
cb = PEM_def_callback;
165165
}
166166
pass_len = cb(psbuf, PEM_BUFSIZE, 0, u);
167-
if (pass_len <= 0) {
167+
if (pass_len < 0) {
168168
OPENSSL_PUT_ERROR(PEM, PEM_R_BAD_PASSWORD_READ);
169169
X509_SIG_free(p8);
170170
return NULL;

crypto/pem/pem_test.cc

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -467,3 +467,61 @@ TEST(PEMTest, WriteReadTraditionalPem) {
467467
EXPECT_FALSE(PEM_write_bio_PrivateKey_traditional(
468468
write_bio.get(), pkey.get(), nullptr, nullptr, 0, nullptr, nullptr));
469469
}
470+
471+
TEST(PEMTest, WriteReadECPemEmptyPassword) {
472+
bssl::UniquePtr<EC_KEY> ec_key(EC_KEY_new());
473+
ASSERT_TRUE(ec_key);
474+
bssl::UniquePtr<EC_GROUP> ec_group(EC_GROUP_new_by_curve_name(NID_X9_62_prime256v1));
475+
ASSERT_TRUE(ec_group);
476+
ASSERT_TRUE(EC_KEY_set_group(ec_key.get(), ec_group.get()));
477+
478+
#if defined(BORINGSSL_FIPS)
479+
ASSERT_TRUE(EC_KEY_generate_key_fips(ec_key.get()));
480+
#else
481+
ASSERT_TRUE(EC_KEY_generate_key(ec_key.get()));
482+
#endif
483+
484+
bssl::UniquePtr<BIO> write_bio(BIO_new(BIO_s_mem()));
485+
ASSERT_TRUE(write_bio);
486+
const EVP_CIPHER* cipher = EVP_get_cipherbynid(NID_aes_256_cbc);
487+
ASSERT_TRUE(cipher);
488+
ASSERT_TRUE(PEM_write_bio_ECPrivateKey(write_bio.get(), ec_key.get(), cipher, nullptr, 0, pem_password_callback, (void*)""));
489+
490+
const uint8_t* content = nullptr;
491+
size_t content_len = 0;
492+
BIO_mem_contents(write_bio.get(), &content, &content_len);
493+
494+
bssl::UniquePtr<BIO> read_bio(BIO_new_mem_buf(content, content_len) );
495+
ASSERT_TRUE(read_bio);
496+
bssl::UniquePtr<EC_KEY> ec_key_read(PEM_read_bio_ECPrivateKey(read_bio.get(), nullptr, pem_password_callback, (void*)""));
497+
ASSERT_TRUE(ec_key_read);
498+
const BIGNUM* orig_priv_key = EC_KEY_get0_private_key(ec_key.get());
499+
const BIGNUM* read_priv_key = EC_KEY_get0_private_key(ec_key_read.get());
500+
ASSERT_EQ(0, BN_cmp(orig_priv_key, read_priv_key));
501+
}
502+
503+
TEST(PEMTest, WriteReadPKCS8DerEmptyPassword) {
504+
bssl::UniquePtr<EC_KEY> ec_key(EC_KEY_new_by_curve_name(NID_X9_62_prime256v1));
505+
ASSERT_TRUE(ec_key);
506+
ASSERT_TRUE(EC_KEY_generate_key(ec_key.get()));
507+
bssl::UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new());
508+
ASSERT_TRUE(pkey);
509+
ASSERT_TRUE(EVP_PKEY_set1_EC_KEY(pkey.get(), ec_key.get()));
510+
511+
bssl::UniquePtr<BIO> write_bio(BIO_new(BIO_s_mem()));
512+
ASSERT_TRUE(write_bio);
513+
ASSERT_TRUE(i2d_PKCS8PrivateKey_bio(write_bio.get(), pkey.get(), EVP_aes_256_cbc(), nullptr, 0, pem_password_callback, (void*)""));
514+
515+
const uint8_t* content = nullptr;
516+
size_t content_len = 0;
517+
BIO_mem_contents(write_bio.get(), &content, &content_len);
518+
519+
bssl::UniquePtr<BIO> read_bio(BIO_new_mem_buf(content, content_len) );
520+
ASSERT_TRUE(read_bio);
521+
bssl::UniquePtr<EVP_PKEY> pkey_read(d2i_PKCS8PrivateKey_bio(read_bio.get(), nullptr, pem_password_callback, (void*)""));
522+
ASSERT_TRUE(pkey_read);
523+
524+
const EC_KEY* read_ec = EVP_PKEY_get0_EC_KEY(pkey_read.get());
525+
ASSERT_TRUE(read_ec);
526+
ASSERT_EQ(0, BN_cmp(EC_KEY_get0_private_key(ec_key.get()), EC_KEY_get0_private_key(read_ec)));
527+
}

0 commit comments

Comments
 (0)