src: make debugger listen on 127.0.0.1 by default

CVE-2018-12120

Backport of 8e7cbe2 to v6.x
Prepared by Sam Roberts <[email protected]>

Original commit:
  Commit 2272052 ("net: bind to `::` TCP address by default") from
  April 2014 seems to have accidentally changed the default listen
  address from 127.0.0.1 to 0.0.0.0, a.k.a. the "any" address.

  From a security viewpoint it's undesirable to accept debug agent
  connections from anywhere so let's change that back.  Users can
  override the default with the `--debug=<host>:<port>` switch.

  Fixes: #8081
  PR-URL: #8106
  Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs-private/node-private#148
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>

src/debug-agent.cc

@@ -67,7 +67,7 @@ Agent::~Agent() {
 }
 
 
-bool Agent::Start(const std::string& host, int port, bool wait) {
+bool Agent::Start(const char* host, int port, bool wait) {
   int err;
 
   if (state_ == kRunning)

src/debug-agent.h

@@ -75,7 +75,7 @@ class Agent {
   typedef void (*DispatchHandler)(node::Environment* env);
 
   // Start the debugger agent thread
-  bool Start(const std::string& host, int port, bool wait);
+  bool Start(const char* host, int port, bool wait);
   // Listen for debug events
   void Enable();
   // Stop the debugger agent

src/node.cc

@@ -148,9 +148,9 @@ static const bool use_inspector = false;
 #endif
 static bool use_debug_agent = false;
 static bool debug_wait_connect = false;
-static std::string debug_host;  // NOLINT(runtime/string)
+static std::string* debug_host;  // coverity[leaked_storage]
 static int debug_port = 5858;
-static std::string inspector_host;  // NOLINT(runtime/string)
+static std::string* inspector_host;  // coverity[leaked_storage]
 static int inspector_port = 9229;
 static const int v8_default_thread_pool_size = 4;
 static int v8_thread_pool_size = v8_default_thread_pool_size;
@@ -3668,15 +3668,15 @@ static bool ParseDebugOpt(const char* arg) {
     return true;
   }
 
-  std::string* const the_host = use_inspector ? &inspector_host : &debug_host;
+  std::string** const the_host = use_inspector ? &inspector_host : &debug_host;
   int* const the_port = use_inspector ? &inspector_port : &debug_port;
 
   // FIXME(bnoordhuis) Move IPv6 address parsing logic to lib/net.js.
   // It seems reasonable to support [address]:port notation
   // in net.Server#listen() and net.Socket#connect().
   const size_t port_len = strlen(port);
   if (port[0] == '[' && port[port_len - 1] == ']') {
-    the_host->assign(port + 1, port_len - 2);
+    *the_host = new std::string(port + 1, port_len - 2);
     return true;
   }
 
@@ -3686,13 +3686,13 @@ static bool ParseDebugOpt(const char* arg) {
     // if it's not all decimal digits, it's a host name.
     for (size_t n = 0; port[n] != '\0'; n += 1) {
       if (port[n] < '0' || port[n] > '9') {
-        *the_host = port;
+        *the_host = new std::string(port);
         return true;
       }
     }
   } else {
     const bool skip = (colon > port && port[0] == '[' && colon[-1] == ']');
-    the_host->assign(port + skip, colon - skip);
+    *the_host = new std::string(port + skip, colon - skip);
   }
 
   char* endptr;
@@ -4093,17 +4093,18 @@ static void DispatchMessagesDebugAgentCallback(Environment* env) {
 static void StartDebug(Environment* env, const char* path, bool wait) {
   CHECK(!debugger_running);
   if (use_inspector) {
+    const char* host = inspector_host ? inspector_host->c_str() : "127.0.0.1";
     debugger_running = v8_platform.StartInspector(env, path,
-                                                  inspector_host.c_str(),
+                                                  host,
                                                   inspector_port, wait);
   } else {
     env->debugger_agent()->set_dispatch_handler(
           DispatchMessagesDebugAgentCallback);
+    const char* host = debug_host ? debug_host->c_str() : "127.0.0.1";
     debugger_running =
-        env->debugger_agent()->Start(debug_host, debug_port, wait);
+        env->debugger_agent()->Start(host, debug_port, wait);
     if (debugger_running == false) {
-      fprintf(stderr, "Starting debugger on %s:%d failed\n",
-              debug_host.c_str(), debug_port);
+      fprintf(stderr, "Starting debugger on %s:%d failed\n", host, debug_port);
       fflush(stderr);
       return;
     }

test/sequential/test-debug-host-port.js

@@ -5,7 +5,7 @@ const assert = require('assert');
 const spawn = require('child_process').spawn;
 
 let run = () => {};
-function test(args, re) {
+function test(args, needle) {
   const next = run;
   run = () => {
     const options = {encoding: 'utf8'};
@@ -14,34 +14,32 @@ function test(args, re) {
     proc.stderr.setEncoding('utf8');
     proc.stderr.on('data', (data) => {
       stderr += data;
-      if (re.test(stderr)) proc.kill();
+      if (stderr.includes(needle)) proc.kill();
     });
     proc.on('exit', common.mustCall(() => {
-      assert(re.test(stderr));
+      assert(stderr.includes(needle));
       next();
     }));
   };
 }
 
-test(['--debug-brk'], /Debugger listening on (\[::\]|0\.0\.0\.0):5858/);
-test(['--debug-brk=1234'], /Debugger listening on (\[::\]|0\.0\.0\.0):1234/);
-test(['--debug-brk=127.0.0.1'], /Debugger listening on 127\.0\.0\.1:5858/);
-test(['--debug-brk=127.0.0.1:1234'], /Debugger listening on 127\.0\.0\.1:1234/);
-test(['--debug-brk=localhost'],
-     /Debugger listening on (\[::\]|127\.0\.0\.1):5858/);
-test(['--debug-brk=localhost:1234'],
-     /Debugger listening on (\[::\]|127\.0\.0\.1):1234/);
+test(['--debug-brk'], 'Debugger listening on 127.0.0.1:5858');
+test(['--debug-brk=1234'], 'Debugger listening on 127.0.0.1:1234');
+test(['--debug-brk=0.0.0.0'], 'Debugger listening on 0.0.0.0:5858');
+test(['--debug-brk=0.0.0.0:1234'], 'Debugger listening on 0.0.0.0:1234');
+test(['--debug-brk=localhost'], 'Debugger listening on 127.0.0.1:5858');
+test(['--debug-brk=localhost:1234'], 'Debugger listening on 127.0.0.1:1234');
 
 if (common.hasIPv6) {
-  test(['--debug-brk=::'], /Debug port must be in range 1024 to 65535/);
-  test(['--debug-brk=::0'], /Debug port must be in range 1024 to 65535/);
-  test(['--debug-brk=::1'], /Debug port must be in range 1024 to 65535/);
-  test(['--debug-brk=[::]'], /Debugger listening on \[::\]:5858/);
-  test(['--debug-brk=[::0]'], /Debugger listening on \[::\]:5858/);
-  test(['--debug-brk=[::]:1234'], /Debugger listening on \[::\]:1234/);
-  test(['--debug-brk=[::0]:1234'], /Debugger listening on \[::\]:1234/);
+  test(['--debug-brk=::'], 'Debug port must be in range 1024 to 65535');
+  test(['--debug-brk=::0'], 'Debug port must be in range 1024 to 65535');
+  test(['--debug-brk=::1'], 'Debug port must be in range 1024 to 65535');
+  test(['--debug-brk=[::]'], 'Debugger listening on [::]:5858');
+  test(['--debug-brk=[::0]'], 'Debugger listening on [::]:5858');
+  test(['--debug-brk=[::]:1234'], 'Debugger listening on [::]:1234');
+  test(['--debug-brk=[::0]:1234'], 'Debugger listening on [::]:1234');
   test(['--debug-brk=[::ffff:127.0.0.1]:1234'],
-       /Debugger listening on \[::ffff:127\.0\.0\.1\]:1234/);
+       'Debugger listening on [::ffff:127.0.0.1]:1234');
 }
 
 run();  // Runs tests in reverse order.

test/sequential/test-debug-port-cluster.js

@@ -16,8 +16,7 @@ child.stderr.setEncoding('utf8');
 
 const checkMessages = common.mustCall(() => {
   for (let port = PORT_MIN; port <= PORT_MAX; port += 1) {
-    const re = RegExp(`Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):${port}`);
-    assert(re.test(stderr));
+    assert(stderr.includes(`Debugger listening on 127.0.0.1:${port}`));
   }
 });
 

test/sequential/test-debug-port-from-cmdline.js

@@ -39,10 +39,10 @@ function processStderrLine(line) {
 function assertOutputLines() {
   const expectedLines = [
     'Starting debugger agent.',
-    'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + debugPort,
+    'Debugger listening on 127.0.0.1:' + debugPort,
   ];
 
   assert.strictEqual(outputLines.length, expectedLines.length);
   for (let i = 0; i < expectedLines.length; i++)
-    assert(RegExp(expectedLines[i]).test(outputLines[i]));
+    assert(expectedLines[i].includes(outputLines[i]));
 }

test/sequential/test-debug-port-numbers.js

@@ -52,10 +52,8 @@ function kill(child) {
 
 process.on('exit', function() {
   for (const child of children) {
-    const port = child.test.port;
-    const one = RegExp(`Debugger listening on (\\[::\\]|0.0.0.0):${port}`);
-    const two = RegExp(`connecting to 127.0.0.1:${port}`);
-    assert(one.test(child.test.stdout));
-    assert(two.test(child.test.stdout));
+    const { port, stdout } = child.test;
+    assert(stdout.includes(`Debugger listening on 127.0.0.1:${port}`));
+    assert(stdout.includes(`connecting to 127.0.0.1:${port}`));
   }
 });

test/sequential/test-debug-signal-cluster.js

@@ -61,11 +61,11 @@ process.on('exit', function onExit() {
 
 const expectedLines = [
   'Starting debugger agent.',
-  'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + (port + 0),
+  'Debugger listening on 127.0.0.1:' + (port + 0),
   'Starting debugger agent.',
-  'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + (port + 1),
+  'Debugger listening on 127.0.0.1:' + (port + 1),
   'Starting debugger agent.',
-  'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + (port + 2),
+  'Debugger listening on 127.0.0.1:' + (port + 2),
 ];
 
 function assertOutputLines() {
@@ -77,5 +77,5 @@ function assertOutputLines() {
 
   assert.strictEqual(outputLines.length, expectedLines.length);
   for (let i = 0; i < expectedLines.length; i++)
-    assert(RegExp(expectedLines[i]).test(outputLines[i]));
+    assert(expectedLines[i].includes(outputLines[i]));
 }