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.

Sign up for GitHub

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 PKCS7_set/get_detached #2134

Merged
merged 2 commits into from
Jan 31, 2025
+63 −0
Merged

Add support for PKCS7_set/get_detached #2134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9626e83
Add support for PKCS7_set/get_detached
samuel40791765 Jan 23, 2025
a712d2c
More tests
samuel40791765 Jan 24, 2025
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
27 changes: 27 additions & 0 deletions crypto/pkcs7/pkcs7.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,33 @@ int PKCS7_is_detached(PKCS7 *p7) {
return 0;
}

int PKCS7_set_detached(PKCS7 *p7, int detach) {
GUARD_PTR(p7);
if (detach != 0 && detach != 1) {
// |detach| is meant to be used as a boolean int.
return 0;
}

if (PKCS7_type_is_signed(p7)) {
if (p7->d.sign == NULL) {
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_NO_CONTENT);
return 0;
}
if (detach && PKCS7_type_is_data(p7->d.sign->contents)) {
ASN1_OCTET_STRING_free(p7->d.sign->contents->d.data);
p7->d.sign->contents->d.data = NULL;
}
return detach;
Copy link
Contributor

Choose a reason for hiding this comment

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

The semantic of the detach parameter is unexpected. The call always "fails" (returns 0) when detach = 0.

Could we detect detach == 0 and return 0 at the top? (The difference would be that calls to OPENSSL_PUT_ERROR in certain situations would not be made.)

if (detach != 1) {
    return 0;
}

If the calls to OPENSSL_PUT_ERROR are important, we should have test coverage to confirm they are made.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The detach parameter is expected to be consumed as a boolean. Ruby has a good example of the usage here: https://github.com/ruby/ruby/blob/dd863714bf377b044645ea12b4db48920d49694e/ext/openssl/ossl_pkcs7.c#L497-L500

I completely agree with the weird semantic of the detach parameter, especially with 0. OpenSSL's implementation allows for other numbers to be used, but seeing that it doesn't align with how the API was meant to be used, I decided to restrict the values to only 0 and 1.
Since the parameter value was meant to take a boolean integer, I do think 0 should be allowed as a value though. I'll add coverage for the unsupported operations.

} else {
OPENSSL_PUT_ERROR(PKCS7, PKCS7_R_OPERATION_NOT_SUPPORTED_ON_THIS_TYPE);
return 0;
}
}

int PKCS7_get_detached(PKCS7 *p7) {
return PKCS7_is_detached(p7);
}


static BIO *pkcs7_find_digest(EVP_MD_CTX **pmd, BIO *bio, int nid) {
GUARD_PTR(pmd);
Expand Down
25 changes: 25 additions & 0 deletions crypto/pkcs7/pkcs7_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2043,3 +2043,28 @@ TEST(PKCS7Test, PKCS7PrintNoop) {
ASSERT_TRUE(BIO_mem_contents(bio.get(), &contents, &len));
EXPECT_EQ(Bytes(contents, len), Bytes("PKCS7 printing is not supported"));
}

TEST(PKCS7Test, SetDetached) {
bssl::UniquePtr<PKCS7> p7(PKCS7_new());
// |PKCS7_set_detached| does not work on an uninitialized |PKCS7|.
EXPECT_FALSE(PKCS7_set_detached(p7.get(), 0));
EXPECT_FALSE(PKCS7_set_detached(p7.get(), 1));
EXPECT_TRUE(PKCS7_set_type(p7.get(), NID_pkcs7_signed));
EXPECT_TRUE(PKCS7_type_is_signed(p7.get()));

PKCS7 *p7_internal = PKCS7_new();
EXPECT_TRUE(PKCS7_set_type(p7_internal, NID_pkcs7_data));
EXPECT_TRUE(PKCS7_type_is_data(p7_internal));
EXPECT_TRUE(PKCS7_set_content(p7.get(), p7_internal));

// Access the |p7|'s internal contents to verify that |PKCS7_set_detached|
// has the right behavior.
EXPECT_TRUE(p7.get()->d.sign->contents->d.data);
EXPECT_FALSE(PKCS7_set_detached(p7.get(), 0));
EXPECT_TRUE(p7.get()->d.sign->contents->d.data);
EXPECT_FALSE(PKCS7_set_detached(p7.get(), 2));
EXPECT_TRUE(p7.get()->d.sign->contents->d.data);
// data is "detached" when |PKCS7_set_detached| is set with 1.
EXPECT_TRUE(PKCS7_set_detached(p7.get(), 1));
EXPECT_FALSE(p7.get()->d.sign->contents->d.data);
samuel40791765 marked this conversation as resolved.
Show resolved Hide resolved
}
11 changes: 11 additions & 0 deletions include/openssl/pkcs7.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,16 @@ OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_verify(PKCS7 *p7,
// PKCS7_is_detached returns 0 if |p7| has attached content and 1 otherwise.
OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_is_detached(PKCS7 *p7);

// PKCS7_set_detached frees the attached content of |p7| if |detach| is set to
// 1. It returns 0 if otherwise or if |p7| is not of type signed.
//
// Note: |detach| is intended to be a boolean and MUST be set with either 1 or
// 0.
OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_set_detached(PKCS7 *p7, int detach);

// PKCS7_get_detached returns 0 if |p7| has attached content and 1 otherwise.
OPENSSL_EXPORT OPENSSL_DEPRECATED int PKCS7_get_detached(PKCS7 *p7);

// PKCS7_dataInit creates or initializes a BIO chain for reading data from or
// writing data to |p7|. If |bio| is non-null, it is added to the chain.
// Otherwise, a new BIO is allocated and returned to anchor the chain.
Expand Down Expand Up @@ -576,5 +586,6 @@ BSSL_NAMESPACE_END
#define PKCS7_R_PKCS7_ADD_SIGNATURE_ERROR 132
#define PKCS7_R_NO_DEFAULT_DIGEST 133
#define PKCS7_R_CERT_MUST_BE_RSA 134
#define PKCS7_R_OPERATION_NOT_SUPPORTED_ON_THIS_TYPE 135

#endif // OPENSSL_HEADER_PKCS7_H
Loading