Fix ASN.1 issues in PKCS#7 and S/MIME signing by facutuesca · Pull Request #10373 · pyca/cryptography

facutuesca · 2024-02-09T17:48:33Z

This PR fixes two issues with the ASN.1 generated when signing using PKCS#7:

First issue

The current implementation defines the SMIMECapabilities attribute so that its value is a SEQUENCE of all the algorithm OIDs that are supported. This is what the ASN.1 currently looks like for a signed message using cryptography:

  U.P.SEQUENCE {
     U.P.OBJECTIDENTIFIER 1.2.840.113549.1.9.15 (S/MIME capabilities)
     U.P.SET {
        U.P.SEQUENCE {
           U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.42 (aes256-CBC-PAD)
           U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.22 (aes192-CBC-PAD)
           U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.2 (aes128-CBC-PAD)
        }
     }
  }

However, the S/MIME v3 spec (RFC 2633) specifies that each algorithm should be specified in its own SEQUENCE:

SMIMECapabilities ::= SEQUENCE OF SMIMECapability

SMIMECapability ::= SEQUENCE {
   capabilityID OBJECT IDENTIFIER,
   parameters ANY DEFINED BY capabilityID OPTIONAL }

(RFC 2633, Appendix A)

The spec matches what OpenSSL outputs (using openssl smime -sign):

  U.P.SEQUENCE {
     U.P.OBJECTIDENTIFIER 1.2.840.113549.1.9.15 (S/MIME capabilities)
     U.P.SET {
        U.P.SEQUENCE {
           U.P.SEQUENCE {
              U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.42 (aes256-CBC-PAD)
           }
           U.P.SEQUENCE {
              U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.22 (aes192-CBC-PAD)
           }
           U.P.SEQUENCE {
              U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.2 (aes128-CBC-PAD)
           }
           U.P.SEQUENCE {
              U.P.OBJECTIDENTIFIER 1.2.840.113549.3.7 (id_des_EDE3_CBC)
           }
          .....

This PR changes the implementation so that each algorithm is inside its own SEQUENCE. After the changes in this PR, the ASN.1 output looks like:

  U.P.SEQUENCE {
     U.P.OBJECTIDENTIFIER 1.2.840.113549.1.9.15 (S/MIME capabilities)
     U.P.SET {
        U.P.SEQUENCE {
           U.P.SEQUENCE {
              U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.42 (aes256-CBC-PAD)
           }
           U.P.SEQUENCE {
              U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.22 (aes192-CBC-PAD)
           }
           U.P.SEQUENCE {
              U.P.OBJECTIDENTIFIER 2.16.840.1.101.3.4.1.2 (aes128-CBC-PAD)
           }
        }
     }
  }

Second issue

There is an issue with the RSA OID used for signing PKCS#7/SMIME, in the SignatureAlgorithmIdentifier field.

The current implementation computes the algorithm identifier used in the digest_encryption_algorithm PKCS#7 field
(or SignatureAlgorithmIdentifier in S/MIME) based on both the algorithm used to sign (e.g. RSA) and the digest algorithm (e.g. SHA512).

This is correct for ECDSA signatures, where the OIDs used include the digest algorithm (e.g: ecdsa-with-SHA512). However, due to historical reasons, when signing with RSA the OID specified should be the one
corresponding to just RSA ("1.2.840.113549.1.1.1" rsaEncryption), rather than OIDs which also include the digest algorithm (such as "1.2.840.113549.1.1.13" sha512WithRSAEncryption).

This means that the logic to compute the algorithm identifier is the same except when signing with RSA, in which case the OID will always be rsaEncryption. This is consistent with the OpenSSL implementation, and the RFCs that define PKCS#7 and S/MIME.

See RFC 3851 (section 2.2), and RFC 3370 (section 3.2) for more details.

alex · 2024-02-09T17:50:54Z

Haven't re-reviewed the RFC, but we should add a test for this.

alex · 2024-02-18T21:17:43Z

src/rust/src/pkcs7.rs

+    if key_type == x509::sign::KeyType::Rsa && !has_pss_padding {
+        Ok(common::AlgorithmIdentifier {
+            oid: asn1::DefinedByMarker::marker(),
+            params: common::AlgorithmParameters::Rsa(None),


Suggested change

params: common::AlgorithmParameters::Rsa(None),

params: common::AlgorithmParameters::Rsa(Some(())),

alex · 2024-02-18T21:18:51Z

src/rust/src/pkcs7.rs

+    rsa_padding: &'p pyo3::PyAny,
+) -> pyo3::PyResult<common::AlgorithmIdentifier<'static>> {
+    let key_type = x509::sign::identify_key_type(py, private_key)?;
+    let has_pss_padding = !rsa_padding.is_none() && rsa_padding.is_instance(types::PSS.get(py)?)?;


Suggested change

let has_pss_padding = !rsa_padding.is_none() && rsa_padding.is_instance(types::PSS.get(py)?)?;

let has_pss_padding = rsa_padding.is_instance(types::PSS.get(py)?)?;

I don't think the is_none() check is required? None isn't an instance of PSS :-)

Ah, I got that code from

cryptography/src/rust/src/x509/sign.rs

Lines 135 to 137 in 50ea0fa

// If this is RSA-PSS we need to compute the signature algorithm from the

// parameters provided in rsa_padding.

if !rsa_padding.is_none() && rsa_padding.is_instance(types::PSS.get(py)?)? {

. Should I remove it from there as well?

Probably that should be fixed, but no need to do it in this PR.

The current implementation defines the SMIMECapabilities attribute so that its value is a SEQUENCE of all the algorithm OIDs that are supported. However, the S/MIME v3 spec (RFC 2633) specifies that each algorithm should be specified in its own SEQUENCE: SMIMECapabilities ::= SEQUENCE OF SMIMECapability SMIMECapability ::= SEQUENCE { capabilityID OBJECT IDENTIFIER, parameters ANY DEFINED BY capabilityID OPTIONAL } (RFC 2633, Appendix A) This commit changes the implementation so that each algorithm is inside its own SEQUENCE. This also matches the OpenSSL implementation.

The current implementation computes the algorithm identifier used in the `digest_encryption_algorithm` PKCS#7 field (or `SignatureAlgorithmIdentifier` in S/MIME) based on both the algorithm used to sign (e.g. RSA) and the digest algorithm (e.g. SHA512). This is correct for ECDSA signatures, where the OIDs used include the digest algorithm (e.g: ecdsa-with-SHA512). However, due to historical reasons, when signing with RSA the OID specified should be the one corresponding to just RSA ("1.2.840.113549.1.1.1" rsaEncryption), rather than OIDs which also include the digest algorithm (such as "1.2.840.113549.1.1.13", sha512WithRSAEncryption). This means that the logic to compute the algorithm identifier is the same except when signing with RSA, in which case the OID will always be `rsaEncryption`. This is consistent with the OpenSSL implementation, and the RFCs that define PKCS#7 and S/MIME. See RFC 3851 (section 2.2), and RFC 3370 (section 3.2) for more details.

facutuesca · 2024-02-20T06:19:03Z

@alex I also added an item to the CHANGELOG

* Fix ASN.1 for S/MIME capabilities. The current implementation defines the SMIMECapabilities attribute so that its value is a SEQUENCE of all the algorithm OIDs that are supported. However, the S/MIME v3 spec (RFC 2633) specifies that each algorithm should be specified in its own SEQUENCE: SMIMECapabilities ::= SEQUENCE OF SMIMECapability SMIMECapability ::= SEQUENCE { capabilityID OBJECT IDENTIFIER, parameters ANY DEFINED BY capabilityID OPTIONAL } (RFC 2633, Appendix A) This commit changes the implementation so that each algorithm is inside its own SEQUENCE. This also matches the OpenSSL implementation. * Fix the RSA OID used for signing PKCS#7/SMIME The current implementation computes the algorithm identifier used in the `digest_encryption_algorithm` PKCS#7 field (or `SignatureAlgorithmIdentifier` in S/MIME) based on both the algorithm used to sign (e.g. RSA) and the digest algorithm (e.g. SHA512). This is correct for ECDSA signatures, where the OIDs used include the digest algorithm (e.g: ecdsa-with-SHA512). However, due to historical reasons, when signing with RSA the OID specified should be the one corresponding to just RSA ("1.2.840.113549.1.1.1" rsaEncryption), rather than OIDs which also include the digest algorithm (such as "1.2.840.113549.1.1.13", sha512WithRSAEncryption). This means that the logic to compute the algorithm identifier is the same except when signing with RSA, in which case the OID will always be `rsaEncryption`. This is consistent with the OpenSSL implementation, and the RFCs that define PKCS#7 and S/MIME. See RFC 3851 (section 2.2), and RFC 3370 (section 3.2) for more details. * Add tests for the changes in PKCS7 signing * PKCS7 fixes from code review * Update CHANGELOG

* Fix ASN.1 for S/MIME capabilities. The current implementation defines the SMIMECapabilities attribute so that its value is a SEQUENCE of all the algorithm OIDs that are supported. However, the S/MIME v3 spec (RFC 2633) specifies that each algorithm should be specified in its own SEQUENCE: SMIMECapabilities ::= SEQUENCE OF SMIMECapability SMIMECapability ::= SEQUENCE { capabilityID OBJECT IDENTIFIER, parameters ANY DEFINED BY capabilityID OPTIONAL } (RFC 2633, Appendix A) This commit changes the implementation so that each algorithm is inside its own SEQUENCE. This also matches the OpenSSL implementation. * Fix the RSA OID used for signing PKCS#7/SMIME The current implementation computes the algorithm identifier used in the `digest_encryption_algorithm` PKCS#7 field (or `SignatureAlgorithmIdentifier` in S/MIME) based on both the algorithm used to sign (e.g. RSA) and the digest algorithm (e.g. SHA512). This is correct for ECDSA signatures, where the OIDs used include the digest algorithm (e.g: ecdsa-with-SHA512). However, due to historical reasons, when signing with RSA the OID specified should be the one corresponding to just RSA ("1.2.840.113549.1.1.1" rsaEncryption), rather than OIDs which also include the digest algorithm (such as "1.2.840.113549.1.1.13", sha512WithRSAEncryption). This means that the logic to compute the algorithm identifier is the same except when signing with RSA, in which case the OID will always be `rsaEncryption`. This is consistent with the OpenSSL implementation, and the RFCs that define PKCS#7 and S/MIME. See RFC 3851 (section 2.2), and RFC 3370 (section 3.2) for more details. * Add tests for the changes in PKCS7 signing * PKCS7 fixes from code review * Update CHANGELOG Co-authored-by: Facundo Tuesca <facundo.tuesca@trailofbits.com>

alex added this to the Forty Third Release milestone Feb 10, 2024

facutuesca force-pushed the smime-capabilities-fix branch from 2e5b1e2 to 67c1a60 Compare February 12, 2024 20:32

facutuesca changed the title ~~Fix ASN.1 for S/MIME capabilities.~~ Fix ASN.1 issues in PKCS#7 and S/MIME signing Feb 12, 2024

facutuesca force-pushed the smime-capabilities-fix branch 4 times, most recently from 2755b62 to 1f797f8 Compare February 12, 2024 21:22

facutuesca marked this pull request as ready for review February 12, 2024 21:32

alex reviewed Feb 18, 2024

View reviewed changes

facutuesca added 5 commits February 20, 2024 07:03

Add tests for the changes in PKCS7 signing

b18f7b1

PKCS7 fixes from code review

c719313

Update CHANGELOG

99f6aba

facutuesca force-pushed the smime-capabilities-fix branch from 1f797f8 to 99f6aba Compare February 20, 2024 06:15

facutuesca requested a review from alex February 20, 2024 06:20

alex approved these changes Feb 20, 2024

View reviewed changes

alex merged commit 8ef3b38 into pyca:main Feb 20, 2024

facutuesca deleted the smime-capabilities-fix branch February 21, 2024 07:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ASN.1 issues in PKCS#7 and S/MIME signing#10373

Fix ASN.1 issues in PKCS#7 and S/MIME signing#10373
alex merged 5 commits intopyca:mainfrom
trail-of-forks:smime-capabilities-fix

facutuesca commented Feb 9, 2024 •

edited

Loading

Uh oh!

alex commented Feb 9, 2024

Uh oh!

alex Feb 18, 2024

Uh oh!

facutuesca Feb 20, 2024

Uh oh!

alex Feb 18, 2024

Uh oh!

facutuesca Feb 19, 2024

Uh oh!

alex Feb 19, 2024

Uh oh!

facutuesca Feb 20, 2024

Uh oh!

facutuesca commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

	params: common::AlgorithmParameters::Rsa(None),
	params: common::AlgorithmParameters::Rsa(Some(())),

	let has_pss_padding = !rsa_padding.is_none() && rsa_padding.is_instance(types::PSS.get(py)?)?;
	let has_pss_padding = rsa_padding.is_instance(types::PSS.get(py)?)?;

	// If this is RSA-PSS we need to compute the signature algorithm from the
	// parameters provided in rsa_padding.
	if !rsa_padding.is_none() && rsa_padding.is_instance(types::PSS.get(py)?)? {

Conversation

facutuesca commented Feb 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

First issue

Second issue

Uh oh!

alex commented Feb 9, 2024

Uh oh!

alex Feb 18, 2024

Choose a reason for hiding this comment

Uh oh!

facutuesca Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

alex Feb 18, 2024

Choose a reason for hiding this comment

Uh oh!

facutuesca Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

alex Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

facutuesca Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

facutuesca commented Feb 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

facutuesca commented Feb 9, 2024 •

edited

Loading