Add support for decrypting S/MIME messages #11555

nitneuqr · 2024-09-06T17:01:31Z

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

My essential thoughts for testing were to do the round-trip: encryption using the PKCS7EnvelopeBuilder and decryption using the PKCS7EnvelopeDecryptor (or whatever its name will be). No tests were made against openssl, even though it might interesting.

For the Python API, I've tried to stick as much as possible with the current API design (PKCS7SignatureBuilder, PKCS7EnvelopeBuilder).

I'm new to rust, so please let me know if you see some issues in variable lifetime, or some unnecessary copying between Python & Rust.

During development, I've realized that we could migrate PKCS7 unpadding to rust instead of using types, so I did migrate it. I'll probably open another smaller PR for that matter (if that makes sense?). I still have some code using PKCS7 Unpadding on Python if needed.

cc @alex

alex · 2024-09-06T17:03:10Z

Thanks for working on this! I haven't had a chance to look in depth, but re: PKCS7Unpadder, it'd be great to have that in a stand-alone PR.

nitneuqr · 2024-09-06T17:15:50Z

Thanks for working on this! I haven't had a chance to look in depth, but re: PKCS7Unpadder, it'd be great to have that in a stand-alone PR.

Sure! Opened #11556 for that matter.

nitneuqr · 2024-09-08T09:05:13Z

Rebased to main after integration of #11556. Should I split it again in smaller PRs? Maybe around symmetric_decrypt or other subfunctions in the code.

alex · 2024-09-08T16:31:49Z

We're always happy to have smaller PRs split out (though it can sometimes be complicated due to coverage). I'm hoping to have time to review this today, though it might be tomorrow.

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

alex · 2024-09-10T03:50:04Z

@facutuesca FYI, if you're interested

nitneuqr · 2024-09-26T07:21:28Z

@alex @reaperhulk took into account the first comments and adapted the tests / coverage accordingly.

Do you have any time for another review? Do you need anything else from me before reviewing? 😊

alex · 2024-10-05T19:40:15Z

Sorry I got behind, will have a look now. Thanks for your patience!

alex

Did an initial review of the code -- I haven't really sat down and reviewed the core PKCS#7 logic against hte RFC yet though.

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

src/rust/src/pkcs12.rs

src/rust/src/pkcs7.rs

alex · 2024-10-05T19:47:19Z

src/rust/src/pkcs7.rs

+    };
+
+    // Deserialize the content info
+    let content_info = asn1::parse_single::<pkcs7::ContentInfo<'_>>(&extracted_data).unwrap();


@reaperhulk This would be our first time using our DER parser for PKCS7. On the one hand: new API, so there's no backwards-compat concerns. On the other hand, surely someone is emitting awful BER PKCS#7. Any concerns?

@reaperhulk just going to keep pinging you on this :-) (Enjoy your vacation)

Let's assume we release with DER only parsing. In that scenario we're attempting to empirically determine whether or not there's enough BER out there to matter. If no one comes to complain then great, job done. However, if we discover we're wrong and BER is relevant, what is our plan? Are we going to add BER support in rust?

If we do determine that this API is basically low utility for many users because their S/MIME messages aren't valid DER, it seems we have a few options:

Declare that's how it goes. This is a net new API, so no users are worse off than they were before. May result in greater timelines for adoption of this, and migration away from other libraries.

Re-implement in terms of OpenSSL S/MIME APIs. Probably straightforward, although as-ever, assessing the compatibility surface will be a PITA.

Add BER support to rust-asn1. No fucking chance, not worth discussing.

We force ourselves to do a survey of the ecosystem before landing this so we can contact producers of non-DER S/MIMEs.

I have a soft spot for 1, I could be persuaded of 4, and if we really need to do it, I guess 2 is the way to go.

WDYT?

Does Go support this? They're DER only for whatever set of PKCS7 they support right?

Go stdlib doesn't support PKCS#7 I don't think. I did find:

https://github.com/mozilla-services/pkcs7/blob/v0.9.0/pkcs7.go#L155-L160

https://github.com/InfiniteLoopSpace/go_S-MIME/blob/master/cms/protocol/contentinfo.go#L25

Sooooo

Okay, I'm comfortable with doing this DER only and finding out how it goes. It's net-new API surface so we're not regressing and I'd love to find out BER doesn't matter.

src/rust/src/pkcs7.rs

src/rust/src/test_support.rs

alex

@reaperhulk would still like your views on deserialization for PKCS#7 in Rust.

src/rust/src/pkcs7.rs

src/rust/src/test_support.rs

vectors/cryptography_vectors/pkcs7/rsa_ca.pem

