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

Warn on potentially insecure inspector options (--inspect=0.0.0.0) #23444

Open
ChALkeR opened this issue Oct 12, 2018 · 3 comments · May be fixed by #55736
Open

Warn on potentially insecure inspector options (--inspect=0.0.0.0) #23444

ChALkeR opened this issue Oct 12, 2018 · 3 comments · May be fixed by #55736
Labels
doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. inspector Issues and PRs related to the V8 inspector protocol security Issues and PRs related to security.

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Oct 12, 2018 •
edited
Loading

Extracted from #21774.

Inspector by default is bound to 127.0.0.1, but suggestion to launch it with --inspect=0.0.0.0 is highly copy-pasted without proper understanding what it does. I've observed that personally in chats, also see google.

Binding inspector to 0.0.0.0 (in fact, to anything but the loopback interface ip) allows RCE, which could be catastrophic in cases where the IP is public. The users should be informed of that.

A warning printed to the console (with corresponding documentation change) should at least somewhat mitigate this.

Note: the doc change and the c++ change can come separately.
@ChALkeR ChALkeR added security Issues and PRs related to security. inspector Issues and PRs related to the V8 inspector protocol labels Oct 12, 2018
@ChALkeR ChALkeR mentioned this issue Oct 12, 2018
Closed
@ChALkeR ChALkeR added the doc Issues and PRs related to the documentations. label Oct 12, 2018
@ChALkeR ChALkeR mentioned this issue Oct 13, 2018
Closed
3 tasks
ChALkeR added a commit to ChALkeR/io.js that referenced this issue Oct 13, 2018
@ChALkeR
doc: inspector security warning for changing host
825dbdb 
Refs: nodejs#23444
Refs: nodejs#21774
@nik72619c
Copy link

can I work on this one ?

@ChALkeR
Copy link
Member Author

ChALkeR commented Oct 13, 2018 •
edited
Loading

@nik72619c Sure!

Documentation change is in #23640, the warning part is free to be taken atm.

I was thinking about:

  1. Making a function to determine if the specified IPv4 is either (1) loopback, (2) private, or (3) public.
  2. If the user specifies a host, it's resolved to an address.
  3. Once we have the address, pass it through the function from part 1. Loopback (1) does not need any warning, and the private (2) and public (3) addresses probably need slightly different warnings, with a link to the doc from doc: inspector security warning for changing host to a public IP #23640 (once that lands).

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, mostly.

There are more special address blocks, but those are unlikely to be observed, so just falling back to «public» and printing a corresponding warning should be fine imo.

I can mentor that.

@slonka
Copy link

slonka commented Oct 18, 2018

@nik72619c are you still working on this? If not I'm happy to take over.

@slonka slonka mentioned this issue Oct 19, 2018
Closed
4 tasks
Trott pushed a commit to Trott/io.js that referenced this issue Nov 6, 2018
@ChALkeR @Trott
doc: inspector security warning for changing host
90be286 
Refs: nodejs#23444
Refs: nodejs#21774

PR-URL: nodejs#23640
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
targos pushed a commit that referenced this issue Nov 6, 2018
@ChALkeR @targos
doc: inspector security warning for changing host
8642eac 
Refs: #23444
Refs: #21774

PR-URL: #23640
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
codebytere pushed a commit that referenced this issue Nov 29, 2018
@ChALkeR @codebytere
doc: inspector security warning for changing host
bbed685 
Refs: #23444
Refs: #21774

PR-URL: #23640
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
@ChALkeR @MylesBorins
doc: inspector security warning for changing host
a77dc9a 
Refs: #23444
Refs: #21774

PR-URL: #23640
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 3, 2018
@ChALkeR @MylesBorins
doc: inspector security warning for changing host
7f1e0e5 
Refs: #23444
Refs: #21774

PR-URL: #23640
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Eugene Ostroukhov <[email protected]>
@jasnell jasnell added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Jun 26, 2020
DemianParkhomenko added a commit to DemianParkhomenko/node that referenced this issue Nov 5, 2024
@DemianParkhomenko
lib: add warning when binding inspector to public IP
847aaae 
Add `isLoopback` function to `internal/net` module to check if a given
host is a loopback address.

Add a warning when binding the inspector to a public IP with an open
port, as it allows external hosts to connect to the inspector.

Fixes: nodejs#23444
Refs: https://nodejs.org/api/cli.html#--inspecthostport
@DemianParkhomenko DemianParkhomenko linked a pull request Nov 5, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations. help wanted Issues that need assistance from volunteers or PRs that need help to proceed. inspector Issues and PRs related to the V8 inspector protocol security Issues and PRs related to security.
Projects
None yet
Milestone
No milestone
4 participants
@ChALkeR @jasnell @slonka @nik72619c