Ordering of SignerInfos

[vishalnayak]

I was trying to parse a pkcs7 signature of an AWS EC2 instance identity document. The Parse method was failing with a tag mismatch error.

After some digging, I realized that the function was expecting a certificate in place of signer information.

I made the below change and am able to successfully parse the signature now.

It would be great if this gets merged.

[gtank]

I've also run into a problem parsing EC2 identity documents with this library. How did you come to this fix, though? It does fix the parsing errors, but TestVerify is failing on your fork and RFC5652 claims that the ordering in signedData is correct.

My error was

asn1: structure error: tags don't match (16 vs {class:0 tag:2 length:1 isCompound:false}) {optional:false explicit:false application:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false} tbsCertificate @2

[fullsailor]

After digging into this, it looks like it might be an issue with the way that asn1.Unmarshal handles tagged fields. The signedData structure includes two optional fields for explicitly bundling certificates and CRLs so that the recipient doesn't need to have the certificate to verify the data. It only needs to verify the signer's certificate & trust its issuer. The signerInfos are non-optional and follow those two fields.

From a look at asn1.go in golang/go, the parseField() function is parsing RawValue types before checking that the asn1 tag matches the struct field's tag.

So in this case, asn1 is incorrectly putting the signerInfo data into the certificates RawValue blob. The RawValue type could be replaced with a specific data type, however the tbsCertificate type isn't exported from the x509 package.

I'm going to do some more poking around at it. We could probably mitigate the problem by checking for this condition before trying to run x509.ParseCertificates() on the raw value and instead parsing it as the set of signerInfos.

Let me know if you have any other thoughts or suggestions.

[vishalnayak]

@gtank I faced the exact same error you are noticing. I was using the AWS public key which they have provided in their documentation, which can be used to verify the instance_identity_document of EC2 instances.

I used openssl binary to see the signature's contents. It showed me the values that were certainly not belonging to x509 certificicate. But they were of exact match to signer information.

@fullsailor You are right. I guess this PR cannot be merged (rather should not be merged). The signedData I was parsing did not have the x509 certificate in it. Since I needed it badly, I had to fork this package and use it for the time being. Although, I am happy that this PR caught your attention. It will be of help if this gets fixed, so I can switch back to this repository instead of the forked one.

[vishalnayak]

@fullsailor Another thing I would like to bring to your attention is that the signature I was trying to parse had DSAWithSHA1 and not SHA1WithRSA.

Here (https://github.com/fullsailor/pkcs7/blob/master/pkcs7.go#L238), algo is getting hardcoded to SHA1WithRSA. It would be great if this is set based on the hashing algorithm identifier in the signature.

[gtank]

@vishalnayak I saw that too, but it turns out both values just set the hash to SHA1. The signature is based on key type: https://github.com/golang/go/blob/master/src/crypto/x509/x509.go#L682

[fullsailor]

Closing this in favor of the fix in PR #5. Thanks again for raising the issue.