Skip to content

Commit

Permalink
Next round of PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@WillChilds-Klein
WillChilds-Klein committed Nov 15, 2024
1 parent 82a0094 commit c1d46a8
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 59 deletions.
100 changes: 45 additions & 55 deletions crypto/pkcs7/pkcs7.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,30 +610,11 @@ void PKCS7_RECIP_INFO_get0_alg(PKCS7_RECIP_INFO *ri, X509_ALGOR **penc) {
}
}

static int PKCS7_type_is_other(const PKCS7 *p7) {
GUARD_PTR(p7);
switch (OBJ_obj2nid(p7->type)) {
case NID_pkcs7_data:
case NID_pkcs7_signed:
case NID_pkcs7_enveloped:
case NID_pkcs7_signedAndEnveloped:
case NID_pkcs7_digest:
case NID_pkcs7_encrypted:
return 0;
default:
return 1;
}
}

static ASN1_OCTET_STRING *PKCS7_get_octet_string(PKCS7 *p7) {
GUARD_PTR(p7);
if (PKCS7_type_is_data(p7)) {
return p7->d.data;
}
if (PKCS7_type_is_other(p7) && p7->d.other &&
(p7->d.other->type == V_ASN1_OCTET_STRING)) {
return p7->d.other->value.octet_string;
}
return NULL;
}

Expand Down Expand Up @@ -688,24 +669,10 @@ static int pkcs7_encode_rinfo(PKCS7_RECIP_INFO *ri, unsigned char *key,
}

pctx = EVP_PKEY_CTX_new(pkey, NULL);
if (pctx == NULL) {
goto err;
}

if (EVP_PKEY_encrypt_init(pctx) <= 0) {
goto err;
}

if (EVP_PKEY_encrypt(pctx, NULL, &eklen, key, keylen) <= 0) {
goto err;
}

ek = OPENSSL_malloc(eklen);
if (ek == NULL) {
goto err;
}

