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

Proposal for changes to NAT. #279629

Closed
wants to merge 1 commit into from

Conversation

gray-heron
Copy link

@gray-heron gray-heron commented Jan 8, 2024

Background

Recently I've been testing NixOS' NAT feature extensively, and found three surprising quirks to its behavior, one of which I find concerning.

I submit a draft of changes to the NAT integration test to cover those quirks, and check against behavior I find unreasonable. If maintainers agree with this proposal, I will proceed to implement changes to the actual NAT module to make this new test pass.

1. NAT affects all interfaces.

As currently implemented, turning on NAT enables indiscriminate traffic forwarding between all interfaces on the nat-enabled machine in all directions. In particular this allows to:

  • reach resources "behind" NAT from the externalNetwork,
  • reach services hosted on uninvolved interfaces from the externalNetwork (uninvolved meaning unspecified at all in the NAT configuration),
  • reach any machine connected to the nat-enabled machine from any network connected to nat-enabled machine.

The new test checks against those in many places, see line 177.

2. DNAT overrides source IP address.

The rules to capture packets intended for loopbackIPs are so wide they capture "normal" traffic from the externalInterface indented for DNAT. In the end, the packets get routed to the correct destination, but because the source IP is being set to externalIP, the listening service cannot identify what is coming from where, which is oftentimes useful for i.e. blacklisting.

See new check client.succeed("check-last-clients-ip ${serverIp}").

A side question: why does the loopbackIP option even exist -- as in why aren't all forwards loopbacked by default? Is there any reasonable use case for routing traffic from the outside differently than from the inside... but only the forwarded ports?

3. DNAT captures traffic on all IPs even if externalIP was specified.

networking.nat.externalIP option allows to specify the exact external address to be used for NAT. This is particularly useful, if there are multiple IP addresses associated with one networking.nat.externalInterface.

However, as currently implmented, even if externalIP was set, packets destined for other IP addresses on the externalInterface would still be captured, making it impossible to host services bound to IP addresses not meant for NAT.

See new check server.succeed('[[ 'curl http://${routerAlternativeExternalIp}:8080' == "router" ]]').

There was a pull request about this #254025, it was closed for... unrelated reasons.

The question was if this isn't too obscure to warrant changes to the configuration semantics. I think if we agree on the points above, this question becomes irrelevant.

I'm looking forward to a feedback.

@samhug @mkg20001 @duament @wegank

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jan 8, 2024
@gray-heron gray-heron requested a review from rbvermaa January 8, 2024 16:55
@gray-heron gray-heron self-assigned this Jan 8, 2024
@gray-heron gray-heron requested a review from nbp February 23, 2024 09:30
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3697

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/3751

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024
@Luflosi
Copy link
Contributor

Luflosi commented Jul 10, 2024

Does #277016 fix one of these surprising behaviours when using iptables?

JustTNE added a commit to JustTNE/nixpkgs that referenced this pull request Jul 21, 2024
This code is mostly from NixOS#279629, the uninvoled client checks were removed (since they are the same as the direct connection to the client test) and the tests were adjusted to work as intended as well as bugs fixed.
In some cases, some tests are skipped when they do not make sense for the specific configuration that is being tested.
JustTNE added a commit to JustTNE/nixpkgs that referenced this pull request Dec 1, 2024
This code is mostly from NixOS#279629, the uninvoled client checks were removed (since they are the same as the direct connection to the client test) and the tests were adjusted to work as intended as well as bugs fixed.
In some cases, some tests are skipped when they do not make sense for the specific configuration that is being tested.
@gray-heron gray-heron closed this Dec 13, 2024
@Luflosi
Copy link
Contributor

Luflosi commented Dec 13, 2024

Were all three points addressed?

@gray-heron
Copy link
Author

Oh, sorry for missing your comment!

Yes, 2. and 3. were resolved. No 1. turns out to be "only" a problem if the attacker has L2 access to any of the NAT-enabled machine interfaces, which I'd argue is still a problem. Thankfully, #277016 appears to deny unrestricted forwarding, but only when using iptables instead of nftables. I've tested it, and couldn't find any hole.

@JustTNE According to your knowledge, is the current iptables implementation tight against L2-enabled attacks?

@Luflosi
Copy link
Contributor

Luflosi commented Dec 17, 2024

Could you please take a look at the NixOS test introduced in #277016, specifically this part:

nixpkgs/nixos/tests/nat.nix

Lines 250 to 251 in 956c52b

# If using nftables without firewall, filterForward can't be used and L2 security can't easily be simulated like with iptables, skipping.
# See moby github issue mentioned above.

Does this explain why No 1. was only addressed for iptables? Is there a way to fix this? Feel free to also audit the rest of the test script and see if anything is missing.

@duament
Copy link
Contributor

duament commented Dec 17, 2024

Thankfully, #277016 appears to deny unrestricted forwarding, but only when using iptables instead of nftables.

As far as I can see, #277016 did not address No. 1. It did add a new chain nixos-filter-forward to accept NATed traffic but didn't set FORWARD policy to DROP.

To address No. 1, we could:

  • Add networking.firewall.filterForward support in firewall-iptables.nix
  • Enable filterForward by default when nat is enabled

@JustTNE
Copy link
Contributor

JustTNE commented Jan 2, 2025

@JustTNE According to your knowledge, is the current iptables implementation tight against L2-enabled attacks?

I do admit it's been quite a while since I've thought about this since the PR is unfortunately so old.
To my knowledge, it is only possible to bypass the NAT if

  1. Your network is L2 insecure (you have other untrusted machines in your L2).

AND

  1. You have nftables' filterForward off/are not using iptables -P FORWARD DROP on iptables. The change in my PR allowed the iptables side of the equation to also use something like filterForward manually with the command mentioned. The Nftables implementation already supported this. I'll circle back to this in a later part of this comment.

The text explaining this is present in the networking.nat.enable description as well:
Whether to enable Network Address Translation (NAT). A properly configured firewall or a trusted L2 on all network interfaces is required to prevent unauthorized access to the internal network.

Does this explain why No 1. was only addressed for iptables? Is there a way to fix this?

To my knowledge, I was simply not familiar with a way to run an equivalent command to iptables -P FORWARD DROP on nftables I think. I'm not 100% on this, though, given how long ago this was by now. Rest assured, if you are using networking.firewall.filterForward on nftables, you should be secure, since the tests do apply in that case.

It did add a new chain nixos-filter-forward to accept NATed traffic but didn't set FORWARD policy to DROP.

This is correct. Due to the already rampant scope creep, that was definitely out of scope for my PR. This is the configuration necessary that is mentioned in: A properly configured firewall or a trusted L2 on all network interfaces is required to prevent unauthorized access to the internal network.

Add networking.firewall.filterForward support in firewall-iptables.nix

This is an excellent idea and should be implemented. This would be a good first time pull request for someone here who is interested. It should be about a 3 line change in nat-iptables.nix.

Enable filterForward by default when nat is enabled

I agree this is a good idea, but it should probably be a separate pull request because it is bound to affect at least one existing setup that may rely on this being off by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants