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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

slonka
Copy link

@slonka slonka commented Oct 19, 2018 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Hi, this is an attempt to fix #23444 it's not complete but I wanted to know if I'm even heading in the right direction.

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Oct 19, 2018
@sam-github
Copy link
Contributor

I'm not sure its worth distinguishing between private and public networks - most "public" networks, such as Cafes, are going to use private IPs.

The localhost vs external access distinction is, I think, the most important, and a dire warning about external machines being able to access the inspector port should get the point across.

devsnek
devsnek suggested changes Oct 19, 2018
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

like sam said, I think the actual distinction here needs to be external vs internal interface

@slonka slonka force-pushed the add-inspector-security-warnings branch from 09eecc2 to a8be9c1 Compare October 19, 2018 20:23
mscdex
mscdex reviewed Oct 21, 2018
View reviewed changes
lib/internal/bootstrap/node.js
@@ -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

mscdex
mscdex reviewed Oct 21, 2018
View reviewed changes
lib/internal/inspector_security.js
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;
}

mscdex
mscdex suggested changes Oct 21, 2018
View reviewed changes
Copy link
Contributor

@mscdex mscdex left a comment

Choose a reason for hiding this comment

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

I agree that this should instead be about loopback vs non-loopback. Also as mentioned inline, there is missing IPv6 support and missing support for other loopback addresses.

@ChALkeR
Copy link
Member

ChALkeR commented Oct 22, 2018

@mscdex
Re: ipv6 — yes, that's mostly my mistake, I left it out completely in the initial description in the issue.

@mscdex @devsnek
Re: loopback — ok, how about this:

  • If the interface is loopback (that also corrsponds to 127.0.0.0/8 subnet for the ipv4 adresses) — no warning.
  • Otherwise, for known private ipv4 adresses, show warning A.
  • Otherwise, for all the rest and ipv6, show warning B.

I supposed the difference between warnings A and B be subtle, just to provide more details to users, as in some situations A does not pose a treat and is intended behavior.

ChALkeR
ChALkeR reviewed Oct 22, 2018
View reviewed changes
lib/internal/inspector_security.js
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 •
edited
Loading

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().

@slonka slonka force-pushed the add-inspector-security-warnings branch from a8be9c1 to 71a1a00 Compare November 25, 2018 09:56
@gireeshpunathil
Copy link
Member

@slonka - can you address the review comments?

@slonka
Copy link
Author

slonka commented Dec 29, 2019 •
edited
Loading

@slonka - can you address the review comments?

I will after new years, sorry for the delay.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:20
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Jun 25, 2020
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

There's been no further activity here. Recommending closing if it does not move forward soon

@jasnell
Copy link
Member

jasnell commented Jul 6, 2020

Closing. Can reopen if someone decides to pick this back up

@jasnell jasnell closed this Jul 6, 2020
@DemianParkhomenko DemianParkhomenko mentioned this pull request Nov 17, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Drieger Drieger Drieger left review comments

@ChALkeR ChALkeR ChALkeR left review comments

@devsnek devsnek devsnek requested changes

@mscdex mscdex mscdex requested changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Warn on potentially insecure inspector options (--inspect=0.0.0.0)
9 participants
@slonka @sam-github @ChALkeR @gireeshpunathil @jasnell @mscdex @Drieger @devsnek @nodejs-github-bot