feat: allow ".local" suffixed addresses for inspector

[yinzara]

The restriction of "localhost" and "localhost6" as the only hostnames allowed for debug prevents easy debugging of locally running VMs especially those inside Kubernetes. This PR relaxes that restriction slightly to allow for any ".local" suffixed address.

nodejs/help#1700

[bnoordhuis]

Context on 583d5af: that change was made to stop DNS rebind attacks. This PR makes that possible again.

[yinzara]

I disagree with that assessment or at least I disagree in spirit that this creates a vulnerability. For a DNS rebind attack to occur, the attacker must use a compromised DNS server to respond to DNS requests and the person being attacked must attempt to resolve an address that DNS server may respond to.

Since '.local' addresses are not resolvable in the global Domain Name System, the only possibility of this being an issue is locally controlled DNS servers in which a DNS rebind attack isn't really a concern anyway.

If I acquiesce to your point, could we add in some other configuration option to make this more secure? Maybe the ability to add a subnet mask of allowed IPs? Any other thoughts?

I am unwilling to accept making NodeJS less usable as a development tool simply because of a vulnerability. There has to be another option.

[bnoordhuis]

the only possibility of this being an issue is locally controlled DNS servers in which a DNS rebind attack isn't really a concern anyway

The sufficiently paranoid will probably disagree with you. :-)

Maybe the ability to add a subnet mask of allowed IPs?

Maybe I misunderstand your suggestion but IP addresses are already allowed:

node/src/inspector_socket.cc

Lines 584 to 589 in 8772da6

	bool IsAllowedHost(const std::string& host_with_port) const {
	std::string host = TrimPort(host_with_port);
	return host.empty() || IsIPAddress(host)
	|| node::StringEqualNoCase(host.data(), "localhost")
	|| node::StringEqualNoCase(host.data(), "localhost6");
	}

[yinzara]

I think bowing to the "sufficiently paranoid" isn't necessarily the correct idea either. The whole point of a DNS rebind attack is to allow an attacker access to a local network that he wouldn't have access to normally. If the attack requires the person to control the local DNS server, it negates the benefit of the attack anyway.

My suggestion was misstated. I should have said, a config option of allowed HOSTNAMES. i.e. "these are the hostnames that are allowed in the Host header". We could keep the existing behavior of denying all hostnames except localhost. Then provide a config option, maybe something like "--inspect-hosts" that allows you to specify a comma separated list of the hostnames accepted in the Host header. As long as we don't allow wildcards or regular expressions, this should prevent the vulnerability while still allowing what I've suggested. This would prevent the DNS rebind attack as only the hostname specifically designated for the service could be allowed.

[bnoordhuis]

I think that would be acceptable but cc @nodejs/inspector, your input is requested.

[benjamingr]

This seems useful in certain cases. Not sure if we can test this?

[bnoordhuis]

I'm going to -1 this for now because #28470 (comment) is still unresolved.

Also cc @nodejs/security and @ofrobots + @eugeneo as this pertains to https://github.com/nodejs-private/node-private/pull/102.

[yinzara]

As I stated previously, the whole point of a DNS rebind attack is to allow an attacker access to a local network that he wouldn't have access to normally. Since .local addresses are not publicly resolvable, an attack would require control of the local DNS server. If the attack requires control of the local DNS server, it negates the benefit of the attack anyway as said server would already have access to the local network.

The only possibly exploit of this that I can think of would be in a sufficiently segmented local network in which a compromised DNS server in one part of the network was used as an attack to access a different part of the local network. I feel that level of security isn't necessary in the debugger and is protecting against such rare and nearly impossible use cases that the benefit of the feature far outweighs the risks.

[bnoordhuis]

If the attack requires control of the local DNS server, it negates the benefit of the attack anyway as said server would already have access to the local network.

Perhaps I should clarify: the PoC for https://github.com/nodejs-private/node-private/pull/102 was sufficiently clever that it turned a rebind attack into a RCE. In a nutshell:

1. Serve up evil.com on a public IP address.
2. Have the victim visit it with his browser.
3. Change evil.com to point to 127.0.0.1 to get around the Same Origin Policy.
4. Connect to the inspector and execute arbitrary commands on the victim's machine.

With e.g. ARP spoofing you don't even need control of the local DNS server, just MiTM'ing it is enough to accomplish step 1.

[yinzara]

Yes I understand that is true for public DNS names but all .local addresses are not resolvable via the global dns system so for someone to take advantage of a .local address they would need to control a local DNS server. The "evil.local" address would never be publicly resolvable so # 1 is impossible.

[sam-github]

they would need to control a local DNS server

Or could an attacker use access to some machine on the local network to standup an evil DNS server, and use that to escalate the breach into RCE on a target machine?

[yinzara]

they would need to control a local DNS server

Or could an attacker use access to some machine on the local network to standup an evil DNS server, and use that to escalate the breach into RCE on a target machine?

Yes and no. Yes they could do it, but the entire point of a DNS rebind attack is to gain access to a local network you don't already have access to. If they could stand up an evil DNS server locally, they already have more access than the exploit would provide.

[yinzara]

Haven't had any change to this since July. What do we need to get this approved or what changes could we implement to allow for this type of functionality in configuration?

[bnoordhuis]

You made a suggestion in #28470 (comment) that's probably an acceptable middle ground.

[fhinkel]

ping @yinzara

[gireeshpunathil]

ping @yinzara - are you planning to progress on this PR?

[yinzara]

I'd love to I just haven't had the time. I'm not sure if anyone else has interest in implementing the suggestion #28470 (comment) but I'd really love to see this happen.

[legendecas]

I'd like to pick this up.

[yinzara]

I'm going to close this PR to replace with #31071