Skip to content

Commit

Permalink
src: make debugger listen on 127.0.0.1 by default
Browse files Browse the repository at this point in the history
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 <jasnell@gmail.com>
  • Loading branch information
bnoordhuis committed Aug 23, 2016
1 parent 16f4b8e commit 8e7cbe2
Show file tree
Hide file tree
Showing 8 changed files with 38 additions and 43 deletions.
2 changes: 1 addition & 1 deletion src/debug-agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,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)
Expand Down
2 changes: 1 addition & 1 deletion src/debug-agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,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
Expand Down
18 changes: 9 additions & 9 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -146,9 +146,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;
Expand Down Expand Up @@ -3468,15 +3468,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;
}

Expand All @@ -3486,13 +3486,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;
Expand Down Expand Up @@ -3769,11 +3769,11 @@ static void StartDebug(Environment* env, bool 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;
}
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-debug-port-cluster.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}`));
}
});

Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-debug-port-from-cmdline.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,10 @@ function processStderrLine(line) {
function assertOutputLines() {
var expectedLines = [
'Starting debugger agent.',
'Debugger listening on (\\[::\\]|0\\.0\\.0\\.0):' + debugPort,
'Debugger listening on 127.0.0.1:' + debugPort,
];

assert.equal(outputLines.length, expectedLines.length);
for (var i = 0; i < expectedLines.length; i++)
assert(RegExp(expectedLines[i]).test(outputLines[i]));
assert(expectedLines[i].includes(outputLines[i]));
}
8 changes: 3 additions & 5 deletions test/parallel/test-debug-port-numbers.js
Original file line number Diff line number Diff line change
Expand Up @@ -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}`));
}
});
8 changes: 4 additions & 4 deletions test/parallel/test-debug-signal-cluster.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ process.on('exit', function onExit() {

var 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() {
Expand All @@ -79,5 +79,5 @@ function assertOutputLines() {

assert.equal(outputLines.length, expectedLines.length);
for (var i = 0; i < expectedLines.length; i++)
assert(RegExp(expectedLines[i]).test(outputLines[i]));
assert(expectedLines[i].includes(outputLines[i]));
}
36 changes: 17 additions & 19 deletions test/sequential/test-debug-host-port.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'};
Expand All @@ -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.

0 comments on commit 8e7cbe2

Please sign in to comment.