Support other field for PKCS7

[samuel40791765]

Issues:

Addresses CryptoAlg-3279

Description of changes:

OpenSSL's PKCS7 structure has historically been non-opaque. This causes issues like projects depending on the PKCS7 internals to be of a certain format. OpenSSL's PKCS7 struct has a field called other, which various projects internally have found to have a dependence on. The usage spans multiple projects and seeing that OpenSSL 3.x still supports the field, this isn't going away anytime soon.

examples:

• https://github.com/tianocore/edk2/blob/686f1e3ea8373db48ea3a8457cc758c986455de0/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7VerifyBase.c#L69-L71
• https://github.com/krb5/krb5/blob/820a7277ed1c553d33b2a7f3efb48655fb993b77/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c#L1615-L1636

Call-outs:

N/A

Testing:

New tests

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.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.71%. Comparing base (04875db) to head (ca6003b).

Files with missing lines	Patch %	Lines
crypto/pkcs7/pkcs7_test.cc	80.76%	3 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2603      +/-   ##
==========================================
- Coverage   78.72%   78.71%   -0.02%     
==========================================
  Files         645      645              
  Lines      111086   111109      +23     
  Branches    15690    15688       -2     
==========================================
+ Hits        87453    87456       +3     
- Misses      22941    22961      +20     
  Partials      692      692              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[justsmth]

Can we also seed our "pkcs7 verify" fuzz test with some example inputs that use the new "other" field?

[samuel40791765]

Can we also seed our "pkcs7 verify" fuzz test with some example inputs that use the new "other" field?

I'll try seeing if I can put this in the same PR.

[WillChilds-Klein]

why do we diverge from what OpenSSL does here?

[samuel40791765]

This was the key to the memory leak I had to resolve. The last field in ASN1_ADB_END points to null_tt within the ASN1_ADB_st structure.

aws-lc/include/openssl/asn1t.h

Lines 374 to 375 in 04875db

	const ASN1_TEMPLATE *default_tt; /* Type to use if no match */
	const ASN1_TEMPLATE *null_tt; /* Type to use if selector is NULL */

type is the selector field for the ASN1 structure. default_tt returns the designated default type for the selector field, while null_tt returns the type to return if the selector field is NULL.

I was testing against a circumstance where only d.other had been designated a value, but where type is NULL. Interestingly enough, this causes a memory leak once PKCS7 goes out of scope in AWS-LC. I pinned the reason to us returning NULL for the selector field (since type hasn't been defined) and our ASN1 macros think that nothing has to be freed here. Said ASN1 code is used below:

• https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/asn1/tasn_utl.c#L209-L213
• https://github.com/openssl/openssl/blob/OpenSSL_1_1_1-stable/crypto/asn1/tasn_fre.c#L106-L110

This doesn't happen in OpenSSL for some reason. type seems to already have a value assigned and cases default to default_tt to free all related memory. I don't necessarily think this is the "correct behavior" though, it's a bit sketchy since we haven't really initialized the type field. My assumption is that the different behavior might have something to do with us calling OpenSSL_zalloc in more places and most undefined fields are actually NULL.

Changing null_tt to a proper default value allows us to properly free values in d.other, even if type has no value assigned to it (like OpenSSL). There won't be double free issues, mainly because OpenSSL_free already does NULL check for us. I added a test called TEST(PKCS7Test, OtherFieldMemoryLeak) mainly to test this behavior.

[WillChilds-Klein]

I pinned the reason to us returning NULL for the selector field (since type hasn't been defined) and our ASN1 macros think that nothing has to be freed here.

that makes sense.

This doesn't happen in OpenSSL for some reason.

perhaps it does, and there isn't similar valgrind CI?

anyway, changes look good to me.

[samuel40791765]

I ran a mini test with OpenSSL locally with valgrind and verified that there was no memory leakage with OpenSSL. I dove deeper and found that reason being type having a non-NULL value and ASN1 cases default to default_tt to free the memory because of that. In AWS-LC, we default to null_tt instead since uninitialized memory is all NULL.