OSSForks/node
1
0
Fork 0
mirror of https://github.com/zebrajr/node.git synced 2026-01-15 12:15:26 +00:00
Code Issues Actions 8 Packages Projects Releases Wiki Activity

tls: move tls.parseCertString to end-of-life

Browse Source 
The internal use of tls.parseCertString was removed in
a336444c7f. 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 <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This commit is contained in:
Tobias Nießen
2022-01-11 16:11:55 +00:00
parent f7be6ab042
commit 807c7e14f4
6 changed files with 21 additions and 148 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
doc/api/deprecations.md
View File

@@ -1595,6 +1595,9 @@ Type: End-of-Life
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/41479
description: End-of-Life.
- version: v9.0.0
pr-url: https://github.com/nodejs/node/pull/14249
description: Runtime deprecation.
@@ -1603,25 +1606,15 @@ changes:
description: Documentation-only deprecation.
-->
Type: Runtime
Type: End-of-Life
`tls.parseCertString()` is a trivial parsing helper that was made public by
mistake. This function can usually be replaced with:
`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.
```js
const querystring = require('querystring');
querystring.parse(str, '\n', '=');
```
This function is not completely equivalent to `querystring.parse()`. One
difference is that `querystring.parse()` does url decoding:
```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()`

8
lib/_tls_common.js
View File

@@ -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);

35
lib/internal/tls/parse-cert-string.js
View File

@@ -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;

7
lib/tls.js
View File

@@ -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 ' +

71
test/parallel/test-tls-parse-cert-string.js
View File

@@ -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' });
}

21
test/parallel/test-tls-translate-peer-certificate.js
View File

@@ -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 = {};