search

fullsailor / pkcs7

Implements a subset of PKCS#7/Crytpographic Message Syntax (rfc2315, rfc5652)
MIT License
123 stars 201 forks source link

Problems parsing some BER encoding #11

Closed isnotnick closed 7 years ago

isnotnick commented 7 years ago

Thanks so much for this package, it's honestly saved me a lot of headache!

However - I've come across some BER encodings that it's having problems with, notably those with indefinite length encodings. The bottom-up search for the EOC blocks seems to fail, I think.

I've attached a sample - it's a SCEP request. (Generated, I think, by the JSCEP library - I've been toying with building a SCEP server for some time, and most other clients like iOS, SSCEP and so on don't seem to do the encoding quite like this does. The request is actually from a major MDM solution, but given the errors I've seen, I'm confident it's using JSCEP).

BER.txt

fullsailor commented 7 years ago

Hey Nick, your kind message means a lot. Thanks.

As for BER... yes, its problematic. lstoll also brought up an issue (#10) that sounds suspiciously the same. He wasn't able to provide a sample, but using yours I can reproduce and work out a fix. I can't recall why the code does a bottom-up search, but you may of pin-pointed the issue.

Thanks again!

isnotnick commented 7 years ago

Wanted to add an update here, as I've been beating my head against this for a couple of weeks now!

I did hack something together that seems to work at handling the indefinite-length objects, but I've not tested enough yet. Essentially I run 'readObject' twice - the first 'pass' only really cares about finding the indefinite-length markers and EOCs.

The total number of indef markers and their offsets, as well as the EOCs and offsets are stored in two slices.

If an indef is found, but no matching EOC is (bytes.LastIndex call) then instead of returning an error, it searches 'back', at lower offsets than the indef offset.

Then, the slices are re-ordered to pair up indef-length indicators and EOC indicators.

Finally, the main 'readObject' is run, and each time an indef is encountered, the offset of the next EOC is used.

This seems to work, but I've now got another problem...

asn1.Unmarshal doesn't seem to correctly handle compound octet-strings. Line 170 in pkcs7.go attempts to unmarshal everything, but I think it's only doing the first octet-string that's generally 1000 bytes.

isnotnick commented 7 years ago

Another update.

My final statement in that asn1.Unmarshal doesn't appear to handle 'compound' octet-strings appears to be true. I put the two octet-strings comprising the pkcs7-data with 1200+ bytes in and get only the first 1000 out.

Your code correctly 'detects' the compound, so I'm testing a hack which - if there's only 1000 bytes returned and the input is larger - unmarshals and appends the rest of the octet-string. It seems to work, and my testing with the major MDM provider is proving successful.

I'll tidy up the code for the BER-to-DER conversion and the unmarshaling of compound octet-strings and send you a pull request to review over the next few days.

fullsailor commented 7 years ago

Interesting that it detects the compound but doesn't rewrite it. I don't think DER allows compound strings, so that would be why Go's asn1 package won't parse it.

Any love you can give to the BER-to-DER code would be much appreciated.

erning commented 7 years ago

any update on this issue?

Vespira commented 7 years ago

Hi there,

I'm using micromdm/scep server and it uses pkcs7 lib as a dependency. At some point, when I send a CSR to enrol my Android client into the scep server, I have a ber2der: Invalid BER format error.

I created an issue (which is maybe a bug, maybe not ? we don't really know) here : https://github.com/micromdm/scep/issues/20

And if someone make any progress on this, can you please post an answer ? it would be great ! I'm not a cryptography-pro, I try to learn, and dive deep to debug the unmarshalling process, and why it see it as invalid format, but so far I didn't find a cause.

fullsailor commented 7 years ago

I've merged #17 which we expect fixes this issue with indefinite lengths in BER encoding.

mikkeloscar commented 7 years ago

As reported by others we also saw AWS identity documents sometimes failing with the error: ber2der: BER tag length is more than available data.

After updating to latest master version it no longer fails on the one test document I had :+1:

Thanks!

fullsailor commented 7 years ago

Super! Thank you for testing.