Support PEM BER data with unparsed trailing data.#977
Open
Support PEM BER data with unparsed trailing data.#977
Conversation
- [asn1] Add `fromPemBer()` call that is more permissive than `fromDer()` and allows trailing data. - [RFC 7468](https://www.rfc-editor.org/rfc/rfc7468) PEM data is BER encoded. The RFC recommends to prefer DER over BER encoding throughout and is described in Appendix B. - PKCS#7 PEM data with trailing zeros appears in the wild. This may be intentional, but unneeded, padding. In any case, it should be accepted. - Recent `node-forge` releases made `fromDer` more strict to by default throw an error when not all data is decoded. `fromDer` is used in many places even for BER data, but in this case an alternate is needed to allow for at least this trailing data. - The API is named `fromPemBer` rather than `fromBer` since it is currently intended to handle only the subset of PEM BER that is DER data followed by possible unparsed bytes. - **NOTE**: This API may not handle all PEM BER data. If other data in wild is found that needs better support please file an issue with an example. - Calls to `asn1.fromDer` that occurred on data from `pem.decode()`, or similar, now use `asn1.fromPemBer` to be more permissive in allowing possible trailing or padding bytes. - [asn1] `fromDer` error message changed to reflect also using the API with BER data. Changed from `'Unparsed DER bytes remain after ASN.1 parsing.'` to `'Unparsed bytes remain after ASN.1 parsing.'`.
dlongley
requested changes
Apr 22, 2022
Member
dlongley
left a comment
dlongley
left a comment
There was a problem hiding this comment.
I recommend we add this feature in a different spot -- otherwise, it looks ok.
| * | ||
| * @return the parsed asn1 object. | ||
| */ | ||
| asn1.fromPemBer = function(bytes, options) { |
Member
There was a problem hiding this comment.
This seems to break abstractions a little too much for my tastes. I understand the reasoning. I think that adding asn1.fromBer would be acceptable -- but we've always tried to avoid implementing an entire BER parser because it's usually not necessary, it's complicated, and a much bigger lift ... so that's not going to happen here.
Since this is PEM-specific, I would recommend that we add this utility only to pem.js instead. Perhaps renamed to pem.toAsn1() or pem.berToAsn1() or similar? Thoughts?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The hope is this less strict parsing doesn't reintroduce the issues fixed in 1.3.x. I think this method of accepting unparsed trailing bytes for PEM data, still parsing by default as DER, and with the additional checks in 1.3.x, this should be ok.