if (EVP_PKEY_encrypt(pctx, ek, &eklen, key, keylen) <= 0) {
if (pctx == NULL || EVP_PKEY_encrypt_init(pctx) <= 0 ||
EVP_PKEY_encrypt(pctx, NULL, &eklen, key, keylen) <= 0 ||
(NULL == (ek = OPENSSL_malloc(eklen))) ||
EVP_PKEY_encrypt(pctx, ek, &eklen, key, keylen) <= 0) {
goto err;
}

Expand Down Expand Up @@ -850,9 +817,13 @@ BIO *PKCS7_dataInit(PKCS7 *p7, BIO *bio) {
goto err;
}
BIO_set_mem_eof_return(bio, /*eof_value*/ 0);
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format off
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format on
if (!PKCS7_is_detached(p7) && content && content->length > 0) {
OPENSSL_END_ALLOW_DEPRECATED
// clang-format off
OPENSSL_END_ALLOW_DEPRECATED
// clang-format on
// |bio |needs a copy of |os->data| instead of a pointer because the data
// will be used after |os |has been freed
if (BIO_write(bio, content->data, content->length) != content->length) {
Expand All @@ -873,9 +844,13 @@ BIO *PKCS7_dataInit(PKCS7 *p7, BIO *bio) {
return NULL;
}

// clang-format off
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format on
int PKCS7_is_detached(PKCS7 *p7) {
OPENSSL_END_ALLOW_DEPRECATED
// clang-format off
OPENSSL_END_ALLOW_DEPRECATED
// clang-format on
GUARD_PTR(p7);
if (PKCS7_type_is_signed(p7)) {
return (p7->d.sign == NULL || p7->d.sign->contents->d.ptr == NULL);
Expand Down Expand Up @@ -905,6 +880,8 @@ static BIO *PKCS7_find_digest(EVP_MD_CTX **pmd, BIO *bio, int nid) {
}

int PKCS7_set_digest(PKCS7 *p7, const EVP_MD *md) {
GUARD_PTR(p7);
GUARD_PTR(md);
switch (OBJ_obj2nid(p7->type)) {
case NID_pkcs7_digest:
if (EVP_MD_nid(md) == NID_undef) {
Expand Down Expand Up @@ -992,13 +969,21 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) {
break;
case NID_pkcs7_signed:
si_sk = p7->d.sign->signer_info;
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format off
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format on
content = PKCS7_get_octet_string(p7->d.sign->contents);
OPENSSL_END_ALLOW_DEPRECATED
// clang-format off
OPENSSL_END_ALLOW_DEPRECATED
// clang-format on
// If detached data then the content is excluded
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format off
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format on
if (PKCS7_type_is_data(p7->d.sign->contents) && PKCS7_is_detached(p7)) {
OPENSSL_END_ALLOW_DEPRECATED
// clang-format off
OPENSSL_END_ALLOW_DEPRECATED
// clang-format on
ASN1_OCTET_STRING_free(content);
content = NULL;
p7->d.sign->contents->d.data = NULL;
Expand All @@ -1008,9 +993,13 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) {
case NID_pkcs7_digest:
content = PKCS7_get_octet_string(p7->d.digest->contents);
// If detached data, then the content is excluded
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format off
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format on
if (PKCS7_type_is_data(p7->d.digest->contents) && PKCS7_is_detached(p7)) {
OPENSSL_END_ALLOW_DEPRECATED
// clang-format off
OPENSSL_END_ALLOW_DEPRECATED
// clang-format on
ASN1_OCTET_STRING_free(content);
content = NULL;
p7->d.digest->contents->d.data = NULL;
Expand Down Expand Up @@ -1047,6 +1036,7 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) {
if (abuflen == 0 || (abuf = OPENSSL_malloc(abuflen)) == NULL) {
goto err;
}
// TODO test sign failure case (maybe bad sig alg or params?)
if (!EVP_SignFinal(md_ctx_tmp, abuf, &abuflen, si->pkey)) {
OPENSSL_free(abuf);
OPENSSL_PUT_ERROR(PKCS7, ERR_R_EVP_LIB);
Expand All @@ -1057,20 +1047,20 @@ int PKCS7_dataFinal(PKCS7 *p7, BIO *bio) {
} else if (OBJ_obj2nid(p7->type) == NID_pkcs7_digest) {
unsigned char md_data[EVP_MAX_MD_SIZE];
unsigned int md_len;
if (!PKCS7_find_digest(&md_ctx, bio, EVP_MD_nid(p7->d.digest->md))) {
goto err;
}
if (!EVP_DigestFinal_ex(md_ctx, md_data, &md_len)) {
goto err;
}
if (!ASN1_OCTET_STRING_set(p7->d.digest->digest, md_data, md_len)) {
if (!PKCS7_find_digest(&md_ctx, bio, EVP_MD_nid(p7->d.digest->md)) ||
!EVP_DigestFinal_ex(md_ctx, md_data, &md_len) ||
!ASN1_OCTET_STRING_set(p7->d.digest->digest, md_data, md_len)) {
goto err;
}
}

OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format off
OPENSSL_BEGIN_ALLOW_DEPRECATED
// clang-format on
if (!PKCS7_is_detached(p7)) {
OPENSSL_END_ALLOW_DEPRECATED
// clang-format off
OPENSSL_END_ALLOW_DEPRECATED
// clang-format on
if (content == NULL) {
goto err;
}
Expand Down
2 changes: 1 addition & 1 deletion crypto/pkcs7/pkcs7_asn1.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include "../internal.h"
#include "internal.h"

ASN1_ADB_TEMPLATE(p7default) = ASN1_EXP_OPT(PKCS7, d.other, ASN1_ANY, 0);
ASN1_ADB_TEMPLATE(p7default) = ASN1_EXP_OPT(PKCS7, d.data, ASN1_ANY, 0);

ASN1_ADB(PKCS7) = {
ADB_ENTRY(NID_pkcs7_data,
Expand Down
38 changes: 36 additions & 2 deletions crypto/pkcs7/pkcs7_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1399,6 +1399,8 @@ TEST(PKCS7Test, GettersSetters) {
EXPECT_TRUE(PKCS7_set_cipher(p7.get(), EVP_aes_128_gcm()));
EXPECT_FALSE(PKCS7_set_content(p7.get(), p7.get()));

// Ruby requires that we can set type NID_pkcs7_encrypted even though we don't
// really support it for most functions.
p7.reset(PKCS7_new());
ASSERT_TRUE(p7.get());
EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_encrypted));
Expand Down Expand Up @@ -1573,7 +1575,7 @@ TEST(PKCS7Test, GettersSetters) {

TEST(PKCS7Test, DataInitFinal) {
bssl::UniquePtr<PKCS7> p7;
bssl::UniquePtr<BIO> bio;
bssl::UniquePtr<BIO> bio, bio_in;
bssl::UniquePtr<STACK_OF(X509)> certs;
bssl::UniquePtr<X509> rsa_x509;
bssl::UniquePtr<uint8_t> p7_der;
Expand All @@ -1586,7 +1588,7 @@ TEST(PKCS7Test, DataInitFinal) {
ASSERT_TRUE(p7);
EXPECT_TRUE(PKCS7_type_is_signed(p7.get()));
bio.reset(PKCS7_dataInit(p7.get(), nullptr));
EXPECT_TRUE(bio);
ASSERT_TRUE(bio);
EXPECT_TRUE(PKCS7_dataFinal(p7.get(), bio.get()));

// parse a cert for use with recipient infos
Expand Down Expand Up @@ -1654,4 +1656,36 @@ TEST(PKCS7Test, DataInitFinal) {
bio.reset(PKCS7_dataInit(p7.get(), nullptr));
EXPECT_TRUE(bio);
EXPECT_TRUE(PKCS7_dataFinal(p7.get(), bio.get()));

// pre-existing BIO?
p7.reset(PKCS7_new());
ASSERT_TRUE(p7);
ASSERT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_enveloped));
ASSERT_TRUE(PKCS7_set_cipher(p7.get(), EVP_aes_128_ctr()));
bio.reset(PKCS7_dataInit(p7.get(), nullptr));
EXPECT_TRUE(bio);
EXPECT_TRUE(PKCS7_dataFinal(p7.get(), bio.get()));

// Error cases

// type NID_pkcs7_encrypted is not supported by the BIO functions
p7.reset(PKCS7_new());
bio_in.reset(BIO_new(BIO_s_mem()));
ASSERT_TRUE(p7);
ASSERT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_encrypted));
bio.reset(PKCS7_dataInit(p7.get(), bio_in.get()));
EXPECT_FALSE(bio);
EXPECT_FALSE(PKCS7_dataFinal(p7.get(), bio.get()));

// NID_pkcs7_enveloped and NID_pkcs7_signedAndEnveloped require a cipher
p7.reset(PKCS7_new());
ASSERT_TRUE(p7);
ASSERT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_enveloped));
bio.reset(PKCS7_dataInit(p7.get(), nullptr));
EXPECT_FALSE(bio);
EXPECT_FALSE(PKCS7_dataFinal(p7.get(), bio.get()));
ASSERT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_enveloped));
bio.reset(PKCS7_dataInit(p7.get(), nullptr));
EXPECT_FALSE(bio);
EXPECT_FALSE(PKCS7_dataFinal(p7.get(), bio.get()));
}
1 change: 0 additions & 1 deletion include/openssl/pkcs7.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ struct pkcs7_st {
PKCS7_SIGN_ENVELOPE *signed_and_enveloped;
PKCS7_DIGEST *digest;
PKCS7_ENCRYPT *encrypted;
ASN1_TYPE *other;
} d;
};

Expand Down

0 comments on commit c1d46a8

Please sign in to comment.