libnet/d/bridge: drop connections to lo mappings, and direct remote connections #49325
Conversation
03b8006 to
ecde2b3
Compare
ecde2b3 to
1f18df4
Compare
There was a problem hiding this comment.
Looks good ...
Disallowing direct routing to published ports is quite a bit more drastic than the original change - we'll need to make it clear in release notes. (In this release we were already blocking direct routing to unpublished ports, which were previously open if the filter-FORWARD policy was ACCEPT. So it's an update for that description.)
We'll also need a docs update - this note can go, but we should probably replace it with a description of these new rules. I've not checked for other mentions / places we should mention this.
Nit - typo releasePortBindigs in the first commit comment, might make it less searchable-for. (Also, in the last commit comment, Traditionnally and unconditionnally only have one n.)
| err := netlink.LinkAdd(iface) | ||
| assert.NilError(t, err) |
There was a problem hiding this comment.
Is this something that needs cleaning up afterwards, or not a problem if we don't?
There was a problem hiding this comment.
Both times this function is called a disposable network namespace is created first. I'll add a comment stating that it should be used that way.
7a891ad to
ecf3873
Compare
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
When a NAT-based port mapping is created, the daemon adds a DNAT rule in nat-DOCKER to replace the dest addr with the container IP. However, the daemon never sets up rules to filter packets destined directly to the container port. This allows a rogue neighbor (ie. a host that shares a L2 segment with the host) to send packets directly to the container on its container-side exposed port. For instance, if container port 5000 is mapped to host port 6000, a neighbor could send packets directly to the container on its port 5000. Since nat-DOCKER mangles the dest addr, and the nat table forbids DROP rules, this change adds a new rule in the raw-PREROUTING chain to filter ingress connections targeting the container's IP address. This filtering is only done when gw_mode=nat. For the unprotected variant, no filtering is done. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Traditionally when Linux receives remote packets with daddr set to a loopback address, it reject them as 'martians'. However, when a NAT rule is applied through iptables this doesn't happen. Our current DNAT rule used to map host ports to containers is applied unconditionally, even for such 'martian' packets. This means a neighbor host (ie. a host connected to the same L2 segment) can send packets to a port mapped on a loopback address. The purpose of publishing on a loopback address is to make ports inaccessible to remote hosts -- lack of proper filtering defeats that. This commit adds an iptables rule to the raw-PREROUTING chain to drop packets with a loopback dest address and coming from any interface other than lo. To accomodate WSL2 mirrored mode, another rule is inserted beforehand to specifically accept packets coming from the loopback0 interface. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
ecf3873 to
d216084
Compare
|
Discussing with @akerouanton and @robmry - more eyes on this could still be useful, but we can make changes in a follow-up where needed; we can probably bring this one in |
|
Hi all, can I check as this was identified as a security issue that's been fixed (and I know from reading some linked issues, it's been around a while), are there any plans to assign a CVE for it? Just thinking that without that people might not realise that it's important to upgrade to this version for security purposes. |
|
Thanks for asking @raesene. We're planning to release a blog post soon that should help clarify what we did, what this issue is about, and why we did consider fixing it in v28. At the moment, we're considering this as an hardening improvement, and we're not planning to get a CVE assigned. |
- What I did
This PR fixes two separate security issues that have a common cause -- lack of proper packet filtering before NATing.
- How I did it
1st and 2nd commit are just a small refactoring, and a new integration for the use-case broken by:
3rd commit: libnet/d/bridge: port mappings: drop direct-access when gw_mode=nat
When a NAT-based port mapping is created, the daemon adds a DNAT rule in nat-DOCKER to replace the dest addr with the container IP. However, the daemon never sets up rules to filter packets destined directly to the container port. This allows a rogue neighbor (ie. a host that shares a L2 segment with the host) to send packets directly to the container on its container-side exposed port.
For instance, if container port 5000 is mapped to host port 6000, a neighbor could send packets directly to the container on its port 5000.
Since nat-DOCKER mangles the dest addr, and the nat table forbids DROP rules, this change adds a new rule in the raw-PREROUTING chain to filter ingress connections targeting the container's IP address.
This filtering is only done when gw_mode=nat. For the unprotected variant, no filtering is done.
4th commit: libnet/d/bridge: drop remote connections to port mapped on lo
Traditionnally when Linux receives remote packets with daddr set to a loopback address, it reject them as 'martians'. However, when a NAT rule is applied through iptables this doesn't happen. Our current DNAT rule used to map host ports to containers is applied unconditionnally, even for such 'martian' packets.
This means a neighbor host (ie. a host connected to the same L2 segment) can send packets to a port mapped on a loopback address. The purpose of publishing on a loopback address is to make ports inaccessible to remote hosts -- lack of proper filtering defeats that.
This commit adds an iptables rule to the raw-PREROUTING chain to drop packets with a loopback dest address and coming from any interface other than lo.
To accomodate WSL2 mirrored mode, another rule is inserted beforehand to specifically accept packets coming from the loopback0 interface.
- How to verify it
New integration tests.
- Description for the changelog