From 4667c7c6559093a4d151e5e4a6a0f10af7e4c52e Mon Sep 17 00:00:00 2001 From: "David I. Lehn" Date: Thu, 21 Apr 2022 18:08:04 -0400 Subject: [PATCH 1/2] Test on Node.js 16.x. --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index a63a29963..7eb8b574a 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -8,7 +8,7 @@ jobs: timeout-minutes: 10 strategy: matrix: - node-version: [6.x, 8.x, 10.x, 12.x, 14.x] + node-version: [6.x, 8.x, 10.x, 12.x, 14.x, 16.x] steps: - uses: actions/checkout@v2 - name: Use Node.js ${{ matrix.node-version }} From b4fbc010f1ad451b2fdacc9fa32007743e56ae13 Mon Sep 17 00:00:00 2001 From: "David I. Lehn" Date: Thu, 21 Apr 2022 20:31:54 -0400 Subject: [PATCH 2/2] Support PEM BER data with unparsed trailing data. - [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.'`. --- CHANGELOG.md | 30 ++++++++++++++++++++++++++++++ lib/asn1.js | 43 ++++++++++++++++++++++++++++++++++++++++++- lib/pbe.js | 6 +++--- lib/pkcs12.js | 3 ++- lib/pkcs7.js | 4 ++-- lib/pki.js | 4 ++-- lib/tls.js | 2 +- lib/x509.js | 12 ++++++------ tests/unit/pkcs7.js | 28 ++++++++++++++++++++++++++++ tests/unit/rsa.js | 2 +- 10 files changed, 117 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 27d0e3a00..306484e00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,36 @@ Forge ChangeLog =============== +## 1.4.0 - 2022-xx-xx + +### Added +- [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. + +### Fixes +- 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. + +### Changed +- [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.'`. + ## 1.3.1 - 2022-03-29 ### Fixes diff --git a/lib/asn1.js b/lib/asn1.js index 4025f8a9e..a9fe561a6 100644 --- a/lib/asn1.js +++ b/lib/asn1.js @@ -457,7 +457,7 @@ asn1.fromDer = function(bytes, options) { var byteCount = bytes.length(); var value = _fromDer(bytes, bytes.length(), 0, options); if(options.parseAllBytes && bytes.length() !== 0) { - var error = new Error('Unparsed DER bytes remain after ASN.1 parsing.'); + var error = new Error('Unparsed bytes remain after ASN.1 parsing.'); error.byteCount = byteCount; error.remaining = bytes.length(); throw error; @@ -465,6 +465,47 @@ asn1.fromDer = function(bytes, options) { return value; }; +/** + * Parses an asn1 object from a byte buffer in PEM BER format as DER. + * + * RFC 7468 specifies PEM data is BER encoded. This call will decode data using + * `fromDer` but allow trailing data. This will allow a subset of BER data that + * appears in the wild with trailing data. Note that RFC 7468 recommends + * to prefer DER over BER encoding as mentioned throughout and described in + * Appendix B. + * + * @reference https://www.rfc-editor.org/rfc/rfc7468 + * + * @param bytes the byte buffer to parse from. + * @param [options] object with options or boolean strict flag + * [strict] true to be strict when checking value lengths, false to + * allow truncated values (default: true). + * [parseAllBytes] true to ensure all bytes are parsed + * (default: false) + * [decodeBitStrings] true to attempt to decode the content of + * BIT STRINGs (not OCTET STRINGs) using strict mode. Note that + * without schema support to understand the data context this can + * erroneously decode values that happen to be valid ASN.1. This + * flag will be deprecated or removed as soon as schema support is + * available. (default: true) + * + * @throws Will throw an error for various malformed input conditions. + * + * @return the parsed asn1 object. + */ +asn1.fromPemBer = function(bytes, options) { + if(options === undefined) { + options = { + parseAllBytes: false + }; + } + if(!('parseAllBytes' in options)) { + options.parseAllBytes = false; + } + + return asn1.fromDer(bytes, options); +}; + /** * Internal function to parse an asn1 object from a byte buffer in DER format. * diff --git a/lib/pbe.js b/lib/pbe.js index cf8456ba6..8f0f729b8 100644 --- a/lib/pbe.js +++ b/lib/pbe.js @@ -421,8 +421,8 @@ pki.encryptedPrivateKeyFromPem = function(pem) { 'PEM is encrypted.'); } - // convert DER to ASN.1 object - return asn1.fromDer(msg.body); + // convert BER to ASN.1 object + return asn1.fromPemBer(msg.body); }; /** @@ -616,7 +616,7 @@ pki.decryptRsaPrivateKey = function(pem, password) { rval = pki.decryptPrivateKeyInfo(asn1.fromDer(rval), password); } else { // decryption already performed above - rval = asn1.fromDer(rval); + rval = asn1.fromPemBer(rval); } if(rval !== null) { diff --git a/lib/pkcs12.js b/lib/pkcs12.js index cd06c494a..c901bf506 100644 --- a/lib/pkcs12.js +++ b/lib/pkcs12.js @@ -517,7 +517,8 @@ function _decodePkcs7Data(data) { * @param {String} password Password to decrypt with (optional). */ function _decodeAuthenticatedSafe(pfx, authSafe, strict, password) { - authSafe = asn1.fromDer(authSafe, strict); /* actually it's BER encoded */ + // actually it's BER encoded + authSafe = asn1.fromDer(authSafe, strict); if(authSafe.tagClass !== asn1.Class.UNIVERSAL || authSafe.type !== asn1.Type.SEQUENCE || diff --git a/lib/pkcs7.js b/lib/pkcs7.js index 3a5d845c5..0ae766cf7 100644 --- a/lib/pkcs7.js +++ b/lib/pkcs7.js @@ -53,8 +53,8 @@ p7.messageFromPem = function(pem) { throw new Error('Could not convert PKCS#7 message from PEM; PEM is encrypted.'); } - // convert DER to ASN.1 object - var obj = asn1.fromDer(msg.body); + // convert BER to ASN.1 object + var obj = asn1.fromPemBer(msg.body); return p7.messageFromAsn1(obj); }; diff --git a/lib/pki.js b/lib/pki.js index ee82ff1ce..f4b7e2f51 100644 --- a/lib/pki.js +++ b/lib/pki.js @@ -61,8 +61,8 @@ pki.privateKeyFromPem = function(pem) { throw new Error('Could not convert private key from PEM; PEM is encrypted.'); } - // convert DER to ASN.1 object - var obj = asn1.fromDer(msg.body); + // convert BER to ASN.1 object + var obj = asn1.fromPemBer(msg.body); return pki.privateKeyFromAsn1(obj); }; diff --git a/lib/tls.js b/lib/tls.js index fadfd646f..ce0c01221 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -2903,7 +2903,7 @@ tls.createCertificate = function(c) { var der = forge.util.createBuffer(msg.body); if(asn1 === null) { - asn1 = forge.asn1.fromDer(der.bytes(), false); + asn1 = forge.asn1.fromPemBer(der.bytes(), {strict: false}); } // certificate entry is itself a vector with 3 length bytes diff --git a/lib/x509.js b/lib/x509.js index 2877810c1..07c593410 100644 --- a/lib/x509.js +++ b/lib/x509.js @@ -816,8 +816,8 @@ pki.certificateFromPem = function(pem, computeHash, strict) { 'Could not convert certificate from PEM; PEM is encrypted.'); } - // convert DER to ASN.1 object - var obj = asn1.fromDer(msg.body, strict); + // convert BER to ASN.1 object + var obj = asn1.fromPemBer(msg.body, {strict: strict}); return pki.certificateFromAsn1(obj, computeHash); }; @@ -859,8 +859,8 @@ pki.publicKeyFromPem = function(pem) { throw new Error('Could not convert public key from PEM; PEM is encrypted.'); } - // convert DER to ASN.1 object - var obj = asn1.fromDer(msg.body); + // convert BER to ASN.1 object + var obj = asn1.fromPemBer(msg.body); return pki.publicKeyFromAsn1(obj); }; @@ -977,8 +977,8 @@ pki.certificationRequestFromPem = function(pem, computeHash, strict) { 'PEM is encrypted.'); } - // convert DER to ASN.1 object - var obj = asn1.fromDer(msg.body, strict); + // convert BER to ASN.1 object + var obj = asn1.fromPemBer(msg.body, {strict: strict}); return pki.certificationRequestFromAsn1(obj, computeHash); }; diff --git a/tests/unit/pkcs7.js b/tests/unit/pkcs7.js index e99c13867..8d478eed6 100644 --- a/tests/unit/pkcs7.js +++ b/tests/unit/pkcs7.js @@ -24,6 +24,22 @@ var UTIL = require('../../lib/util'); 'gEAm2mfSF5xFPLEqqFkvKTM4w8PfhnF0ehmfQNApvoWQRQanNWLCT+Q9GHx6DCFj\r\n' + 'TUHl+53x88BrCl1E7FhYPs92\r\n' + '-----END PKCS7-----\r\n', + p7ZeroPadded: + '-----BEGIN PKCS7-----\r\n' + + 'MIICTgYJKoZIhvcNAQcDoIICPzCCAjsCAQAxggHGMIIBwgIBADCBqTCBmzELMAkG\r\n' + + 'A1UEBhMCREUxEjAQBgNVBAgMCUZyYW5jb25pYTEQMA4GA1UEBwwHQW5zYmFjaDEV\r\n' + + 'MBMGA1UECgwMU3RlZmFuIFNpZWdsMRIwEAYDVQQLDAlHZWllcmxlaW4xFjAUBgNV\r\n' + + 'BAMMDUdlaWVybGVpbiBERVYxIzAhBgkqhkiG9w0BCQEWFHN0ZXNpZUBicm9rZW5w\r\n' + + 'aXBlLmRlAgkA1FQcQNg14vMwDQYJKoZIhvcNAQEBBQAEggEAJhWQz5SniCd1w3A8\r\n' + + 'uKVZEfc8Tp21I7FMfFqou+UOVsZCq7kcEa9uv2DIj3o7zD8wbLK1fuyFi4SJxTwx\r\n' + + 'kR0a6V4bbonIpXPPJ1f615dc4LydAi2tv5w14LJ1Js5XCgGVnkAmQHDaW3EHXB7X\r\n' + + 'T4w9PR3+tcS/5YAnWaM6Es38zCKHd7TnHpuakplIkwSK9rBFAyA1g/IyTPI+ktrE\r\n' + + 'EHcVuJcz/7eTlF6wJEa2HL8F1TVWuL0p/0GsJP/8y0MYGdCdtr+TIVo//3YGhoBl\r\n' + + 'N4tnheFT/jRAzfCZtflDdgAukW24CekrJ1sG2M42p5cKQ5rGFQtzNy/n8EjtUutO\r\n' + + 'HD5YITBsBgkqhkiG9w0BBwEwHQYJYIZIAWUDBAEqBBBmlpfy3WrYj3uWW7+xNEiH\r\n' + + 'gEAm2mfSF5xFPLEqqFkvKTM4w8PfhnF0ehmfQNApvoWQRQanNWLCT+Q9GHx6DCFj\r\n' + + 'TUHl+53x88BrCl1E7FhYPs92AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA\r\n' + + '-----END PKCS7-----\r\n', certificate: '-----BEGIN CERTIFICATE-----\r\n' + 'MIIDtDCCApwCCQDUVBxA2DXi8zANBgkqhkiG9w0BAQUFADCBmzELMAkGA1UEBhMC\r\n' + @@ -412,6 +428,18 @@ var UTIL = require('../../lib/util'); ASSERT.equal(p7.encryptedContent.parameter.data.length, 16); // IV }); + it('should import padded message from PEM', function() { + ASSERT.doesNotThrow(function() { + var p7 = PKCS7.messageFromPem(_pem.p7); + var p7ZeroPadded = PKCS7.messageFromPem(_pem.p7ZeroPadded); + ASSERT.deepEqual(p7.type, p7ZeroPadded.type); + ASSERT.deepEqual(p7.version, p7ZeroPadded.version); + ASSERT.deepEqual(p7.recipients, p7ZeroPadded.recipients); + ASSERT.deepEqual(p7.encryptedContent, p7ZeroPadded.encryptedContent); + ASSERT.deepEqual(p7.rawCapture, p7ZeroPadded.rawCapture); + }); + }); + it('should import indefinite length message from PEM', function() { ASSERT.doesNotThrow(function() { var p7 = PKCS7.messageFromPem(_pem.p7IndefiniteLength); diff --git a/tests/unit/rsa.js b/tests/unit/rsa.js index b1c1b64aa..9f9f6f4d1 100644 --- a/tests/unit/rsa.js +++ b/tests/unit/rsa.js @@ -830,7 +830,7 @@ var UTIL = require('../../lib/util'); ASSERT.throws(function() { publicKey.verify(md.digest().getBytes(), S); }, - /^Error: Unparsed DER bytes remain after ASN.1 parsing.$/); + /^Error: Unparsed bytes remain after ASN.1 parsing.$/); } function _checkBadDigestInfo(publicKey, S, skipTailingGarbage) {