Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for PEM Parameters without ASN1 hooks #1831

Merged
merged 2 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crypto/pem/pem_lib.c
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
#include <openssl/x509.h>

#include "../internal.h"
#include "../fipsmodule/evp/internal.h"


#define MIN_LENGTH 4
Expand Down Expand Up @@ -146,6 +147,13 @@ static int check_pem(const char *nm, const char *name) {
!strcmp(nm, PEM_STRING_DSA);
}

// These correspond with the PEM strings that have "PARAMETERS".
if (!strcmp(name, PEM_STRING_PARAMETERS)) {
return !strcmp(nm, PEM_STRING_ECPARAMETERS) ||
!strcmp(nm, PEM_STRING_DSAPARAMS) ||
!strcmp(nm, PEM_STRING_DHPARAMS);
}

// Permit older strings

Comment on lines +156 to 158
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: The remainder of this function does not have any test coverage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the remainder are related to PEM functions we're implementing for this PR. This function is a generic function for parsing the first line of PEM files and the rest below apply to other supported formats.

if (!strcmp(nm, PEM_STRING_X509_OLD) && !strcmp(name, PEM_STRING_X509)) {
Expand Down
83 changes: 83 additions & 0 deletions crypto/pem/pem_pkey.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
#include <openssl/pkcs8.h>
#include <openssl/rand.h>
#include <openssl/x509.h>
#include "../fipsmodule/evp/internal.h"

EVP_PKEY *PEM_read_bio_PrivateKey(BIO *bp, EVP_PKEY **x, pem_password_cb *cb,
void *u) {
Expand Down Expand Up @@ -156,6 +157,88 @@ int PEM_write_bio_PrivateKey(BIO *bp, EVP_PKEY *x, const EVP_CIPHER *enc,
return PEM_write_bio_PKCS8PrivateKey(bp, x, enc, (char *)kstr, klen, cb, u);
}

EVP_PKEY *PEM_read_bio_Parameters(BIO *bio, EVP_PKEY **pkey) {
char *nm = NULL;
unsigned char *data = NULL;
long len;
if (!PEM_bytes_read_bio(&data, &len, &nm, PEM_STRING_PARAMETERS, bio, 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NP: We have zero documentation for PEM_bytes_read_bio.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added documentation

NULL)) {
return NULL;
}
const unsigned char *data_const = data;

// Implementing the ASN1 logic here allows us to decouple the pem logic for
// |EVP_PKEY|. These correspond to the historical |param_decode|
// |EVP_PKEY_ASN1_METHOD| hooks in OpenSSL.
EVP_PKEY *ret = EVP_PKEY_new();
samuel40791765 marked this conversation as resolved.
Show resolved Hide resolved
if (strcmp(nm, PEM_STRING_ECPARAMETERS) == 0) {
EC_KEY *ec_key = d2i_ECParameters(NULL, &data_const, len);
if (ec_key == NULL || !EVP_PKEY_assign_EC_KEY(ret, ec_key)) {
OPENSSL_PUT_ERROR(EVP, ERR_R_EC_LIB);
EC_KEY_free(ec_key);
goto err;
}
} else if (strcmp(nm, PEM_STRING_DSAPARAMS) == 0) {
DSA *dsa = d2i_DSAparams(NULL, &data_const, len);
if (dsa == NULL || !EVP_PKEY_assign_DSA(ret, dsa)) {
OPENSSL_PUT_ERROR(EVP, ERR_R_DSA_LIB);
DSA_free(dsa);
goto err;
}
} else if (strcmp(nm, PEM_STRING_DHPARAMS) == 0) {
DH *dh = d2i_DHparams(NULL, &data_const, len);
if (dh == NULL || !EVP_PKEY_assign_DH(ret, dh)) {
OPENSSL_PUT_ERROR(EVP, ERR_R_DH_LIB);
DH_free(dh);
goto err;
}
} else {
goto err;
}

if (pkey != NULL) {
EVP_PKEY_free(*pkey);
*pkey = ret;
}

OPENSSL_free(nm);
OPENSSL_free(data);
return ret;

err:
EVP_PKEY_free(ret);
OPENSSL_free(nm);
OPENSSL_free(data);
return NULL;
}

int PEM_write_bio_Parameters(BIO *bio, EVP_PKEY *pkey) {
if (bio == NULL || pkey == NULL || pkey->ameth == NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the check for pkey->ameth? Should this be a check for pkey->type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, this is a remnant of the original way OpenSSL had implemented these. I tried checking for pkey->type, but it's an int and not a pointer.
The switch statement below should help take care of that.

return 0;
}

// Implementing the ASN1 logic here allows us to decouple the pem logic for
// |EVP_PKEY|. These correspond to the historical |param_encode|
// |EVP_PKEY_ASN1_METHOD| hooks in OpenSSL.
char pem_str[80];
switch (pkey->type) {
case EVP_PKEY_EC:
BIO_snprintf(pem_str, 80, PEM_STRING_ECPARAMETERS);
return PEM_ASN1_write_bio((i2d_of_void *)i2d_ECParameters, pem_str, bio,
pkey->pkey.ec, NULL, NULL, 0, 0, NULL);
case EVP_PKEY_DSA:
BIO_snprintf(pem_str, 80, PEM_STRING_DSAPARAMS);
return PEM_ASN1_write_bio((i2d_of_void *)i2d_DSAparams, pem_str, bio,
pkey->pkey.dsa, NULL, NULL, 0, 0, NULL);
case EVP_PKEY_DH:
BIO_snprintf(pem_str, 80, PEM_STRING_DHPARAMS);
return PEM_ASN1_write_bio((i2d_of_void *)i2d_DHparams, pem_str, bio,
pkey->pkey.dh, NULL, NULL, 0, 0, NULL);
default:
return 0;
}
}

EVP_PKEY *PEM_read_PrivateKey(FILE *fp, EVP_PKEY **x, pem_password_cb *cb,
void *u) {
BIO *b = BIO_new_fp(fp, BIO_NOCLOSE);
Expand Down
101 changes: 101 additions & 0 deletions crypto/pem/pem_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@


#include "../test/test_util.h"
#include "openssl/rand.h"

const char* SECRET = "test";

Expand Down Expand Up @@ -265,3 +266,103 @@ TEST(PEMTest, WriteReadECPKPem) {
ASSERT_TRUE(read_group);
ASSERT_EQ(EC_GROUP_cmp(EC_group_p256(), read_group.get(), nullptr), 0);
}

TEST(ParametersTest, PEMReadwrite) {
// Test |PEM_read/write_bio_Parameters| with |EC_KEY|.
bssl::UniquePtr<EC_KEY> ec_key(EC_KEY_new());
ASSERT_TRUE(ec_key);
bssl::UniquePtr<EC_GROUP> ec_group(EC_GROUP_new_by_curve_name(NID_secp384r1));
ASSERT_TRUE(ec_group);
ASSERT_TRUE(EC_KEY_set_group(ec_key.get(), ec_group.get()));
ASSERT_TRUE(EC_KEY_generate_key(ec_key.get()));

bssl::UniquePtr<BIO> write_bio(BIO_new(BIO_s_mem()));
bssl::UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new());
ASSERT_TRUE(EVP_PKEY_set1_EC_KEY(pkey.get(), ec_key.get()));
EXPECT_TRUE(PEM_write_bio_Parameters(write_bio.get(), pkey.get()));

const uint8_t *content;
size_t content_len;
BIO_mem_contents(write_bio.get(), &content, &content_len);

bssl::UniquePtr<BIO> read_bio(BIO_new_mem_buf(content, content_len));
ASSERT_TRUE(read_bio);
bssl::UniquePtr<EVP_PKEY> pkey_read(
PEM_read_bio_Parameters(read_bio.get(), nullptr));
ASSERT_TRUE(pkey_read);

EC_KEY *pkey_eckey = EVP_PKEY_get0_EC_KEY(pkey.get());
const EC_GROUP *orig_params = EC_KEY_get0_group(pkey_eckey);
const EC_GROUP *read_params = EC_KEY_get0_group(pkey_eckey);
ASSERT_EQ(0, EC_GROUP_cmp(orig_params, read_params, nullptr));

// Test |PEM_read/write_bio_Parameters| with |DH|.
bssl::UniquePtr<BIGNUM> p(BN_get_rfc3526_prime_1536(nullptr));
ASSERT_TRUE(p);
bssl::UniquePtr<BIGNUM> g(BN_new());
ASSERT_TRUE(g);
ASSERT_TRUE(BN_set_u64(g.get(), 2));
bssl::UniquePtr<DH> dh(DH_new());
ASSERT_TRUE(dh);
ASSERT_TRUE(DH_set0_pqg(dh.get(), p.release(), nullptr, g.release()));
write_bio.reset(BIO_new(BIO_s_mem()));
pkey.reset(EVP_PKEY_new());
ASSERT_TRUE(EVP_PKEY_set1_DH(pkey.get(), dh.get()));
EXPECT_TRUE(PEM_write_bio_Parameters(write_bio.get(), pkey.get()));

BIO_mem_contents(write_bio.get(), &content, &content_len);
read_bio.reset(BIO_new_mem_buf(content, content_len));
ASSERT_TRUE(read_bio);
pkey_read.reset(PEM_read_bio_Parameters(read_bio.get(), nullptr));
ASSERT_TRUE(pkey_read);

DH *pkey_dh = EVP_PKEY_get0_DH(pkey.get());
EXPECT_EQ(0, BN_cmp(DH_get0_p(pkey_dh), DH_get0_p(dh.get())));
EXPECT_EQ(0, BN_cmp(DH_get0_g(pkey_dh), DH_get0_g(dh.get())));

// Test |PEM_read/write_bio_Parameters| with |DSA|.
bssl::UniquePtr<DSA> dsa(DSA_new());
ASSERT_TRUE(dsa);
uint8_t seed[20];
ASSERT_TRUE(RAND_bytes(seed, sizeof(seed)));
ASSERT_TRUE(DSA_generate_parameters_ex(dsa.get(), 512, seed, sizeof(seed),
nullptr, nullptr, nullptr));
ASSERT_TRUE(DSA_generate_key(dsa.get()));

write_bio.reset(BIO_new(BIO_s_mem()));
pkey.reset(EVP_PKEY_new());
ASSERT_TRUE(EVP_PKEY_set1_DSA(pkey.get(), dsa.get()));
EXPECT_TRUE(PEM_write_bio_Parameters(write_bio.get(), pkey.get()));

BIO_mem_contents(write_bio.get(), &content, &content_len);
read_bio.reset(BIO_new_mem_buf(content, content_len));
ASSERT_TRUE(read_bio);
pkey_read.reset(PEM_read_bio_Parameters(read_bio.get(), nullptr));
ASSERT_TRUE(pkey_read);

DSA *pkey_dsa = EVP_PKEY_get0_DSA(pkey.get());
EXPECT_EQ(0, BN_cmp(DSA_get0_p(pkey_dsa), DSA_get0_p(dsa.get())));
EXPECT_EQ(0, BN_cmp(DSA_get0_g(pkey_dsa), DSA_get0_g(dsa.get())));
}

const char *kRubyPemDHPARAMETERS =
"-----BEGIN DH PARAMETERS-----\n"
"MIIBCAKCAQEA7E6kBrYiyvmKAMzQ7i8WvwVk9Y/+f8S7sCTN712KkK3cqd1jhJDY"
"JbrYeNV3kUIKhPxWHhObHKpD1R84UpL+s2b55+iMd6GmL7OYmNIT/FccKhTcveab"
"VBmZT86BZKYyf45hUF9FOuUM9xPzuK3Vd8oJQvfYMCd7LPC0taAEljQLR4Edf8E6"
"YoaOffgTf5qxiwkjnlVZQc3whgnEt9FpVMvQ9eknyeGB5KHfayAc3+hUAvI3/Cr3"
"1bNveX5wInh5GDx1FGhKBZ+s1H+aedudCm7sCgRwv8lKWYGiHzObSma8A86KG+MD"
"7Lo5JquQ3DlBodj3IDyPrxIv96lvRPFtAwIBAg==\n"
"-----END DH PARAMETERS-----\n";

TEST(ParametersTest, RubyDHFile) {
bssl::UniquePtr<BIO> read_bio(
BIO_new_mem_buf(kRubyPemDHPARAMETERS, strlen(kRubyPemDHPARAMETERS)));
ASSERT_TRUE(read_bio);
bssl::UniquePtr<EVP_PKEY> pkey_read(
PEM_read_bio_Parameters(read_bio.get(), nullptr));
ASSERT_TRUE(pkey_read);
bssl::UniquePtr<DH> dh(EVP_PKEY_get1_DH(pkey_read.get()));
EXPECT_TRUE(dh);
EXPECT_EQ(DH_num_bits(dh.get()), 2048u);
}
14 changes: 14 additions & 0 deletions include/openssl/pem.h
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ extern "C" {
#define PEM_STRING_ECDSA_PUBLIC "ECDSA PUBLIC KEY"
#define PEM_STRING_ECPARAMETERS "EC PARAMETERS"
#define PEM_STRING_ECPRIVATEKEY "EC PRIVATE KEY"
#define PEM_STRING_PARAMETERS "PARAMETERS"
#define PEM_STRING_CMS "CMS"

// enc_type is one off
Expand Down Expand Up @@ -473,6 +474,19 @@ OPENSSL_EXPORT int PEM_write_PKCS8PrivateKey(FILE *fp, const EVP_PKEY *x,
int klen, pem_password_cb *cd,
void *u);

// PEM_read_bio_Parameters is a generic PEM deserialization function that
// parses the public "parameters" in |bio| and returns a corresponding
// |EVP_PKEY|. If |*pkey| is non-null, the original |*pkey| is freed and the
// returned |EVP_PKEY| is also written to |*pkey|. This is only supported with
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's implied from this statement, but it would be good to explicitly say that *pkey must be initialized (either to NULL or to an allocated value) prior to being passed in. (Passing in an uninitialized pointer would be UB.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added clarification

// |EVP_PKEY_EC|, |EVP_PKEY_DH|, and |EVP_PKEY_DSA|.
OPENSSL_EXPORT EVP_PKEY *PEM_read_bio_Parameters(BIO *bio, EVP_PKEY **pkey);

// PEM_write_bio_Parameters is a generic PEM serialization function that parses
// the public "parameters" of |pkey| to |bio|. It returns 1 on success or 0 on
// failure. This is only supported with |EVP_PKEY_EC|, |EVP_PKEY_DH|, and
// |EVP_PKEY_DSA|.
OPENSSL_EXPORT int PEM_write_bio_Parameters(BIO *bio, EVP_PKEY *pkey);

// PEM_read_bio_ECPKParameters deserializes the PEM file written in |bio|
// according to |ECPKParameters| in RFC 3279. It returns the |EC_GROUP|
// corresponding to deserialized output and also writes it to |out_group|. Only
Expand Down
Loading