From 9b7f32b234f8b2e648d75ab75d5569057eea1ef5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 11 Jan 2022 16:11:55 +0000 Subject: [PATCH] tls: move tls.parseCertString to end-of-life The internal use of tls.parseCertString was removed in a336444c7fb9fd1d0055481d84cdd57d7d569879. The function does not handle multi-value RDNs correctly, leading to incorrect representations and security concerns. This change is breaking in two ways: tls.parseCertString is removed (but has been runtime-deprecated since Node.js 9) and _tls_common.translatePeerCertificate does not translate the `subject` and `issuer` properties anymore. This change also removes the recommendation to use querystring.parse instead, which is similarly dangerous. PR-URL: https://github.com/nodejs/node/pull/41479 Reviewed-By: James M Snell Reviewed-By: Benjamin Gruenbaum Reviewed-By: Matteo Collina Reviewed-By: Richard Lau Reviewed-By: Colin Ihrig --- doc/api/deprecations.md | 27 +++---- lib/_tls_common.js | 8 --- lib/internal/tls/parse-cert-string.js | 35 --------- lib/tls.js | 7 -- test/parallel/test-tls-parse-cert-string.js | 71 ------------------- .../test-tls-translate-peer-certificate.js | 21 +++--- 6 files changed, 21 insertions(+), 148 deletions(-) delete mode 100644 lib/internal/tls/parse-cert-string.js delete mode 100644 test/parallel/test-tls-parse-cert-string.js diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 31eb7209dce89b..2a879e693aefd1 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -1595,6 +1595,9 @@ Type: End-of-Life -Type: Runtime - -`tls.parseCertString()` is a trivial parsing helper that was made public by -mistake. This function can usually be replaced with: - -```js -const querystring = require('querystring'); -querystring.parse(str, '\n', '='); -``` +Type: End-of-Life -This function is not completely equivalent to `querystring.parse()`. One -difference is that `querystring.parse()` does url decoding: +`tls.parseCertString()` was a trivial parsing helper that was made public by +mistake. While it was supposed to parse certificate subject and issuer strings, +it never handled multi-value Relative Distinguished Names correctly. -```console -> querystring.parse('%E5%A5%BD=1', '\n', '='); -{ '好': '1' } -> tls.parseCertString('%E5%A5%BD=1'); -{ '%E5%A5%BD': '1' } -``` +Earlier versions of this document suggested using `querystring.parse()` as an +alternative to `tls.parseCertString()`. However, `querystring.parse()` also does +not handle all certificate subjects correctly and should not be used. ### DEP0077: `Module._debug()` diff --git a/lib/_tls_common.js b/lib/_tls_common.js index 533fbe7fd256e3..79d3ac1136ec99 100644 --- a/lib/_tls_common.js +++ b/lib/_tls_common.js @@ -55,10 +55,6 @@ const { configSecureContext, } = require('internal/tls/secure-context'); -const { - parseCertString, -} = require('internal/tls/parse-cert-string'); - function toV(which, v, def) { if (v == null) v = def; if (v === 'TLSv1') return TLS1_VERSION; @@ -126,13 +122,9 @@ function translatePeerCertificate(c) { if (!c) return null; - // TODO(tniessen): can we remove parseCertString without breaking anything? - if (typeof c.issuer === 'string') c.issuer = parseCertString(c.issuer); if (c.issuerCertificate != null && c.issuerCertificate !== c) { c.issuerCertificate = translatePeerCertificate(c.issuerCertificate); } - // TODO(tniessen): can we remove parseCertString without breaking anything? - if (typeof c.subject === 'string') c.subject = parseCertString(c.subject); if (c.infoAccess != null) { const info = c.infoAccess; c.infoAccess = ObjectCreate(null); diff --git a/lib/internal/tls/parse-cert-string.js b/lib/internal/tls/parse-cert-string.js deleted file mode 100644 index a499df886097b4..00000000000000 --- a/lib/internal/tls/parse-cert-string.js +++ /dev/null @@ -1,35 +0,0 @@ -'use strict'; - -const { - ArrayIsArray, - ArrayPrototypeForEach, - ArrayPrototypePush, - StringPrototypeIndexOf, - StringPrototypeSlice, - StringPrototypeSplit, - ObjectCreate, -} = primordials; - -// Example: -// C=US\nST=CA\nL=SF\nO=Joyent\nOU=Node.js\nCN=ca1\nemailAddress=ry@clouds.org -function parseCertString(s) { - const out = ObjectCreate(null); - ArrayPrototypeForEach(StringPrototypeSplit(s, '\n'), (part) => { - const sepIndex = StringPrototypeIndexOf(part, '='); - if (sepIndex > 0) { - const key = StringPrototypeSlice(part, 0, sepIndex); - const value = StringPrototypeSlice(part, sepIndex + 1); - if (key in out) { - if (!ArrayIsArray(out[key])) { - out[key] = [out[key]]; - } - ArrayPrototypePush(out[key], value); - } else { - out[key] = value; - } - } - }); - return out; -} - -exports.parseCertString = parseCertString; diff --git a/lib/tls.js b/lib/tls.js index f17a2fd0c5b0aa..de20505fde242b 100644 --- a/lib/tls.js +++ b/lib/tls.js @@ -64,7 +64,6 @@ const { canonicalizeIP } = internalBinding('cares_wrap'); const _tls_common = require('_tls_common'); const _tls_wrap = require('_tls_wrap'); const { createSecurePair } = require('internal/tls/secure-pair'); -const { parseCertString } = require('internal/tls/parse-cert-string'); // Allow {CLIENT_RENEG_LIMIT} client-initiated session renegotiations // every {CLIENT_RENEG_WINDOW} seconds. An error event is emitted if more @@ -338,12 +337,6 @@ exports.Server = _tls_wrap.Server; exports.createServer = _tls_wrap.createServer; exports.connect = _tls_wrap.connect; -exports.parseCertString = internalUtil.deprecate( - parseCertString, - 'tls.parseCertString() is deprecated. ' + - 'Please use querystring.parse() instead.', - 'DEP0076'); - exports.createSecurePair = internalUtil.deprecate( createSecurePair, 'tls.createSecurePair() is deprecated. Please use ' + diff --git a/test/parallel/test-tls-parse-cert-string.js b/test/parallel/test-tls-parse-cert-string.js deleted file mode 100644 index c1f32524d578b5..00000000000000 --- a/test/parallel/test-tls-parse-cert-string.js +++ /dev/null @@ -1,71 +0,0 @@ -/* eslint-disable no-proto */ -'use strict'; - -const common = require('../common'); -if (!common.hasCrypto) - common.skip('missing crypto'); - -const { - hijackStderr, - restoreStderr -} = require('../common/hijackstdio'); -const assert = require('assert'); -// Flags: --expose-internals -const { parseCertString } = require('internal/tls/parse-cert-string'); -const tls = require('tls'); - -const noOutput = common.mustNotCall(); -hijackStderr(noOutput); - -{ - const singles = 'C=US\nST=CA\nL=SF\nO=Node.js Foundation\nOU=Node.js\n' + - 'CN=ca1\nemailAddress=ry@clouds.org'; - const singlesOut = parseCertString(singles); - assert.deepStrictEqual(singlesOut, { - __proto__: null, - C: 'US', - ST: 'CA', - L: 'SF', - O: 'Node.js Foundation', - OU: 'Node.js', - CN: 'ca1', - emailAddress: 'ry@clouds.org' - }); -} - -{ - const doubles = 'OU=Domain Control Validated\nOU=PositiveSSL Wildcard\n' + - 'CN=*.nodejs.org'; - const doublesOut = parseCertString(doubles); - assert.deepStrictEqual(doublesOut, { - __proto__: null, - OU: [ 'Domain Control Validated', 'PositiveSSL Wildcard' ], - CN: '*.nodejs.org' - }); -} - -{ - const invalid = 'fhqwhgads'; - const invalidOut = parseCertString(invalid); - assert.deepStrictEqual(invalidOut, { __proto__: null }); -} - -{ - const input = '__proto__=mostly harmless\nhasOwnProperty=not a function'; - const expected = Object.create(null); - expected.__proto__ = 'mostly harmless'; - expected.hasOwnProperty = 'not a function'; - assert.deepStrictEqual(parseCertString(input), expected); -} - -restoreStderr(); - -{ - common.expectWarning('DeprecationWarning', - 'tls.parseCertString() is deprecated. ' + - 'Please use querystring.parse() instead.', - 'DEP0076'); - - const ret = tls.parseCertString('foo=bar'); - assert.deepStrictEqual(ret, { __proto__: null, foo: 'bar' }); -} diff --git a/test/parallel/test-tls-translate-peer-certificate.js b/test/parallel/test-tls-translate-peer-certificate.js index f8499e0c7e84ff..a14308ab87b762 100644 --- a/test/parallel/test-tls-translate-peer-certificate.js +++ b/test/parallel/test-tls-translate-peer-certificate.js @@ -9,11 +9,6 @@ const { strictEqual, deepStrictEqual } = require('assert'); const { translatePeerCertificate } = require('_tls_common'); const certString = '__proto__=42\nA=1\nB=2\nC=3'; -const certObject = Object.create(null); -certObject.__proto__ = '42'; -certObject.A = '1'; -certObject.B = '2'; -certObject.C = '3'; strictEqual(translatePeerCertificate(null), null); strictEqual(translatePeerCertificate(undefined), null); @@ -23,19 +18,25 @@ strictEqual(translatePeerCertificate(1), 1); deepStrictEqual(translatePeerCertificate({}), {}); +// Earlier versions of Node.js parsed the issuer property but did so +// incorrectly. This behavior has now reached end-of-life and user-supplied +// strings will not be parsed at all. deepStrictEqual(translatePeerCertificate({ issuer: '' }), - { issuer: Object.create(null) }); + { issuer: '' }); deepStrictEqual(translatePeerCertificate({ issuer: null }), { issuer: null }); deepStrictEqual(translatePeerCertificate({ issuer: certString }), - { issuer: certObject }); + { issuer: certString }); +// Earlier versions of Node.js parsed the issuer property but did so +// incorrectly. This behavior has now reached end-of-life and user-supplied +// strings will not be parsed at all. deepStrictEqual(translatePeerCertificate({ subject: '' }), - { subject: Object.create(null) }); + { subject: '' }); deepStrictEqual(translatePeerCertificate({ subject: null }), { subject: null }); deepStrictEqual(translatePeerCertificate({ subject: certString }), - { subject: certObject }); + { subject: certString }); deepStrictEqual(translatePeerCertificate({ issuerCertificate: '' }), { issuerCertificate: null }); @@ -43,7 +44,7 @@ deepStrictEqual(translatePeerCertificate({ issuerCertificate: null }), { issuerCertificate: null }); deepStrictEqual( translatePeerCertificate({ issuerCertificate: { subject: certString } }), - { issuerCertificate: { subject: certObject } }); + { issuerCertificate: { subject: certString } }); { const cert = {};