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

CmsSignedData: Don't assume the signed content is a Asn1OctetString object #63

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

CmsSignedData: Don't assume the signed content is a Asn1OctetString object #63

wants to merge 2 commits into from

Conversation

qmfrederik
Copy link

This PR fixes an issue in the CmsSignedData where the constructor would assume that the signed content is an Asn1OctetString object. That's not always true - in my case, the data was a DerSequence.

@peterdettman peterdettman self-assigned this Jun 23, 2016
@peterdettman
Copy link
Collaborator

The proposed change would change (break) the behaviour when the signed content is an Asn1OctetString, since GetOctets is not the same as GetEncoded.

The BC Java version has extra code to allow a sequence to be handled here, which would be reasonably straightforward to port over, but it is not quite as simple as calling GetEncoded on the sequence (in fact it appears to just encode the elements of the sequence). So I doubt if the proposed change is correct for sequences either.

It would be helpful if you could give more information on what type of files you are trying to work with, and if you know it, the relevant applicable RFC or other standards document that we could refer to.

@qmfrederik
Copy link
Author

Thanks for the feedback!

The files I'm working with are security catalogs for drivers (.cat files). Amongst others, Windows drivers ship with .cat files which contain the digital signature for the driver. An example of such a .cat file is androidwinusb86.zip.

As far as I understand, RFC 2315 section 5 does not place any restriction on the content:

The use of content types defined outside this document is possible, but is subject to bilateral agreement between parties exchanging content.

(The Java branch seems to refer to this content as "PKCS#7 ANY").

And hence, Microsoft defined the Certificate Trust List with OID 1.3.6.1.4.1.311.10.1, which is the content type for this file.

I've amended the PR to be backward compatibility. Let me know if you require additional information from my side!

@nmoinvaz
Copy link

I think RFC 5652 section 5.2.1, Compatibility with PKCS #7, applies here as well:

PKCS #7 defines content as:

content [0] EXPLICIT ANY DEFINED BY contentType OPTIONAL

The CMS defines eContent as:

eContent [0] EXPLICIT OCTET STRING OPTIONAL

@Genbox
Copy link

Genbox commented Oct 16, 2016

Authenticode signed files are PKCS#7 and not CMS, which will result in BC breaking inside CmsSignedDataParser. BC Java Beta (156b03) have implemented PKCS#7 support, which should solve this.

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

4 participants