Fix parsing content of PKCS#7-message if it is DER-data#474
Fix parsing content of PKCS#7-message if it is DER-data#474anthrax63 wants to merge 3 commits intodigitalbazaar:mainfrom
Conversation
dlongley
left a comment
dlongley
left a comment
There was a problem hiding this comment.
I think the issue sounds valid, but I don't think this is the proper solution. We'll need to poke around a bit with the test case and see what's going on. Clearly, you should be able to specify a byte buffer with DER bytes as the content, then convert to/from PEM and get that content back out. However, you shouldn't have to put it into an OCTETSTRING to make that happen; this would severely restrain the utility of forge's PKCS#7 implementation, making it such that it could not support any contentType that didn't call for an OCTETSTRING around the content.
| var content; | ||
| if(msg.content instanceof forge.util.ByteBuffer) { | ||
| content = msg.content.bytes(); | ||
| content = asn1.create(asn1.Class.UNIVERSAL, asn1.Type.OCTETSTRING, false, |
There was a problem hiding this comment.
I believe this would break the design here. The format for the content of PKCS#7 messages is dependent on the contentType. The current design allows for people to specify a ByteBuffer (or binary string of bytes) with DER-encoded content -- that expresses whatever ASN.1 format they desire. This would force the content to be an asn1.Type.OCTETSTRING, which is undesirable.
There was a problem hiding this comment.
Nevermind -- I see that the OCTETSTRING below was, in effect, moved up here (this is not a new one that wraps it).
|
I may have misread these changes ... looking back over it again. |
| forge.util.encodeUtf8(msg.content)); | ||
| } else if (typeof msg.content === 'object') { | ||
| content = asn1.create(asn1.Class.UNIVERSAL, asn1.Type.OCTETSTRING, true, | ||
| [msg.content]); |
There was a problem hiding this comment.
So, looking back at this code, this section of code only executes as a helper to automatically generate contentInfo in a common case. I think there was a presumption that the contentInfo would be generated externally unless msg.content is set to a byte buffer or a string. However, the conditional seems to allow this code to execute if msg.content is an object ... and I'm not sure why yet. I'll need to see if there is well defined behavior in this case in the documentation, but it seems to me that msg.content as an object could imply a number of different things and we can't simply pick to encode it like this (it can't be arbitrary).
In other words, if your msg.content is a string or a byte buffer, we have a well defined way in which it will be automatically wrapped. If you need to do something else, you should generate your own contentInfo. If a simple tweaking to this code better handles some other common cases that's fine, but we need to say explicitly what's going to happen if you pass in msg.content as an object -- and it's not yet clear to me that what's going on here is the "right" thing (or the highest utility thing) to do.
There was a problem hiding this comment.
I'm also not yet sure how my above comment meshes with parsing PKCS#7 messages, so I'll have to look back at that.
| p7 = PKCS7.messageFromPem(pem); | ||
| p7.decrypt(p7.recipients[0], PKI.privateKeyFromPem(_pem.privateKey)); | ||
| var asn1 = ASN1.fromDer(p7.content); | ||
| ASSERT.equal(asn1.value, 'Just a little test'); |
There was a problem hiding this comment.
This test case looks good from what I can tell. Will look more closely when I get a chance.
|
No activity since 2017? |
3 participants
If i use DER-data as a "content" of PKCS#7 message then after i parse it from PEM-format i get the ByteString with zero length instead of valid DER-bytes. This commit fixes that problem.