Skip to content

Commit

Permalink
tls: move tls.parseCertString to end-of-life
Browse files Browse the repository at this point in the history
The internal use of tls.parseCertString was removed in
a336444. 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: nodejs#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>
  • Loading branch information
tniessen authored and Linkgoron committed Jan 31, 2022
1 parent db8e623 commit 9b7f32b
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 148 deletions.
27 changes: 10 additions & 17 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -1603,25 +1606,15 @@ changes:
description: Documentation-only deprecation.
-->

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()`

Expand Down
8 changes: 0 additions & 8 deletions lib/_tls_common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
35 changes: 0 additions & 35 deletions lib/internal/tls/parse-cert-string.js

This file was deleted.

7 changes: 0 additions & 7 deletions lib/tls.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 ' +
Expand Down
71 changes: 0 additions & 71 deletions test/parallel/test-tls-parse-cert-string.js

This file was deleted.

21 changes: 11 additions & 10 deletions test/parallel/test-tls-translate-peer-certificate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -23,27 +18,33 @@ 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 });
deepStrictEqual(translatePeerCertificate({ issuerCertificate: null }),
{ issuerCertificate: null });
deepStrictEqual(
translatePeerCertificate({ issuerCertificate: { subject: certString } }),
{ issuerCertificate: { subject: certObject } });
{ issuerCertificate: { subject: certString } });

{
const cert = {};
Expand Down

0 comments on commit 9b7f32b

Please sign in to comment.