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

Add support for rfc2633 sMIME capabilities signed attr #215

Merged
merged 4 commits into from Mar 5, 2022
Merged

Add support for rfc2633 sMIME capabilities signed attr #215

merged 4 commits into from Mar 5, 2022

Conversation

Hellzed
Copy link
Contributor

@Hellzed Hellzed commented Sep 14, 2021

Hi,
I'm using asn1crypto on a project that requires support for S/MIME Version 3 (rfc2633), especially the sMIME capabilities signed attribute.
Right now, asn1crypto master only parses the sMIME capabilities signed attr into universal types (a set of sequences) with raw OIDs.

This also exposes some (probably buggy) behaviour from asn1crypto, where unknown signed attrs such as sMIME capabilities are empty and hoisted at the beginning of the signed attrs field when dumping.
For an example, see this PKCS#7 signature before and after being parsed by asn1crypto (look for the sMIMECapabilities field and see how it was 1) moved, 2) emptied).

This pull request adds support for rfc2633 sMIME capabilities signed attr, to allow parsing it as a set of sequences of algorithm identifiers (as per the spec). This prevents the faulty parsing and dumping behaviour, only for this signed attr.

I need to add a couple of unit tests, could you provide guidance about this? I think new fixtures need to be generated for the CMS tests, but I'm not sure what's the most appropriate way.

Thank you for this incredibly useful library.

@wbond
Copy link
Owner

wbond commented Feb 12, 2022

I fixed the bug you identified in 5a24aed. It was that if you try and force DER re-encoding of a Sequence that has no field info, it resulted in an empty value since it didn't known the canonical encoding.

Now if it hits such a situation, it preserves the existing encoding.

@wbond
Copy link
Owner

wbond commented Feb 12, 2022

In terms of tests, ideally you can just add a fixture with a value encoded by another program and ensure asn1crypto can parse it.

@Hellzed
Copy link
Contributor Author

Hellzed commented Feb 13, 2022

I've added a sMIME capabilities parsing test on a PKCS#7 signature generated by Thunderbird 91.5.0.

I have also updated the cms module to better reflect sMIME capabilities semantics as per rfc2633:

  • not all capability OIDs are AlgorithmIds (some are compression or encoding-related), and even when a capability OID is identical to an AlgorithmId, the associated params field may not be structured in the same way as expected by a full AlgorithmIdentifier (e.g.: RC2 capability params value is only the key length, not algos.Rc2Params). This requires a new, different class cms.SMIMECapabilityIdentifier without the checks enforced by algos.AnyAlgorithmIdentifier.
  • the sMIME capabilities attribute value is a SET OF containing a single instance of a sequence (the capabilities themselves). This is apparent in the test.

A nice extra would have been to provide the algorithm name as capability_id attribute (when using SMIMECapabilityIdentifier.native) if the OID matches a known algorithm, but this doesn't seem actually useful for now.

@wbond
Copy link
Owner

wbond commented Mar 5, 2022

This looks good - thanks! I have a commit to add the algorithm names, but for some reason GitHub isn't letting me push to your master branch even though the PR says I can. I'll add it after I merge.

@wbond wbond merged commit 976dbba into wbond:master Mar 5, 2022
@wbond
Copy link
Owner

wbond commented Mar 5, 2022

You can see my tweaks at 4791c1e

@Hellzed
Copy link
Contributor Author

Hellzed commented Mar 6, 2022

Awesome, thank you!

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.

None yet

2 participants