Repository navigation
fix(net): backport the deny-all DNS refusal and the owner-only answerer socket to 0.7.0 - #2678
Closed
norrietaylor wants to merge 5 commits into
Closed
norrietaylor wants to merge 5 commits into
norrietaylor wants to merge 5 commits into
Conversation
* fix(minimal,minvmd): hold the answerer channel socket owner-only The box-zone answerer's channel socket was world-connectable: the launchd plist bound it SockPathMode 0666 and the systemd socket unit SocketMode=0666, leaving the peer-uid gate the only check on who may connect. Both units now bind it owner-only -- SockPathOwner the operator, SockPathMode 0600 on launchd; SocketUser the operator, SocketMode=0600 on systemd -- with the uid gate kept as defence in depth. An install whose socket node carries group or other bits -- one that predates the mode, or a hand that relaxed it -- now fails custody through a channel_mode fact, so the advisory re-surfaces and a step re-run re-binds the socket. The accept loop's foreign-refusal warn is rate-limited to a few lines a second, so a connect storm cannot flood the log; the refusals themselves are never limited. * fix(minimal,minimald,minvmd): name the other account on a refused channel connect A daemon whose connect the owner-only channel socket refuses now says the host's DNS service belongs to another account, not a bare Permission denied. The answerer step needs the operator's account to resolve, with no stand-in uid, and custody fails a channel socket the operator does not own. * fix(minimal,minvmd): leave another operator's DNS service alone A channel socket owned by another existing, non-root account is that operator's service: the answerer step is now unavailable with the same sentence the daemons say on a refused connect, instead of a custody failure whose step would reinstall the unit and take the service over. A root-owned socket, or one whose uid names no account, still fails custody so the step re-binds it. The minvmd tests that take the process-global foreign-refusal slots now run one at a time and start and end with every slot free, and the warn-rate test bounds the burst at two windows' worth of lines, so a slow runner that spreads the burst does not fail it. * fix(minimal): offer the step over a socket another account left behind A channel socket another account owns refuses the answerer step only when a service is installed; with none, the step's install removes the leftover socket before it loads the unit, so it is offered. --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> (cherry picked from commit 746770d)
…er (#2300) * fix(net): refuse out-of-zone names at a guest deny-subtree DNS listener A deny-all host-address box on a VM-backed guest resolved out-of-zone names at the switch gateway: the interim NET-079 carve-out admitted the gateway's port 53 whole, UDP and TCP, so a box that admits no destination could still ask a resolver beyond the machine — and DNS over TCP could retry past the verdict. The guest's deny subtree now steers its UDP-53 lookups onto a loopback listener on the answerer's port (NET-141 slice 2): deny_out admits the listener beside the reply direction, the dstnat match carries no destination address (the box asks the gateway like every other host-address box; the steer is the carve-out), and the listener itself answers REFUSED for every name outside the box zone — apex and a non-lowercase zone suffix among them — NODATA for an in-zone non-A question, and forwards the zone's A questions to the switch gateway alone. DNS over TCP is neither steered nor admitted. The guest daemon starts the listener after its classifier table loads; the daemon's own lookups are untouched (cgroup-keyed match, deny path only). The interim --gateway-resolver installer flag and every test, digest and pin that spelled the interim carve-out follow the new shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(net): bound and pin the guest DNS listener's forwards Connect each forward socket to the gateway so a datagram from any other sender is dropped, cap forwards in flight so a flood cannot exhaust the daemon's descriptors, and drop the needless to_string calls clippy rejects in the guest classifier integration test. * fix(net): refuse questionless and non-QUERY messages at the guest DNS listener decide() forwarded a message with no question, and an UPDATE or NOTIFY whose zone section named an in-zone A, because every per-question check passed vacuously. Answer the first REFUSED and the rest NOTIMP, so the forward path carries only standard lookups. Bind the listener directly so its log does not read as the box-zone answerer, pin the integration test's boot-log check to the exact example.com REFUSED line, and cite NET-079 (the deny-all case) rather than NET-141 at the lines this change added. --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 2c2f8cc)
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
Contributor
|
Reviewed: no findings; approval withheld, 1 thread unresolved at APPROVE withheld: 1 unresolved thread — minimal-foundry[bot] crates/minimald/src/net/guest_dns.rs:212. Resolving the last thread releases the approval on this head. Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required. Clobber: none seen. |
crate::traced landed on main after the release branch was cut, so the cherry-pick of #2300 did not compile here.
v0.7.2 is tagged at the branch head, so the version check requires the next patch release.
Comment on lines
+205
to
+212
| if datagram.len() > MAX_DATAGRAM { | ||
| tracing::debug!( | ||
| component = COMPONENT, | ||
| n = datagram.len(), | ||
| "dropping an oversized DNS query at the guest listener" | ||
| ); | ||
| return None; | ||
| } |
Contributor
There was a problem hiding this comment.
LOW · correctness · confidence 0.55 · in parse
The oversize guard in parse can never fire from serve_bounded. The receive buffer is exactly MAX_DATAGRAM bytes, so recv_from truncates a larger datagram to 4096 bytes and len never exceeds the limit. The truncated bytes are then parsed and can be forwarded, although the doc comment says a larger datagram is dropped. Use a buffer of MAX_DATAGRAM + 1 bytes, or drop a datagram whose len equals the buffer size. The unit test only passes MAX_DATAGRAM + 1 bytes directly to parse, so it does not cover the real path.
What the reviewer checked
guest_dns.rs:143
let mut buf = vec![0u8; MAX_DATAGRAM];and :205if datagram.len() > MAX_DATAGRAM
Hand this to an agent
Act on a code-review finding.
File: crates/minimald/src/net/guest_dns.rs
Lines: 205-212
Category: correctness
Severity: low (reviewer confidence 0.55)
Finding:
The oversize guard in `parse` can never fire from `serve_bounded`. The receive buffer is exactly `MAX_DATAGRAM` bytes, so `recv_from` truncates a larger datagram to 4096 bytes and `len` never exceeds the limit. The truncated bytes are then parsed and can be forwarded, although the doc comment says a larger datagram is dropped. Use a buffer of `MAX_DATAGRAM + 1` bytes, or drop a datagram whose `len` equals the buffer size. The unit test only passes `MAX_DATAGRAM + 1` bytes directly to `parse`, so it does not cover the real path.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
Member
Author
There was a problem hiding this comment.
Not fixed in this PR. The finding looks valid, but the same code is on main (crates/minimald/src/net/guest_dns.rs: the receive buffer is vec![0u8; MAX_DATAGRAM] and parse checks > MAX_DATAGRAM). This PR is a cherry-pick backport of #2300, and its merge-back relies on identical patches. The fix belongs on main first, then a backport if it is wanted for 0.7.x. Leaving this open for Norrie to decide.
This was referenced Oct 28, 2026
This was referenced Oct 28, 2026
Closed
Closed
evanspearman
approved these changes
Oct 29, 2026
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
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.
What
Cherry-picks two security fixes from
mainontorelease/0.7.0for 0.7.2. Both applied cleanly withgit cherry-pick -x:--network host_ip --deny-all-egressbox no longer resolves public names, which closed a DNS exfiltration channel.Why
The NET close-out (#1437) ran the user stories by hand on the released 0.7.1 on macOS. A deny-all
host_ipbox resolvedgithub.com(140.82.114.3), so the DNS channel is open in the current release. The fix is onmainonly, because 0.7.1 was cut from this branch before it merged.Verification
just clippy(macOS scope) passes on this branch.minimaldis Linux-only, so this PR's Linux and VM lanes are the proof for the daemon side. Nothing has been run against a VM build of this branch by hand.main, so the merge-back after tagging finds an identical patch for each commit.Release-branch adjustments
crate::tracedcame tomainwith feat(min, minimald): one trace per request across the CLI and the daemon (3/7, spec 25) #1995, afterrelease/0.7.0was cut. The cherry-picked guest DNS listener spawns its tasks withtokio::spawnhere. Those tasks don't carry the request trace thatmainattaches.v0.7.2is tagged at the branch head, sopackage.versionmoves to 0.7.3, as fix(deps): hotfix 0.7.2 with the RUSTSEC bumps (zstd, ctutils, crypto-bigint, tokio) #2630 did for 0.7.2.just check-versionpasses.