Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: add security warning when inspector is running on public network #23756

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,19 @@
});
process.argv[0] = process.execPath;

// Handle inspector security warning
const debugOptions = process.binding('config').debugOptions;
if (debugOptions.host !== '127.0.0.1') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other loopback addresses? What about IPv6?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a guard against default value that is here, even when the user does not run node with inspect parameter

const {
checkInspectorHost
} = NativeModule.require('internal/inspector_security');
checkInspectorHost(debugOptions.host).then((warning) => {
if (warning !== '') {
process.emitWarning(warning);
}
});
}

// Handle `--debug*` deprecation and invalidation.
if (process._invalidDebug) {
process.emitWarning(
Expand Down
67 changes: 67 additions & 0 deletions lib/internal/inspector_security.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
'use strict';

const dns = require('dns');
const util = require('util');

const lookup = util.promisify(dns.lookup);

const IP_RANGES = {
local: 'LOCAL',
private: 'PRIVATE',
public: 'PUBLIC'
};

function isValidIpV4(parts) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have this kind of functionality in core, it would be better to just reuse that.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind pointing out where it is?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slonka I believe you can find this kind of validation on internal/net.js

node/lib/internal/net.js

Lines 25 to 37 in 2f1c356

function isIPv4(s) {
return IPv4Reg.test(s);
}
function isIPv6(s) {
return IPv6Reg.test(s);
}
function isIP(s) {
if (isIPv4(s)) return 4;
if (isIPv6(s)) return 6;
return 0;
}

return parts.length === 4 &&
(parts.every((part) => part >= 0 && part <= 255));
}

function convertPartsToLong(parts) {
return parts[0] * 1e9 + parts[1] * 1e6 + parts[2] * 1e3 + parts[3];
}

// Loopback: 127.0.0.0 - 127.255.255.255.
// Private: 10.0.0.0 - 10.255.255.255, 172.16.0.0 - 172.31.255.255
// 192.168.0.0 - 192.168.255.255
// Public: everything else
function toRange(ip) {
if (ip >= 127000000000 && ip <= 127255255255) {
return IP_RANGES.local;
} else if ((ip >= 10000000000 && ip <= 10255255255) ||
(ip >= 172016000000 && ip <= 172031255255) ||
(ip >= 192168000000 && ip <= 192168255255)) {
return IP_RANGES.private;
}
return IP_RANGES.public;
}

async function checkInspectorHost(host) {
let parts = host.split('.').map((part) => parseInt(part, 10));

if (!isValidIpV4(parts)) {
try {
parts = await lookup(host);
} catch (e) {
return `Inspector: could not determinate the ip of ${host}`;
}
} else {
return '';
}

const ip = convertPartsToLong(parts);
const range = toRange(ip);

if (range === IP_RANGES.local) {
return '';
} else if (range === IP_RANGES.private) {
return 'Inspector: you are running inspector on a private network. ' +
'Make sure you trust all the hosts on this network ' +
Copy link
Member

@ChALkeR ChALkeR Oct 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning does not describe the actual implications and doesn't tell the user what the actual problem is.

How about

In case if port ${port} is not filtered on your machine by a firewall, anyone in the same
private network ${subnet} could access your setup and perform a remote code execution.

Subnet could be taken from os.networkInterfaces().

'or filter out traffic on your firewall';
} else {
return 'Inspector: you are running inspector on a PUBLIC network. ' +
'This is a high security risk, anyone who has access to your computer ' +
'can run arbitrary code on your machine.';
}
}

module.exports = { checkInspectorHost };
1 change: 1 addition & 0 deletions node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@
'lib/internal/fs/watchers.js',
'lib/internal/http.js',
'lib/internal/inspector_async_hook.js',
'lib/internal/inspector_security.js',
'lib/internal/linkedlist.js',
'lib/internal/modules/cjs/helpers.js',
'lib/internal/modules/cjs/loader.js',
Expand Down
4 changes: 3 additions & 1 deletion test/sequential/test-inspector-port-zero.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ function test(arg, port = '') {
proc.stdout.on('close', (hadErr) => assert(!hadErr));
proc.stderr.on('close', (hadErr) => assert(!hadErr));
proc.stderr.on('data', () => {
if (!stderr.includes('\n')) return;
if (!stderr.includes('\n') ||
(stderr.includes('Warning: Inspector'))) return;

assert(/Debugger listening on (.+)/.test(stderr));
port = new URL(RegExp.$1).port;
assert(+port > 0);
Expand Down