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

Support PEM BER data with unparsed trailing data. #977

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

Conversation

davidlehn
Copy link
Member

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.

- [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.'`.
Copy link
Member

@dlongley dlongley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend we add this feature in a different spot -- otherwise, it looks ok.

*
* @return the parsed asn1 object.
*/
asn1.fromPemBer = function(bytes, options) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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.

2 participants