Skip to content

Harden guest TAP bridge ports against cross-guest frame leakage - #476

Open
ulziibay-kernel wants to merge 2 commits into
mainfrom
hypeship/harden-guest-tap-ports
Open

Harden guest TAP bridge ports against cross-guest frame leakage#476
ulziibay-kernel wants to merge 2 commits into
mainfrom
hypeship/harden-guest-tap-ports

Conversation

@ulziibay-kernel

@ulziibay-kernel ulziibay-kernel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Summary

Guest VMs on a shared bridge can passively capture unicast frames addressed to other guests. Port isolation is already enabled on isolated networks, but it only blocks guest-to-guest forwarding: when the bridge has no FDB entry for a destination MAC (aged out, or a guest that was just deleted while upstream packets were still in flight), it floods the frame to every port, including other guests' TAPs. A guest with a raw socket sees those frames.

This applies the standard multi-tenant bridge-port hardening to each guest TAP on isolated networks, right after isolation is set:

step effect
permanent FDB entry (bridge fdb replace <mac> dev <tap> master permanent) inbound delivery to the guest never depends on flooding, and learning cannot relocate the entry
LinkSetFlood(tap, false) unknown-unicast frames are never flooded to the guest port
LinkSetLearning(tap, false) a guest cannot poison the FDB by spoofing a source MAC (which could blackhole or misdirect neighbours' traffic)

The three steps live in hardenIsolatedPort. The FDB entry is built (and the MAC parsed) before the port is touched, so an unparseable MAC leaves the port at its defaults rather than flooding-off with nothing pinned to it.

createTAPDevice now takes the allocated guest MAC; both call sites (CreateAllocation, RecreateAllocation) already had it. Only isolated networks are affected — non-isolated networks keep their current behaviour.

Notes for review

  • Learning-off is the one step with blast radius. With every guest MAC pinned statically it is safe, but it means a guest that changes its own MAC in-guest loses inbound until it changes back. That is an unsupported configuration, and it is what closes the FDB-poisoning vector, so I kept it — happy to drop it to a follow-up if you'd rather land flood-off + static FDB first.
  • Failures in the new steps fail TAP creation, matching how the existing isolation step behaves.
  • FDB entries are per-port and are removed by the kernel when the TAP is deleted, so no cleanup path was needed; TAP GC is unaffected.
  • The entry is installed with NeighSet (NLM_F_REPLACE) rather than NeighAppend: NLM_F_APPEND carries multi-destination semantics, and replace is the idempotent primitive for pinning a single unicast MAC. NeighAdd (NLM_F_EXCL) is wrong here — it returns EEXIST if the MAC is still pinned to a stale port.
  • Broadcast and multicast flooding are deliberately left on (bcast_flood / mcast_flood), so DHCP and ND still work. Guests can therefore still observe broadcast ARP from the uplink; suppressing that (neigh_suppress) is a separate change.

Testing

  • go vet ./lib/network/ on linux, and GOOS=darwin go vet ./lib/network/ for the stub — both clean.
  • go test ./lib/network/ passes.
  • Exercised against a real bridge. TestHardenIsolatedPortOnRealBridge and TestHardenIsolatedPortLeavesPortAloneOnBadMAC create an actual bridge and enslaved TAP and assert the resulting port state, so the netlink requests themselves are covered rather than just the entry builder. They skip when not root, following the existing convention for privileged tests in this repo. Run as root on kernel 6.12:
    • flood off / learning off read back via LinkGetProtinfo
    • the guest MAC present in NeighList(tap, AF_BRIDGE) as NUD_PERMANENT
    • on an unparseable MAC, flood on / learning on — the port is untouched
    • both tests clean up their links, verified with ip -br link afterwards
  • TestGuestFDBEntry / TestGuestFDBEntryRejectsBadMAC still cover the pure entry builder.
  • go build ./... reports three //go:embed errors for downloaded binaries; identical on main (pre-existing, unrelated).

Port isolation only blocks guest-to-guest forwarding. Unknown-unicast
frames arriving from the uplink for a MAC the bridge has forgotten
(aged out, or a guest that was just torn down) are still flooded to
every guest port, so a guest with a raw socket can passively capture
traffic addressed to other guests on the same bridge.

On isolated networks, for each guest TAP:
- disable unicast flooding so unknown-destination frames are never
  delivered to the port
- pin the guest MAC to its port with a permanent FDB entry so inbound
  delivery never depends on flooding
- disable MAC learning on the port so a guest cannot relocate FDB
  entries by spoofing a source MAC

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ulziibay-kernel
ulziibay-kernel marked this pull request as ready for review September 9, 2026 20:59

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7e52be6. Configure here.

State: netlink.NUD_PERMANENT,
Flags: netlink.NTF_MASTER,
HardwareAddr: hwAddr,
}, nil

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

FDB entry marks guest MAC local

High Severity

guestFDBEntry sets NUD_PERMANENT, which the kernel maps to BR_FDB_LOCAL. Frames for the guest MAC are passed up to the host instead of forwarded to the TAP. With unicast flooding disabled, isolated guests lose inbound unicast entirely.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7e52be6. Configure here.

Move the flood/learning/FDB steps into hardenIsolatedPort and cover them
with root-gated tests that build an actual bridge and TAP, so the netlink
requests are exercised rather than only the entry builder.

Build the FDB entry before touching the port so an unparseable MAC leaves
the port at its defaults instead of flooding off with nothing pinned, and
use NeighSet rather than NeighAppend: NLM_F_APPEND carries
multi-destination semantics, while replace is the idempotent primitive for
pinning a single unicast MAC.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@sjmiller609 sjmiller609 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting more information and validation of this change:

  • before/after benchmarks: test latency in a development hypeman server deployed from this code version (e.g. working on the deft server). try this skill /benchmark-hypeman-fork, which is the most-critical path for our platform, but also please test normal creation and restore. compare latency and failure rates, including concurrent operations. Post in this PR in a comment or similar to know latency doesn't suffer majorly from this.
  • security proof or documented what you tried before / after: either a test or comment showing the practical issue demonstrated in the before case and resolved in the after case.
  • staging QA: deploy the change to staging hypeman via our CI and validate it there, ensuring normal platform behavior. I think it would be conspicuously broken in the case of not working. Rollback is safe, i.e. we can undo this change in staging and so long as the associated VMs are deleted, then the change is cleanly reverted. Document in this PR what was validated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants