From 6c7fff6f1d53dfb6c2b184ee41809b8d7614cb80 Mon Sep 17 00:00:00 2001
From: Matteo Collina <hello@matteocollina.com>
Date: Wed, 4 Aug 2021 18:40:00 +0200
Subject: [PATCH] tls: validate "rejectUnauthorized: undefined"

Incomplete validation of rejectUnauthorized parameter (Low)

If the Node.js https API was used incorrectly and "undefined" was passed
in for the "rejectUnauthorized" parameter, no error was returned and
connections to servers with an expired certificate would have been
accepted.

CVE-ID: CVE-2021-22939
Refs: https://nvd.nist.gov/vuln/detail/CVE-2021-22939
Refs: https://hackerone.com/reports/1278254
PR-URL: https://github.com/nodejs-private/node-private/pull/276
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Akshay K <iit.akshay@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
---
 lib/_tls_wrap.js                        | 17 ++++++++++++++++-
 test/parallel/test-tls-client-reject.js | 13 +++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index f596614af5fef9..57399c602a10bb 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -1544,7 +1544,15 @@ function onConnectSecure() {
     this.authorized = false;
     this.authorizationError = verifyError.code || verifyError.message;
 
-    if (options.rejectUnauthorized) {
+    // rejectUnauthorized property can be explicitly defined as `undefined`
+    // causing the assignment to default value (`true`) fail. Before assigning
+    // it to the tlssock connection options, explicitly check if it is false
+    // and update rejectUnauthorized property. The property gets used by
+    // TLSSocket connection handler to allow or reject connection if
+    // unauthorized.
+    // This check is potentially redundant, however it is better to keep it
+    // in case the option object gets modified somewhere.
+    if (options.rejectUnauthorized !== false) {
       this.destroy(verifyError);
       return;
     }
@@ -1629,6 +1637,13 @@ exports.connect = function connect(...args) {
     signal: options.signal,
   });
 
+  // rejectUnauthorized property can be explicitly defined as `undefined`
+  // causing the assignment to default value (`true`) fail. Before assigning
+  // it to the tlssock connection options, explicitly check if it is false
+  // and update rejectUnauthorized property. The property gets used by TLSSocket
+  // connection handler to allow or reject connection if unauthorized
+  options.rejectUnauthorized = options.rejectUnauthorized !== false;
+
   tlssock[kConnectOptions] = options;
 
   if (cb)
diff --git a/test/parallel/test-tls-client-reject.js b/test/parallel/test-tls-client-reject.js
index d41ad806ea3012..ea10ef6da20834 100644
--- a/test/parallel/test-tls-client-reject.js
+++ b/test/parallel/test-tls-client-reject.js
@@ -71,6 +71,19 @@ function rejectUnauthorized() {
     servername: 'localhost'
   }, common.mustNotCall());
   socket.on('data', common.mustNotCall());
+  socket.on('error', common.mustCall(function(err) {
+    rejectUnauthorizedUndefined();
+  }));
+  socket.end('ng');
+}
+
+function rejectUnauthorizedUndefined() {
+  console.log('reject unauthorized undefined');
+  const socket = tls.connect(server.address().port, {
+    servername: 'localhost',
+    rejectUnauthorized: undefined
+  }, common.mustNotCall());
+  socket.on('data', common.mustNotCall());
   socket.on('error', common.mustCall(function(err) {
     authorized();
   }));