alex · 2024-10-28T16:13:46Z

(Thanks for updating, I'm going to pause on reviewing until #11843 is merged, hopefully that'll be quick.)

nitneuqr · 2024-11-06T08:37:05Z

#11843 is merged, I've rebased the branch onto main to take the new changes into account. It should b ready to review again!

alex · 2024-11-06T12:08:00Z

Great, will try to get to it this evening. Thanks!

…

On Wed, Nov 6, 2024 at 3:37 AM Quentin Retourne ***@***.***> wrote: #11843 is merged, I've rebased the branch onto main to take the new changes into account. It should b ready to review again! — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

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

nitneuqr · 2024-11-17T11:16:20Z

Hey, any news on this? Do you need anything else from me? 😊

alex · 2024-11-17T15:47:25Z

Ooops, sorry this fell off my radar. I will get to it today!

reaperhulk · 2024-11-17T15:48:19Z

@nitneuqr Could you rebase this to fix the conflict? 😄

reaperhulk · 2024-11-17T16:02:07Z

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

@@ -263,6 +263,81 @@ def encrypt(
        return rust_pkcs7.encrypt_and_serialize(self, encoding, options)


+def pkcs7_decrypt_der(


I think we should move all these functions to rust and export them here.

Which also means, passing the smime-related function in Python as private, and calling it on Rust, I guess? And also doing all the initial checks in Rust?

If you look at pkcs7.rs and search for SMIME_ENVELOPED_ENCODE you can see how we call into the S/MIME Python from Rust.

Done! One last question: should I also move the logic of the new _check_pkcs7_decrypt_arguments function to rust, or just leave it as such and call it in the rust code, the same way I'm calling SMIME_ENVELOPED_DECODE?

Yes, let's move that as well -- it actually will probably become a fair bit easier, as many things are automatically handled by giving parameters types.

Should be done. I'd love a quick review on the check_decrypt_parameters function, it is probably more intricate than it should be!

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

src/rust/src/pkcs7.rs

alex · 2024-11-17T19:34:25Z

src/rust/src/pkcs7.rs

+                }
+            };
+
+            // Decrypt the key using the private key


Before this, you need to check that key_encryption_algorithm is the expected type.

I see this on the RFC :

I'll try to create another PKCS#7 with the OAEP padding instead of PKCS#1 v1.5, to have full coverage then.

The only way it could happen is a bug, or an attempt to exploit something. We just want to be conservative in making sure we strictly enforce things.

Actually, it's even more relevant than that. Basically our key decryption does not support RSAES-OAEP for now so we need to specify this case, add a vector and tests for coverage. Just pushed it now, letting you check.

src/rust/src/pkcs7.rs

alex · 2024-11-17T19:35:54Z

src/rust/src/pkcs7.rs

+        let header = b"Content-Type: text/plain\r\n\r\n";
+        if data.starts_with(header) {
+            &data[header.len()..]
+        } else {


If the header isn't present in text mode, should that be an error?

I have no idea how OpenSSL handles it for now. I'll check it out when I have time and let you know.

After some checking with my version of OpenSSL (on Windows!), it seems the behavior of the -text and -binary options on decrypt are much weirder than I expected. They are not symmetrical to their usage in encrypt :

No options passed : replaces the \n into \r\n

-binary : leaves the \n as such, do not replace them by \r\n

-text : Removes the header, raises an error if there is none (your comment is correct)

Both options can be combined. In encrypt, -binary was taking precedence.

This implies that if you encrypt then decrypt data using openssl smime and the text option, you can start with Hello, world!\n and end up with Hello, world!\r\r\n 😢

I was mostly using -binary optons in my use cases, so I never quite found out. Adding this right now.

@nitneuqr this looks similar to a bug I encountered while implementing s/mime signing: openssl/openssl#23886. There is also a comment on that issue from someone finding the same bug during encryption

Thanks for that. To me, the documentation for the -binary option makes sense, at least during the -encrypt part. But for decryption, it doesn't.

-binary

Normally the input message is converted to "canonical" format which is effectively using CR and LF as end of line: as required by the S/MIME specification. When this option is present no translation occurs. This is useful when handling binary data which may not be in MIME format.

To be honest, for this PR I'd stick with the current behavior of OpenSSL (even if seems incorrect). What's your opinion on this?

To be honest, for this PR I'd stick with the current behavior of OpenSSL (even if seems incorrect). What's your opinion on this?

I think it's up to Alex and Paul. For context, the decision for signing was to follow the RFC, rather than imitate what OpenSSL does.

I might have misunderstood your first comment, so I'm rephrasing just to be sure:

Your issue was with the format of the output file (which -crlfeol is supposed to control in OpenSSL). Ours is about the format of the ciphered message inside it (which is controlled by -text or -binary), at least for encryption / decryption.

As far as my testing goes, the -crlfeol parameter does not seem to have any effect in decryption. Nevertheless, the issue looks similar and I guess we need to address it the same way as yours (by following the RFC instead of OpenSSL).

Thanks for the feedback 😄

alex

A few small nits, but this has come together great!

I want @reaperhulk's opinion on the text vs. binary stuff, since IIRC he did the original impl for the encryption side.

src/rust/src/pkcs7.rs

alex · 2024-11-21T04:31:37Z

src/rust/src/pkcs7.rs

+        }
+    };
+
+    // If text_mode, strip the header if possible, else return an error


