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

Remove custom PKCS7 ASN1 functions, add new structs #1726

Merged
merged 8 commits into from
Aug 28, 2024

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Jul 29, 2024

Issues:

Resolves #CryptoAlg-2493

Description of changes:

This PR is the first in a series implementing Ruby's supported subset of the PKCS7 standard. To do this, we remove AWS-LC's customized ASN.1 serialization logic and delegate to asn1.h and asn1t.h. We also update the various PKCS_type_is_* no-op functions to actually check the input's type.

The PKCS7 tests currently contain a test signedData structure that is encoded using BER. Because our ASN1_get_object currently disallows indefinite-length BER, we need to modify d2i_PKCS7 to detect and convert BER (if present) into DER before parsing. This allows us to retain backwards compatibility with BER-encoded PKCS7 objects. Please see the appendix for more discussion on this topic.

The next PR in this series will implement the various getters, setters, and allocation functions needed to support CRruby with testing for each. Please see PR #1780 for an idea of what that will look like.

Call-outs:

Source edits conform to OpenSSL's implementation, inlining some of OpenSSL's common subroutines.

Prior struct definitions in public pkcs7.h are preserved for backwards compatibility and expanded for the rest of this series. Whether we can make these existing definitions opaque should be a point of discussion. New structure definitions are made opaque.

Also, because AWS-LC previously implemented BER/DER parsing semi-manually and cached the encoded value in PKCS7 structs, there may exist performance differences due to the new lack of caching. Similarly, BER parsing is now slightly less efficient because we need to make 2 passes over the input (once to convert from BER to DER, once to parse the DER).

Testing:


Appendix: Indefinite-length BER in PKCS7

TL;DR -- Reading RFC 2315, it looks like encrypted is the only content type should be affected, but the RFC imposes no restrictions around how the top-level ContentInfo SEQUENCE shall be encoded.

Support for indefinite-length BER was removed (see also here) in the general ASN.1 parser macros (although BoringSSL has retained support for it in their minimal PKCS7 implementation, as it used the CBS API for parsing).

Per section 5 of RFC 2315 defining PKCS7:

   The syntax is general enough to support many different content types.
   This document defines six: data, signed data, enveloped data,
   signed-and-enveloped data, digested data, and encrypted data. ...

   There are two classes of content types: base and enhanced.  Content
   types in the base class contain "just data," with no cryptographic
   enhancements. Presently, one content type is in this class, the data
   content type. ...

   The document is designed such that the enhanced content types can be
   prepared in a single pass using indefinite-length BER encoding, and
   processed in a single pass in any BER encoding. Single-pass operation
   is especially helpful if content is stored on tapes, or is "piped"
   from another process. One of the drawbacks of single-pass operation,
   however, is that it is difficult to output a DER encoding in a single
   pass, since the lengths of the various components may not be known in
   advance. Since DER encoding is required by the signed-data, signed-
   and-enveloped data, and digested-data content types, an extra pass
   may be necessary when a content type other than data is the inner
   content of one of those content types.

Since indefinite length is forbidden in DER and data is not an enhanced content type, the last paragraph excerpted above implies that only enveloped and encrypted content types can utilize indefinite BER. However, section 7 states:

        2.   When a ContentInfo value is the inner content of
             signed-data, signed-and-enveloped-data, or digested-data
             content, a message-digest algorithm is applied to the
             contents octets of the DER encoding of the content field.
             When a ContentInfo value is the inner content of
             enveloped-data or signed-and-enveloped-data content, a
             content-encryption algorithm is applied to the contents
             octets of a definite-length BER encoding of the content
             field.

So, it would seem that the only content type that can be encoded in indefinite length BER is the encrypted type.

Interestingly, the NSS certificate previously included in our PKCS7 tests contains a signed content encoded in indefinite-length BER (note that length value 0x80 indicates indefinite length).

Screenshot 2024-07-30 at 1 05 29 PM

Unfortunately, this appears to be a valid PKCS7 object per the RFC because the RFC only imposes definite-length encoding on the contents that are operated on cryptographically, not the top-level structures themselves.


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.

@WillChilds-Klein WillChilds-Klein force-pushed the pkcs7-rm-custom-asn1 branch 2 times, most recently from 185bac3 to de74723 Compare July 29, 2024 22:04
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2024

Codecov Report

Attention: Patch coverage is 51.47059% with 33 lines in your changes missing coverage. Please review.

Project coverage is 78.36%. Comparing base (353228b) to head (578d967).

Files Patch % Lines
crypto/pkcs7/pkcs7_asn1.c 58.53% 17 Missing ⚠️
crypto/pkcs7/pkcs7_x509.c 20.00% 16 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1726      +/-   ##
==========================================
+ Coverage   78.34%   78.36%   +0.01%     
==========================================
  Files         581      582       +1     
  Lines       97343    97330      -13     
  Branches    13960    13951       -9     
