fix(start-wrt): scope published-port NAT reflection to permitted Security Profiles - #3888
Open
Dominion5254 wants to merge 8 commits into
Open
fix(start-wrt): scope published-port NAT reflection to permitted Security Profiles#3888Dominion5254 wants to merge 8 commits into
Dominion5254 wants to merge 8 commits into
Conversation
Published-port redirects never set `reflection_zone`, so fw4 defaulted it to the redirect's `dest` zone and emitted hairpin rules matching only the target device's own subnet. A client on a different Security Profile — one already permitted to forward into the target's zone — hit the router's INPUT chain instead of the DNAT when it used the router's WAN address. Set `reflection_zone` to the target's zone plus every zone a `config forwarding` section already permits to forward into it. Not every zone: each reflection zone becomes the reflection redirect's `src`, which sets fw4's `dflags.dnat` and emits a blanket `ct status dnat accept` into that zone's forward chain ahead of the zone policy, so a reflected flow cannot be rejected afterwards. Also set `reflection '0'` on any port carrying a source restriction. fw4 builds the reflection redirect fresh and copies neither `src_ip` nor `src_mac` (`fw4.uc` ~2898), taking its saddr from the reflection zone's subnets — so a source-restricted forward was reachable by hairpin from any client in the reflection zone, including the same-subnet case that already worked before this change.
`FirewallZone.masq`/`masq6`/`mtu_fix` are `Option<bool>` and serialize as '1'/'0'; `reflection` is the same kind of fw4 boolean in the same struct family, so give it the same type instead of the older `Option<String>` idiom. The bytes written are unchanged.
`reflection_zones` copied `config forwarding` src names verbatim, excluding only the literal "wan". fw4 parses each `reflection_zone` entry as a zone reference: one name that is not a parsed zone fails the whole option and drops the entire redirect — WAN-side DNAT included — and a `src '*'` forwarding (legal fw4) would crash ruleset rendering outright when fw4 dereferences the wildcard's subnets. Stock images never write such forwardings, but an adopted config can carry them, and a zone deleted after the list was written turns into exactly that invalid name. Filter the list against the `config zone` sections actually present, and exclude WAN-like zones by property (`masq` set) rather than by the name "wan"; the redirect's own src zone stays excluded as before. Also write down why cross-zone hairpin is DNAT-only: the reflection SNAT lands in the emitted zone's srcnat chain, which a hairpinned flow never enters, so the server sees the client's real address and replies via the router — intentional, not a missing rule.
`reflection_zone` was written only by `published-ports set`, but it is derived from the `config forwarding` set, which changes underneath it: - Deleting a profile removed its zone but left the name in every list carrying it, and fw4 treats one unknown name as an invalid option — dropping the whole redirect, WAN-side DNAT included. Deleting a guest profile could silently kill an admin-LAN published port from the Internet until some unrelated re-save rewrote the pp_ sections. - Revoking a profile's Access left it in the lists, so its clients kept reaching the port via the WAN address — the exact reflect-then-reject hole the scoped list exists to prevent, with no rule able to stop it. - A profile granted Access later (or created with it) was missing from the lists, so its clients kept hitting the original bug. Add `sync_reflection_zones`, one pass re-deriving the list on every redirect tagged `_pp_id` or `_apf_label` (skipping `reflection '0'`), and run it wherever the inputs or the redirects change: at the end of `rewrite_firewall` and `delete_config` (both already restart the firewall afterwards), in `published-ports set` (replacing the inline computation), in `apply_forward` — automatic UPnP/PCP forwards now get the same scoped hairpin instead of fw4's dest-zone default, closing the canonical StartOS case of a phone on another profile reaching a UPnP-opened port by the server's public hostname — and once at boot, so routers already carrying stale lists heal (reloading the firewall only when the pass changed something).
`dest_zone` came from the neighbor table alone and fell back to "lan" whenever the device had no entry — and a device that is offline (or whose entry aged out) still passes validation, because its IPv4 resolves from the static reservation `set` itself creates. Since every save rewrites all rules, toggling any port while one target was offline silently rewrote that target's rule with the guessed zone; with scoped reflection the wrong guess also computes the wrong permitted set, e.g. handing an IoT profile with Access to the admin LAN a hairpin route to a server that actually lives in a guest zone. Resolve the zone from the device's IPv4 against the interface subnets of the non-masquerading firewall zones (every profile is a /24 at its gateway; the static reservation covers an offline device, and configs-only mode too), keeping the neighbor-table result as a logged cross-check only. A device that still cannot be placed keeps `dest 'lan'` — the WAN-side DNAT stays as it was — but gets `reflection '0'`: never hairpin into a guessed zone, and never refuse the save over one offline device. `set` now parses "network" alongside "firewall"/"dhcp"; the file is round-tripped unchanged.
- "every other profile that profile permits to reach it" read as though the target's profile grants the access; the Access setting lives on the *other* profile (see security-profiles.md), so say so. - "answered by the router itself" becomes "the router answers instead of the device" — plainer. - Reflection matches only addresses actually on the WAN interface: behind CGNAT or another upstream router the public IP a domain resolves to never hairpins, so one clause now says so before a support thread has to. - Automatic (UPnP/PCP) forwards are scoped identically now, so the section says they are covered.
Resolving the zone by address alone dropped the neighbor-table result that the previous code relied on, so a device with no IPv4 at all — an IPv6-only target, which has neither a lease nor a reservation to place it — fell to the "lan" guess even while the neighbor table knew its zone. The IPv6 forward rule's `dest` then named the wrong zone and fw4 bound it to the wrong egress interfaces. The same applied to an IPv4 outside every profile subnet. Pull the resolution into `device_zone`: the address decides when it resolves (an offline device with a reservation still places, and a save while one target is offline no longer rewrites its rule around a guess), and the neighbor table places what the address cannot. Only a device neither can place keeps the "lan" guess, and only that one is never hairpinned. Tests run non-effectful, so the helper is what makes the fallback testable. Claude-Session: https://claude.ai/code/session_01Mx4wjVx7EC29aBsQmSt5xv
`sync_reflection_zones` left `reflection '0'` redirects untouched on the grounds that they carry no list. fw4 validates every `reflection_zone` name before it reads `reflection` or `enabled`, so a stale name on such a redirect still drops the whole section, WAN-side DNAT included. Our writer never emits that combination, but a hand-edited config can, and the boot heal exists precisely for configs we did not write. Empty the list on those redirects instead of skipping them; a redirect that is already clean still counts as unchanged. Claude-Session: https://claude.ai/code/session_01Mx4wjVx7EC29aBsQmSt5xv
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A published port was only reachable at the router's WAN address from the target device's own Security Profile. fw4 defaults a redirect's
reflection_zoneto itsdestzone, so the hairpin rules matched the target's subnet alone. A client on any other profile, even one whose Access already permits it to reach the target's profile, sent its packet to the router's INPUT chain instead of the DNAT and got the router's own web UI. Found by running Synapse on a public domain behind StartWRT: the domain worked from the server's profile and from the Internet, but not from a phone on the guest profile.What changed
b5b6d6b8c).reflection_zoneis now the target's zone plus every zone aconfig forwardingsection already permits into it. Not every zone: each reflection zone gets a blanketct status dnat acceptahead of its zone policy, so a reflected flow can never be rejected afterwards. The list is the only place to say no.36be9d08c). fw4 treats one unknown name in the list as an invalid option and drops the whole redirect, WAN-side DNAT included. The list is filtered against the zones actually present, and WAN-like zones are excluded by theirmasqproperty rather than by the namewan.e329ca04e). The list is derived state, so async_reflection_zonespass now runs at the end ofrewrite_firewallanddelete_config, inpublished-ports set, in the automatic-forwardapply_forward, and once at daemon start. Without it, deleting a profile left a dead zone name that killed the redirect, revoking Access left the profile hairpin-reachable, and a newly granted profile never joined. Automatic UPnP/PCP forwards get the same scoping through this pass.c342e537e). The zone came from the neighbor table and fell back tolanfor an offline device, which with scoped reflection computes the wrong permitted set. It is now resolved from the device's IPv4 against the profile subnets, with the static reservation covering offline devices. A device that still cannot be placed keepsdest 'lan'for the WAN DNAT but getsreflection '0'.src_ip, so a rule restricted to one public address was reachable at the WAN address by every device on the zone. Such rules are now written withreflection '0'. This is the### Securityentry in the changelog.84e55877ctypesFirewallRedirect.reflectionasOption<bool>like the other fw4 booleans;bc9aad6fctrues up the docs wording.Cross-zone reflection is DNAT-only by design: the reflection SNAT lands in the emitted zone's
srcnatchain, which a hairpinned flow from another zone never enters, so the server sees the client's real address. Same-zone flows are SNATed to the router's zone address as before.Verification
543
startwrt-coreunit tests (7 new),make format-checkclean.Hardware bench on a K1 behind an upstream router (double NAT), deployed as a binary update, three profiles (Admin with the StartOS server, Guest with Access to Admin, IoT with no Access), manual TCP 443 rule to the server with the Remote Access override, public domain on the server's root CA, probed with
curl --resolve <domain>:443:<router WAN address>:reflection_zone 'lan' 'vlan_<guest>'; guestdstnat_chain holds the reflection DNAT; IoT has no chainreflection '0', bothdstnat_chains empty, direct LAN access unaffected; restoring Source to Any brings the list and the hairpin backfw4 checkclean, WAN DNAT for 443 still renderedBehind an upstream router only addresses on the router's own WAN interface hairpin, never the public IP; the docs now say so.
Merging with #3783
Rebasing this branch onto #3783's current tip is clean, and the combined tree builds. One test in this branch,
apply_scopes_reflection_like_published_ports, usesLABEL_PCP, which #3783 renames toKIND_PCP, so whichever PR lands second renames that one token.Docs and
CHANGELOG.md(under the unreleased 1.1.0) are updated in the same change.https://claude.ai/code/session_016bxSq9BshATVCVZy3eoKu9