@reaperhulk can I get your opinion on the right behavior for text vs. binary?

We added these flags to behave like OpenSSL (sigh).

OpenSSL calls SMIME_text in its decryption path when you pass PKCS7_TEXT. That function errors for a few conditions, but most notably if parsing the MIME headers fails, if there is no “content-type” header, or if the content type is not “text/plain”.

This code appears to make two assumptions that may not match OpenSSL's though:

Case sensitivity on Content-Type

That the Content-Type header is guaranteed to be the last header before the payload.

The behavior I believe we want is to parse the MIME headers, check that content-type is text/plain, and then discard them entirely and only keep the payload.

Do you have any suggestion of a lib to handle MIME efficiently in rust? For now, I'm thinking about
using some Python function. Seems simpler to me.

I would just use Python for this.

First version pushed, will compare with openssl and let you know!

And yeah, it should be up to the user to sanitize the decrypted output after pkcs7_decrypt, especially with the libraries available on Python that are perfect for this ...

I've tested against some custom headers (you can see them in tests), behavior is the same as openssl (using test_support.pkcs7_decrypt from main).

src/rust/src/pkcs7.rs

removed excess get_type

src/rust/src/pkcs7.rs

alex

Besides these unwrap comments and @reaperhulk's on text, I think this is ready. Thanks so much for your continued work on it.

cleaned tests, added some others added more vectors

src/rust/src/pkcs7.rs

added tests accordingly will compare with OpenSSL

nitneuqr · 2024-11-25T10:59:37Z

Besides these unwrap comments and @reaperhulk's on text, I think this is ready. Thanks so much for your continued work on it.

Thanks for your reviews! Should be good now, header removal is now tested in test_pkcs7_decrypt_der_text_handmade_header, and unwraps have been removed.

reaperhulk

Other than my small nits LGTM on the text path now. Thanks!

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

alex

One final nit + Paul's comments and we can merge!

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

src/rust/src/pkcs7.rs

nitneuqr · 2024-11-26T08:24:34Z

One final nit + Paul's comments and we can merge!

Should be done now! Merci beaucoup! Also added some documentation 🛩️

some minor refactoring

alex

Thank you for all your hard work and persistence on this!

reaperhulk · 2024-11-26T16:24:30Z

Yes, thank you for sticking with this and being so responsive. It's a pleasure having contributors like you!

nitneuqr · 2024-11-26T16:55:25Z

My pleasure! I might come back in a few days / weeks to tackle some more problems (namely, pkcs verify and encrypt/decrypt with other content encryption algorithms)

nitneuqr mentioned this pull request Sep 6, 2024

Rustify PKCS7 unpadding #11556

Merged

nitneuqr force-pushed the smime-decryption branch 2 times, most recently from cb082ce to e1c4620 Compare September 8, 2024 09:01

alex reviewed Sep 10, 2024

View reviewed changes

src/cryptography/hazmat/primitives/serialization/pkcs7.py Outdated Show resolved Hide resolved

nitneuqr force-pushed the smime-decryption branch from d4e7741 to 69d2e23 Compare September 12, 2024 20:44

nitneuqr requested review from alex and reaperhulk September 18, 2024 08:46

alex reviewed Oct 5, 2024

View reviewed changes

alex reviewed Oct 21, 2024

View reviewed changes

src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved

src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved

src/rust/src/test_support.rs Outdated Show resolved Hide resolved

vectors/cryptography_vectors/pkcs7/rsa_ca.pem Outdated Show resolved Hide resolved

nitneuqr force-pushed the smime-decryption branch from 1591c66 to 0dea78d Compare October 27, 2024 12:26

nitneuqr mentioned this pull request Oct 27, 2024

added new vectors for PKCS7 tests #11843

Merged

nitneuqr force-pushed the smime-decryption branch from e504553 to fd8db30 Compare November 6, 2024 08:34

alex reviewed Nov 8, 2024

View reviewed changes

src/rust/cryptography-x509/src/pkcs7.rs Outdated Show resolved Hide resolved

nitneuqr mentioned this pull request Nov 8, 2024

passing PKCS7 Content Info to readable #11922

Merged

reaperhulk reviewed Nov 17, 2024

View reviewed changes

alex reviewed Nov 17, 2024

View reviewed changes

nitneuqr force-pushed the smime-decryption branch from 91b014c to f39a11d Compare November 18, 2024 09:27

removed a parameter

be4db81

alex reviewed Nov 21, 2024

View reviewed changes

nitneuqr added 2 commits November 21, 2024 09:27

took comments into account

c398b60

removed unused import

cde2c71

removed excess get_type

alex reviewed Nov 22, 2024

View reviewed changes

src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved

alex reviewed Nov 22, 2024

View reviewed changes

src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved

alex reviewed Nov 22, 2024

View reviewed changes

src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved

alex reviewed Nov 22, 2024

View reviewed changes

added first unwrap corrections

8ef2ac0

cleaned tests, added some others added more vectors

alex reviewed Nov 24, 2024

View reviewed changes

src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved

nitneuqr added 2 commits November 24, 2024 19:25

no more unwrap for parameter checks

4ba77de

removing headers is Python now

47e5004

added tests accordingly will compare with OpenSSL

reaperhulk reviewed Nov 25, 2024

View reviewed changes

src/cryptography/hazmat/primitives/serialization/pkcs7.py Outdated Show resolved Hide resolved

src/cryptography/hazmat/primitives/serialization/pkcs7.py Outdated Show resolved Hide resolved

src/cryptography/hazmat/primitives/serialization/pkcs7.py Outdated Show resolved Hide resolved

alex reviewed Nov 25, 2024

View reviewed changes

src/cryptography/hazmat/primitives/serialization/pkcs7.py Outdated Show resolved Hide resolved

src/rust/src/pkcs7.rs Outdated Show resolved Hide resolved

final corrections?

0f39f75

nitneuqr added 5 commits November 26, 2024 10:51

first version of documentation

4cd74b8

some minor refactoring

corrected doctests

115eada

better indentation

8c8523b

doctest: added RSA private key

7b8a7c9

oops

d521ebd

alex approved these changes Nov 26, 2024

View reviewed changes

alex merged commit d6cac75 into pyca:main Nov 26, 2024
60 checks passed

alex mentioned this pull request Nov 26, 2024

Add S/MIME decryption #6263

Closed

nitneuqr deleted the smime-decryption branch November 26, 2024 14:21

alex mentioned this pull request Dec 7, 2024

support for PKCS7_NO_VERIFY in test_support.pkcs7_verify #12116

Closed

nitneuqr mentioned this pull request Jan 10, 2025

Add support for verifying S/MIME messages #12267

Open

		@@ -263,6 +263,81 @@ def encrypt(
		return rust_pkcs7.encrypt_and_serialize(self, encoding, options)


		def pkcs7_decrypt_der(

Add support for decrypting S/MIME messages #11555

Add support for decrypting S/MIME messages #11555

Uh oh!

Conversation

nitneuqr commented Sep 6, 2024

Uh oh!

alex commented Sep 6, 2024

Uh oh!

nitneuqr commented Sep 6, 2024

Uh oh!

nitneuqr commented Sep 8, 2024

Uh oh!

alex commented Sep 8, 2024

Uh oh!

Uh oh!

alex commented Sep 10, 2024

Uh oh!

nitneuqr commented Sep 26, 2024

Uh oh!

alex commented Oct 5, 2024

Uh oh!

alex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex commented Oct 28, 2024

Uh oh!

nitneuqr commented Nov 6, 2024

Uh oh!

alex commented Nov 6, 2024 via email

Uh oh!

Uh oh!

nitneuqr commented Nov 17, 2024

Uh oh!

alex commented Nov 17, 2024

Uh oh!

reaperhulk commented Nov 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitneuqr Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitneuqr Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nitneuqr Nov 18, 2024 •

edited

Loading

nitneuqr Nov 20, 2024 •

edited

Loading

nitneuqr Nov 18, 2024 •

edited

Loading

nitneuqr Nov 18, 2024 •

edited

Loading

nitneuqr Nov 19, 2024 •

edited

Loading

nitneuqr Nov 19, 2024 •

edited

Loading

nitneuqr Nov 19, 2024 •

edited

Loading