==========================================
+ Hits        76268    76273       +5     
+ Misses      20455    20436      -19     
- Partials      620      621       +1     

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

@WillChilds-Klein WillChilds-Klein force-pushed the pkcs7-rm-custom-asn1 branch 2 times, most recently from 8906a7f to 40014a2 Compare July 30, 2024 22:13
@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review July 30, 2024 22:13
@WillChilds-Klein WillChilds-Klein requested a review from a team as a code owner July 30, 2024 22:13
@WillChilds-Klein WillChilds-Klein marked this pull request as draft August 1, 2024 14:58
@WillChilds-Klein WillChilds-Klein force-pushed the pkcs7-rm-custom-asn1 branch 2 times, most recently from c97b588 to 42a8be5 Compare August 20, 2024 16:46
@WillChilds-Klein WillChilds-Klein marked this pull request as ready for review August 20, 2024 17:20
Copy link
Contributor

@samuel40791765 samuel40791765 left a comment

Choose a reason for hiding this comment

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

Really like the way of handling the backwards compatibility retainment with BER-encoded PKCS7 objects

include/openssl/pkcs7.h Outdated Show resolved Hide resolved
include/openssl/base.h Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7_x509.c Outdated Show resolved Hide resolved
crypto/pkcs7/pkcs7_asn1.c Show resolved Hide resolved
crypto/pkcs7/pkcs7_asn1.c Outdated Show resolved Hide resolved
crypto/pkcs7/internal.h Outdated Show resolved Hide resolved
crypto/pkcs7/internal.h Outdated Show resolved Hide resolved
samuel40791765
samuel40791765 previously approved these changes Aug 27, 2024
include/openssl/pkcs7.h Outdated Show resolved Hide resolved
samuel40791765
samuel40791765 previously approved these changes Aug 27, 2024
crypto/pkcs7/pkcs7_asn1.c Show resolved Hide resolved
Comment on lines +12 to +22
ASN1_ADB(PKCS7) = {
ADB_ENTRY(NID_pkcs7_data, ASN1_EXP_OPT(PKCS7, d.data, ASN1_OCTET_STRING, 0)),
ADB_ENTRY(NID_pkcs7_signed, ASN1_EXP_OPT(PKCS7, d.sign, PKCS7_SIGNED, 0)),
ADB_ENTRY(NID_pkcs7_enveloped, ASN1_EXP_OPT(PKCS7, d.enveloped, PKCS7_ENVELOPE, 0)),
ADB_ENTRY(NID_pkcs7_signedAndEnveloped, ASN1_EXP_OPT(PKCS7, d.signed_and_enveloped, PKCS7_SIGN_ENVELOPE, 0)),
ADB_ENTRY(NID_pkcs7_digest, ASN1_EXP_OPT(PKCS7, d.digest, PKCS7_DIGEST, 0)),
ADB_ENTRY(NID_pkcs7_encrypted, ASN1_EXP_OPT(PKCS7, d.encrypted, PKCS7_ENCRYPT, 0))
} ASN1_ADB_END(PKCS7, 0, type, 0, &p7default_tt, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is ADB and what is this doing? How is this different than the ASN1_SEQUENCE definitions down below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to use a rough C analogy, it's kind of like a union (PKCS7) of structs (PKCS7_SIGNED, other SEQUENCEs). ADB stands for ANY DEFINED BY, which is like ASN.1's ANY type, but restricted to a defined subset of possible types. there's a good explanation in sections 5.2 and 5.3 here.

crypto/pkcs7/internal.h Show resolved Hide resolved
@WillChilds-Klein WillChilds-Klein merged commit 621bceb into aws:main Aug 28, 2024
107 of 108 checks passed
@WillChilds-Klein WillChilds-Klein deleted the pkcs7-rm-custom-asn1 branch August 28, 2024 17:42
smittals2 added a commit that referenced this pull request Sep 17, 2024
## What's Changed
* Use OPENSSL_STATIC_ASSERT which handles all the platform/compiler/C s…
by @andrewhop in #1791
* ML-KEM refactor by @dkostic in #1763
* ML-KEM-IPD to ML-KEM as defined in FIPS 203 by @dkostic in
#1796
* Add KDA OneStep testing to ACVP by @skmcgrail in
#1792
* Updating erroneous documentation for BIO_get_mem_data and subsequent
usage by @smittals2 in #1752
* No-op impls for several EVP_PKEY_CTX functions by @justsmth in
#1759
* Drop "ipd" suffix from ML-KEM related code by @dkostic in
#1797
* Upstream merge 2024 08 19 by @skmcgrail in
#1781
* ML-KEM move to the FIPS module by @dkostic in
#1802
* Reduce collision probability for variable names by @torben-hansen in
#1804
* Refactor ENGINE API and memory around METHOD structs by @smittals2 in
#1776
* bn: Move x86-64 argument-based dispatching of bn_mul_mont to C. by
@justsmth in #1795
* Check at runtime that the tool is loading the same libcrypto it was
built with by @andrewhop in #1716
* Avoid matching prefixes of a symbol as arm registers by @torben-hansen
in #1807
* Add CI for FreeBSD by @justsmth in
#1787
* Move curve25519 implementations to fips module except spake25519 by
@torben-hansen in #1809
* Add CAST for SP 800-56Cr2 One-Step function by @skmcgrail in
#1803
* Remove custom PKCS7 ASN1 functions, add new structs by
@WillChilds-Klein in #1726
* NASM use default debug format by @justsmth in
#1747
* Add KDF in counter mode ACVP Testing by @skmcgrail in
#1810
* add support for OCSP_request_verify by @samuel40791765 in
#1778
* Fix GitHub/CodeBuild Purge Lambda by @justsmth in
#1808
* KBKDF_ctr_hmac FIPS Service Indicator by @skmcgrail in
#1798
* Update x509 tool to write all output to common BIO which is a file or
stdout by @andrewhop in #1800
* Add ML-KEM to speed.cc, bump AWSLC_API_VERSION to 30 by @andrewhop in
#1817
* Add EVP_PKEY_asn1_* functions by @justsmth in
#1751
* Improve portability of CI integration script by @torben-hansen in
#1815
* Upstream merge 2024 08 23 by @justsmth in
#1799
* Replace ECDSA_METHOD with EC_KEY_METHOD and add the associated API by
@smittals2 in #1785
* Cherrypick "Add some barebones support for DH in EVP" by
@samuel40791765 in #1813
* Add KDA OneStep (SSKDF_digest and SSKDF_hmac) to FIPS indicator by
@skmcgrail in #1793
* Add EVP_Digest one-shot test XOFs by @WillChilds-Klein in
#1820
* Wire-up ACVP Testing for SHA3 Signatures with RSA by @skmcgrail in
#1805
* Make SHA3 (not SHAKE) Approved for EVP_DigestSign/Verify, RSA and
ECDSA. by @nebeid in #1821
* Begin tracking RelWithDebInfo library statistics by @andrewhop in
#1822
* Move EVP ed25519 function table under FIPS module by @torben-hansen in
#1826
* Avoid C11 Atomics on Windows by @justsmth in
#1824
* Improve pre-sandbox setup by @torben-hansen in
#1825
* Add OCSP round trip integration test with minor fixes by
@samuel40791765 in #1811
* Add various PKCS7 getters and setters by @WillChilds-Klein in
#1780
* Run clang-format on pkcs7 code by @WillChilds-Klein in
#1830
* Move KEM API and ML-KEM definitions to FIPS module by @torben-hansen
in #1828
* fix socat integration CI by @samuel40791765 in
#1833
* Retire out-of-module KEM folder by @torben-hansen in
#1832
* Refactor RSA_METHOD and expand API by @smittals2 in
#1790
* Update benchmark documentation in tool/readme.md by @andrewhop in
#1812
* Pre jail unit test by @torben-hansen in
#1835
* Move EVP KEM implementation to in-module and correct OID by
@torben-hansen in #1838
* More minor symbols Ruby depends on by @samuel40791765 in
#1837
* ED25519 Power-on Self Test / CAST / KAT by @skmcgrail in
#1834
* ACVP ML-KEM testing by @skmcgrail in
#1840
* ACVP ECDSA SHA3 Digest Testing by @skmcgrail in
#1819
* ML-KEM Service Indicator for EVP_PKEY_keygen, EVP_PKEY_encapsulate,
EVP_PKEY_decapsulate by @skmcgrail in
#1844
* Add ML-KEM CAST for KeyGen, Encaps, and Decaps by @skmcgrail in
#1846
* ED25519 Service Indicator by @skmcgrail in
#1829
* Update Allowed RSA KeySize Generation to FIPS 186-5 specification by
@skmcgrail in #1823
* Add ED25519 ACVP Testing by @skmcgrail in
#1818
* Make EDDSA/Ed25519 POST lazy initalized by @skmcgrail in
#1848
* add support for PEM Parameters without ASN1 hooks by @samuel40791765
in #1831
* Add OpenVPN tip of main to CI by @smittals2 in
#1843
* Ensure SSE2 is enabled when using optimized assembly for 32-bit x86 by
@graebm in #1841
* Add support for `EVP_PKEY_CTX_ctrl_str` - Step #1 by @justsmth in
#1842
* Added SHA3/SHAKE XOF functionality by @jakemas in
#1839
* Migrated ML-KEM SHA3/SHAKE usage to fipsmodule by @jakemas in
#1851
* AVX-512 support for RSA Signing by @pittma in
#1273
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants