Add support for encrypting S/MIME messages #10889

facutuesca · 2024-04-25T17:22:49Z

I'm opening this PR with an initial implementation of S/MIME encryption, in order to better discuss the API design, the algorithms we want to support, and how we want to approach testing.

The target is a subset of S/MIME v3.2 (RFC5751):

Content encryption is done using AES-128-CBC
Key management is done with key transport: the symmetric encryption key used for the message is included and encrypted using the recipients' public keys.
The other two key management methods (previously-distributed keys and key agreement) are not supported.
The symmetric encryption key is encrypted using RSA (PKCS1 v1.5). That is, we only support recipients with RSA public keys, and we use PKCS1v15 padding.

Here are the openssl commands that can be used for testing, to compare our encrypted output against, or to decrypt our encrypted output.

# encrypt
openssl smime -encrypt  -aes-128-cbc -in msg.txt -out out.txt -outform PEM vectors/cryptography_vectors/x509/custom/ca/rsa_ca.pem
# decrypt
openssl smime -decrypt -recip vectors/cryptography_vectors/x509/custom/ca/rsa_ca.pem -inkey vectors/cryptography_vectors/x509/custom/ca/rsa_key.pem -in out.txt -inform pem

I added some tests for the unencrypted parts of the message, but complete testing would require that we parse and decrypt the messages. We could follow a similar approach as with testing S/MIME signing, where we call OpenSSL directly to parse and check our output during the tests

The tests now use OpenSSL's PKCS7_decrypt in order to see if our encrypted output can be correctly decrypted by OpenSSL.

cc @alex @reaperhulk @woodruffw

(the issue tracking this feature is #5488)

TODO:

Write the docs
Add to CHANGELOG

woodruffw · 2024-04-25T19:18:04Z

src/rust/cryptography-x509/src/common.rs

+    //
+    // AES-IV ::= OCTET STRING (SIZE(16))
+    #[defined_by(oid::AES_128_CBC_OID)]
+    AesCbc(&'a [u8]),


Out of curiosity: does rust-asn1 know to turn this &[u8] into the inner value of the OCTET STRING, or does this end up containing the raw TLV for the OCTET STRING? I suspect it's the former, but we should confirm.

&[u8] in rust-asn1 are OCTET STRINGs, not raw TLVs.

woodruffw · 2024-04-25T19:19:31Z

src/rust/cryptography-x509/src/oid.rs

+pub const AES_256_CBC_OID: asn1::ObjectIdentifier = asn1::oid!(2, 16, 840, 1, 101, 3, 4, 1, 42);
+pub const AES_192_CBC_OID: asn1::ObjectIdentifier = asn1::oid!(2, 16, 840, 1, 101, 3, 4, 1, 22);
+pub const AES_128_CBC_OID: asn1::ObjectIdentifier = asn1::oid!(2, 16, 840, 1, 101, 3, 4, 1, 2);


Just for cross-checking: confirmed these against their parent arc: https://oidref.com/2.16.840.1.101.3.4.1

woodruffw · 2024-04-25T19:24:50Z

src/cryptography/hazmat/primitives/serialization/pkcs7.py

+    fp = io.BytesIO()
+    g = email.generator.BytesGenerator(
+        fp,
+        maxheaderlen=0,
+        mangle_from_=False,
+        policy=m.policy.clone(linesep="\n"),
+    )
+    g.flatten(m)
+    return fp.getvalue()


Nitpick: you might be able to get away with just m.as_bytes(...) rather than jumping through a BytesIO + BytesGenerator here.

woodruffw · 2024-04-26T19:51:25Z

src/cryptography/hazmat/primitives/serialization/pkcs7.py

@@ -225,6 +307,26 @@ def _smime_encode(
    return fp.getvalue()


+def _smime_enveloped_encode(data: bytes) -> bytes:
+    # This function works pretty hard to replicate what OpenSSL does
+    # precisely. For good and for ill.


Nitpick: might be good to include a URL or code reference for OpenSSL's construction of the encoding here, just in case this ever needs to be re-evaluated 🙂

This is actually a comment from the other encoder function that I used as a base for this one (_smime_signed_encode).

I think it doesn't make much sense here since we just add the headers, so I removed it

alex

Left a few comments. Deciding on a testing strategy for the encrypted components is probably the most important next step.

alex · 2024-06-22T18:45:07Z

src/cryptography/hazmat/primitives/serialization/pkcs7.py

+        data: bytes | None = None,
+        recipients: list[x509.Certificate] | None = None,


Suggested change

data: bytes | None = None,

recipients: list[x509.Certificate] | None = None,

*,

_data: bytes | None = None,

_recipients: list[x509.Certificate] | None = None,

People keep assuming these are public, let's make that harder for them.

alex · 2024-06-22T18:46:17Z

src/cryptography/hazmat/primitives/serialization/pkcs7.py

+        # These options only make sense when signing, not encrypting
+        if (
+            PKCS7Options.NoAttributes in options
+            or PKCS7Options.NoCapabilities in options
+            or PKCS7Options.NoCerts in options
+            or PKCS7Options.DetachedSignature in options
+        ):


I think we probably want to do an allow-list of sensible options, not the inverse.

alex · 2024-06-22T18:46:52Z

src/rust/cryptography-x509/src/common.rs

+    //
+    // AES-IV ::= OCTET STRING (SIZE(16))
+    #[defined_by(oid::AES_128_CBC_OID)]
+    AesCbc(&'a [u8]),


Pretty sure if you rebase you'll find this was added in the meantime :-)

I see that Aes256Cbc was added, did you mean that one?

Oops, missed that. I think you'll want to follow the style on that one -- using fixed size array and name including 128

alex · 2024-06-22T18:47:46Z

src/rust/cryptography-x509/src/pkcs7.rs

+#[derive(asn1::Asn1Write)]
+pub struct EncryptedContentInfo<'a> {
+    pub _content_type: asn1::ObjectIdentifier,
+    pub content_encryption_algorithm: common::AlgorithmIdentifier<'a>,
+    #[implicit(0)]
+    pub content: Option<&'a [u8]>,
+}


Note this is also defined in #11059

Fixed so that the field names match

alex · 2024-06-22T18:49:23Z

src/rust/src/pkcs7.rs

+    let padder = types::SYMMETRIC_PADDING_PKCS7
+        .get(py)?
+        .call1((128,))?
+        .call_method0(pyo3::intern!(py, "padder"))?;
+    let padded_content_start =
+        padder.call_method1(pyo3::intern!(py, "update"), (data_with_header,))?;
+    let padded_content_end = padder.call_method0(pyo3::intern!(py, "finalize"))?;
+    let padded_content = padded_content_start.add(padded_content_end)?;


There's now a rust pkcs7 padding API

Fixed. Since the new PKCS7PaddingContext::update method takes a CffiBuf, I had to use PyBytes::new_bound to go from Cow[u8] to PyBound<PyBytes> to CffiBuf, which means there is now an extra copy operation. Is there a better way of doing that?

alex · 2024-06-22T18:50:08Z

src/rust/src/pkcs7.rs

+    // The message is encrypted with AES-128-CBC, which the S/MIME v3.2 RFC
+    // specifies as MUST support (https://datatracker.ietf.org/doc/html/rfc5751#section-2.7)


Someone noted that there's an S/MIME 4.0 which supports AEADs. Is there a reason not to use them?

Not really, mostly scope (I limited this PR to a subset of S/MIME 3.2)

I’m fine with that unless 4.0 is broadly supported, at which point we should just do 4.0 only. Is there any reasonable way to survey current support?

I'm not sure actually. A quick internet search doesn't reveal anything about usage of v3 vs v4. @woodruffw, do you know if there's a way?

A rough way to determine this would be just to see if the major S/MIME clients all support it. e.g. does Mail.app? Does Outlook? Gmail has S/MIME, what about that? Thunderbird?

I did a bit of searching for this:

I can't find references to AEAD cipher support (or other S/MIME 4.0 features) in the public docs for Mail.app, Outlook, or Gmail

Thunderbird appears to default to S/MIME 3.x, e.g. using AES-128-CBC by default. It's unclear whether they support AEADs for verifying.

Other parts of RFC 8551 appear to be unsupported by Thunderbird: https://bugzilla.mozilla.org/show_bug.cgi?id=1600776

Edit: here's a tracker for RFC 8551 in Thunderbird: https://bugzilla.mozilla.org/show_bug.cgi?id=1847703. TL;DR is that much of the RFC is not implemented yet, it seems.

With a bit more searching: as best I can tell, all of the major mail clients handle S/MIME encryption by parroting whatever cipher selection they receive (with some enforcing a floor, e.g. no 3DES unless explicitly enabled). Thunderbird may support the SMIMECapabilities extension for improved negotiation, but not unless explicitly enabled through an about:config setting.

Support among other MUAs is similarly mixed or not present: https://security.stackexchange.com/questions/271353/any-client-still-supporting-the-s-mime-capabilities-extension-in-2023

TL;DR: It seems like MUAs are still pretty firmly on S/MIME 3.1 and 3.2.

This comment has a good summary as well: https://bugzilla.mozilla.org/show_bug.cgi?id=1847703#c7

facutuesca · 2024-07-01T17:01:37Z

@alex @woodruffw

Deciding on a testing strategy for the encrypted components is probably the most important next step

I added a binding for PKCS7_decrypt and changed the tests to check that the encrypted output can be correctly decrypted by OpenSSL.

There are 4 new commits since the last review round:

Use correct PKCS7/SMIME OID for enveloped data
- This was a mistake I did while rebasing. We use envelopedData rather than encryptedData for S/MIME encryption.
Fail when passing Text and Binary options at the same time
- OpenSSL allows passing both options at the same time, but ignores Text. We defensively fail here so that users know they're doing something weird/wrong.
Use PKCS7 tag for PEM-encoding encrypted messages
- Originally I used the CMS tag for PEM encoded messages (-----BEGIN CMS-----), matching the output of openssl cms. However, this doesn't match our other APIs which use -----BEGIN PKCS7----- (which corresponds to openssl smime). Furthermore, if we wanted to test decryption of the former, we would need to add new bindings for cms.h functions and structures, whereas with PKCS7 we can reuse the existing bindings and be consistent with our other PCKS7 APIs.
Test S/MIME encryption by decrypting and comparing with plaintext
- As mentioned before, it adds a binding for PKCS7_decrypt and uses it to test our encrypted output.

alex

Started to review but realized there's going to be a few things that are no longer needed now that pkcs12 is merged.

alex · 2024-07-05T19:06:27Z

src/cryptography/hazmat/primitives/serialization/pkcs7.py

+            [
+                opt not in [PKCS7Options.Text, PKCS7Options.Binary]
+                for opt in options
+            ]


No need for list comprehension, generator is fine

alex · 2024-07-05T19:07:06Z

src/rust/cryptography-x509/src/common.rs

+    //
+    // AES-IV ::= OCTET STRING (SIZE(16))
+    #[defined_by(oid::AES_128_CBC_OID)]
+    AesCbc(&'a [u8]),


Oops, missed that. I think you'll want to follow the style on that one -- using fixed size array and name including 128

src/rust/cryptography-x509/src/pkcs7.rs

alex · 2024-07-14T23:43:06Z

src/rust/src/pkcs7.rs

+    let encryptor = cipher.call_method0(pyo3::intern!(py, "encryptor"))?;
+    let encrypted_content_start =
+        encryptor.call_method1(pyo3::intern!(py, "update"), (padded_content,))?;
+    let encrypted_content_end = encryptor.call_method0(pyo3::intern!(py, "finalize"))?;
+    let encrypted_content = encrypted_content_start.add(encrypted_content_end)?;


We've now got symmetric encryption repeated a few times. I'm going to refactor the duplication into a fucntion, then it can be used here.

Fixed, now this uses pkcs12::symmetric_encrypt too

alex · 2024-07-14T23:44:34Z

src/rust/src/pkcs7.rs

+    submod.add_function(pyo3::wrap_pyfunction_bound!(
+        encrypt_and_serialize,
+        &submod
+    )?)?;


Merge conflict here, sorry!

alex · 2024-07-14T23:45:26Z

tests/hazmat/primitives/test_pkcs7.py

+
+    return _read_mem_bio(out_bio, backend)
+
+


Does rust-openssl have teh functions we need? It might make mroe sense to expose a test API from there (see the asn1 testcertificate API for an example of this).

What do you think @reaperhulk ?

Makes sense, I removed the decrypt (and helper) Python functions from here, and added a Rust pkcs7_decrypt to test_support.rs, similar to the new pkcs7_verify.

facutuesca · 2024-07-17T09:07:31Z

@alex I squashed the old commits into a single one, and then added two new commits with the changes since your last review

alex

One comment, and this needs docs and a changelog entry, but otherwise looks great. Thanks for your work here!

alex · 2024-07-17T11:30:33Z

tests/hazmat/primitives/test_pkcs7.py

+        with open("msg.p7m", "wb") as f:
+            f.write(enveloped)


I assume this was debugging leftover?

yes, good catch! Fixed

reaperhulk · 2024-07-17T13:51:56Z

src/cryptography/hazmat/primitives/serialization/pkcs7.py

+        self,
+        encoding: serialization.Encoding,
+        options: typing.Iterable[PKCS7Options],
+        backend: typing.Any = None,


Why are we including this on new APIs?

my bad, it's a leftover from copy-pasting the sign() method. Removed

reaperhulk

This generally looks good now, so just one final question: have we verified that S/MIME encrypted messages can be successfully decrypted by one or more of the major clients? I see we test decryption in OpenSSL and we follow the spec, but it'd be good to have that final bit of validation.

facutuesca · 2024-07-17T20:34:50Z

and this needs docs and a changelog entry,

I pushed a commit with the doc and changelog entry

have we verified that S/MIME encrypted messages can be successfully decrypted by one or more of the major clients?

I'll do this next

alex · 2024-07-17T22:10:03Z

Let us know once you've confirmed and we'll do a final review. Thanks for your work on this!

facutuesca · 2024-07-18T14:41:53Z

@alex @reaperhulk I verified that the output can be read by both Mail.app and Thunderbird. The way I did it was by saving the output of encrypt() to an .eml file, and importing that file in both applications. Before adding the corresponding certificate and private key to the settings/keychain, this is what you see:

Mail.app (no private key available)

Thunderbird (no private key available)

After importing the private key:

Mail.app (private key in keychain)

Thunderbird (private key in keychain)

reaperhulk · 2024-07-18T15:40:54Z

@facutuesca Thanks! Apologies for one final question here: Users are likely to want to do both signing and encryption now that we have them -- are our current APIs composable to achieve that or is there some gross PKCS7 data structure we need to implement still?

facutuesca · 2024-07-18T15:50:37Z

@facutuesca Thanks! Apologies for one final question here: Users are likely to want to do both signing and encryption now that we have them -- are our current APIs composable to achieve that or is there some gross PKCS7 data structure we need to implement still?

They are already composable: the S/MIME RFC specifies that creating signed+encrypted messages is just a matter of doing one operation on the output of the other:

The signed-only, enveloped-only, and compressed-only MIME formats can
be nested. This works because these formats are all MIME entities
that encapsulate other MIME entities.

and

It is possible to apply any of the signing, encrypting, and
compressing operations in any order. It is up to the implementer and
the user to choose.

(src)

reaperhulk · 2024-07-18T15:51:35Z

That's what I wanted to hear, thanks! 😄

woodruffw · 2024-07-18T16:08:21Z

It is possible to apply any of the signing, encrypting, and
compressing operations in any order. It is up to the implementer and
the user to choose.

lol, very good cryptography design.

woodruffw reviewed Apr 25, 2024

View reviewed changes

facutuesca force-pushed the smime-encryption branch from ed60635 to 9b83b39 Compare April 26, 2024 13:27

woodruffw reviewed Apr 26, 2024

View reviewed changes

facutuesca force-pushed the smime-encryption branch from 6a2e2ef to 21c50ec Compare April 29, 2024 12:43

facutuesca force-pushed the smime-encryption branch from 21c50ec to ddebb90 Compare June 17, 2024 14:27

facutuesca marked this pull request as ready for review June 17, 2024 14:49

facutuesca requested review from alex and woodruffw June 17, 2024 14:49

alex reviewed Jun 22, 2024

View reviewed changes

facutuesca force-pushed the smime-encryption branch from ddebb90 to d941be0 Compare June 24, 2024 09:52

facutuesca force-pushed the smime-encryption branch 3 times, most recently from c37582e to 7797423 Compare July 1, 2024 16:42

alex reviewed Jul 5, 2024

View reviewed changes

facutuesca force-pushed the smime-encryption branch from 7797423 to 0fe9bb6 Compare July 8, 2024 17:30

alex reviewed Jul 14, 2024

View reviewed changes

Add support for encrypting S/MIME messages

d523419

facutuesca force-pushed the smime-encryption branch from 0fe9bb6 to 9236f70 Compare July 17, 2024 08:44

facutuesca added 2 commits July 17, 2024 10:59

Move PKCS7 decrypt test function to Rust

8e35141

Use symmetric encryption function from PKCS12

74d899e

facutuesca force-pushed the smime-encryption branch from 9236f70 to 74d899e Compare July 17, 2024 08:59

facutuesca requested a review from alex July 17, 2024 09:07

alex reviewed Jul 17, 2024

View reviewed changes

Remove debug file write from tests

c5e6053

reaperhulk reviewed Jul 17, 2024

View reviewed changes

Remove unneeded backend parameter

cadb5b4

reaperhulk reviewed Jul 17, 2024

View reviewed changes

docs and changelog

8155995

reaperhulk approved these changes Jul 18, 2024

View reviewed changes

reaperhulk merged commit 0faaffc into pyca:main Jul 18, 2024
57 checks passed

facutuesca deleted the smime-encryption branch July 18, 2024 15:53

reaperhulk mentioned this pull request Jul 20, 2024

Add S/MIME encryption #5488

Closed

		data: bytes \| None = None,
		recipients: list[x509.Certificate] \| None = None,

		// The message is encrypted with AES-128-CBC, which the S/MIME v3.2 RFC
		// specifies as MUST support (https://datatracker.ietf.org/doc/html/rfc5751#section-2.7)

Add support for encrypting S/MIME messages #10889

Add support for encrypting S/MIME messages #10889

Conversation

facutuesca commented Apr 25, 2024 • edited Loading

TODO:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facutuesca Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woodruffw Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facutuesca commented Jul 1, 2024

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facutuesca commented Jul 17, 2024

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facutuesca Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

reaperhulk left a comment

Choose a reason for hiding this comment

facutuesca commented Jul 17, 2024

alex commented Jul 17, 2024

facutuesca commented Jul 18, 2024

Mail.app (no private key available)

Thunderbird (no private key available)

Mail.app (private key in keychain)

Thunderbird (private key in keychain)

reaperhulk commented Jul 18, 2024

facutuesca commented Jul 18, 2024

reaperhulk commented Jul 18, 2024

woodruffw commented Jul 18, 2024

facutuesca commented Apr 25, 2024 •

edited

Loading

woodruffw Apr 25, 2024 •

edited

Loading

facutuesca Jul 17, 2024 •

edited

Loading

woodruffw Jul 17, 2024 •

edited

Loading

facutuesca Jul 17, 2024 •

edited

Loading