tls: validate "rejectUnauthorized: undefined"

mcollina · BethGriggs · commit 35b86110e450 · 2021-08-09T16:28:43.000+01:00
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: nodejs-private/node-private#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>
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
@@ -1528,7 +1528,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;
     }
@@ -1611,6 +1619,13 @@ exports.connect = function connect(...args) {
     highWaterMark: options.highWaterMark,
   });
 
+  // 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
@@ -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();
   }));
