Harden the Rust daemon, and make lz4 optional as COMPRESS implies - #12
Merged
Conversation
added 8 commits
September 4, 2026 11:32
pfui_wire imported lz4.frame at module scope, so the package was mandatory whatever COMPRESS said. A resolver with compression off still could not load the module, and neither could a firewall: the one dependency documented as droppable was not droppable. HAVE_LZ4 now reports what is available and the codec is resolved only when a payload is actually compressed. Asking for compression without the package raises rather than falling back to plain JSON. Falling back would put an uncompressed payload on the wire while the far end expected a frame, turning a missing package into a COMPRESS mismatch, which is a much harder fault to read than the thing that caused it. Both ends check the combination when their config loads rather than per message. The resolver would otherwise raise inside every answer, answering DNS normally while telling no firewall anything, which leaves clients denied by PF with nothing in the log to explain it; the daemon would refuse every message as undecodable, which reads as a client fault.
The acknowledgement is not length-prefixed, so a client reads a reply until the connection closes. This daemon wrote ACKUPDATE before touching Redis or the persist file, but held the socket open until the message was fully handled, so the resolver waited for both anyway. The ordering PF, ACKUPDATE, Redis, persist was therefore observable only in the daemon's own log, never by the client it exists for. Half-closing the write half as soon as the ACK is flushed releases the resolver once PF passes the traffic, which is the moment it has nothing left to wait for. server-python has always done this, with a full close in disconnect() ahead of its stores; the port lost it. Stream gains shutdown_write and loses its blanket impl, so a future stream type has to decide what half-closing means for it rather than silently inheriting a no-op. A test drives a real client against a backend whose db_push blocks, and fails if the reply arrives only after that unblocks. With stats on, the latency line now reports the time to the acknowledgement as well as the total, which are no longer the same measurement.
A TTL decides how long an address stays in the PF table, and it is attacker-influenced twice over: an authoritative server chooses it, and the listener is unauthenticated, so anything pf.conf lets reach the port can send one. Unbounded, a single large value pinned its address for the life of the daemon, because the Redis backstop and the scan loop both derived their windows from it and neither capped the result. Both now clamp through the same helpers, which they have to: disagreeing would leave an entry lingering in whichever store read it as the fresher. Seven days is far above any legitimate TTL times TTL_MULTIPLIER and far below forever. The stored ttl and expires stay exactly as sent, so the record is still faithful to the answer; the bound applies where it is evaluated. A cache expiry near i64::MIN also overflowed the subtraction that sizes the Redis backstop, which panics in debug and wraps to a near-eternal lifetime in release. Separately, five config integers were cast into their destination types rather than checked. SOCKET_PORT: 65536 truncated to 0 and bound an ephemeral port, REDIS_DB: 256 selected database 0, and both did so silently. MAX_WORKERS is bounded as well: it is a thread count spawned up front, and thread::spawn panics rather than returning an error.
sync_pf_table withdraws every address in the table that has no Redis key, and ip_of reported the first address of any entry as though it were a host. A table carrying 10.0.0.0/8 read back as 10.0.0.0, and !192.0.2.1 as 192.0.2.1, so both looked orphaned and drew a host delete on every scan that could never match the entry it came from. Tables are shared: pf.conf loads the persist file into one an operator may also add to by hand. PFUI adds host-width, non-negated addresses and nothing else, so restricting the read to those hides none of its own entries and stops it fighting anyone else's.
A panic inside receiver::handle unwound the worker that ran it. Any repeatable fault therefore retired the pool one thread at a time, and once the last one had gone every message was shed: a firewall that logs, accepts connections and whitelists nothing. Each job is now caught individually, with the payload logged. The channel cannot be poisoned by this, because the receive lock is released before the job runs. try_submit also reported success for a disconnected channel, so a job dropped after the pool had gone looked handled and the accept loop carried on taking work that nothing would ever run. The three outcomes are now distinct, and a stopped pool closes the listener instead of spinning.
The sync threads were spawned before the UDP gate and before any bind, so a daemon that went on to refuse UDP mode or fail a bind had already been rewriting PF tables, Redis and the persist files. run() then returned its exit code without setting the term flag or joining them, and process::exit killed them wherever they had got to. Starting them after the binds also keeps them clear of the umask bind_unix narrows, which is process-global: a persist tempfile created inside that window came out without its group read bit. A sync thread that now fails to spawn stops the ones already running and removes the socket before exiting.
The unveil list granted rx on /sbin/pfctl and nothing else. pfctl is dynamically linked, so exec needs the loader and the libraries it maps, and without them the fallback failed at exec. CTL: IOCTL falls back to pfctl on any ioctl error, so the broken path was the safety net of the default configuration, reached exactly when the ioctl was already failing. The list is now built as data and applied separately, so it can be tested off OpenBSD. That is why the gap survived: platform.rs is compiled only for the target, so neither CI nor a developer's machine ever evaluated it, and a missing entry surfaces as a runtime failure on one code path.
handle_stream is shared by TCP and the local socket, and a reply is unframed on both, so both peers read until EOF. A unit test drives the half-close over a UnixStream pair and a TCP pair, which the release-ordering test cannot: it runs over TCP only, and the trait impls are separate code. own_group in the unix tests insisted on the caller's primary group having a name. On a directory-joined host it often does not, which failed all ten tests there for want of a grant target; any membership will do, so the primary is now only the first candidate.
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.
Eight defects in the Rust daemon, plus the two follow-ups left open by #11. Each
was verified against the code before being fixed, and each new test was verified
failing against the old behaviour first.
The resolver is released at the table update, not after the stores
ACKUPDATEwas written before Redis and the persist file, but the daemon held thesocket open until the message was fully handled. A reply is not length-prefixed,
so a client reads until the connection closes — and therefore waited for Redis and
the persist write anyway. The
PF → ACKUPDATE → Redis → persistordering wasobservable only in the daemon's own log, never by the client it exists for.
Half-closing the write half as soon as the ACK is flushed releases the resolver
the moment PF passes the traffic.
server-pythonhas always done this, with afull
conn.close()indisconnect()ahead of its stores under the comment# Unblock PFUI_Unbound DNS Client; the Rust port lost it, so this restoresparity with the reference rather than changing the design.
This applied to both stream transports.
handle_streamignores which one itis on and the client's read loop is shared, so the local socket waited exactly as
TCP did — relatively worse, since it is the fast path.
Streamgainsshutdown_writeand loses its blanket impl, so a future stream type has to decidewhat half-closing means for it instead of silently inheriting a no-op. One test
drives a real client against a backend whose
db_pushblocks; another drives thehalf-close over a
UnixStreampair and a TCP pair, which the first cannot, becauseit runs over TCP only and the impls are separate code.
With stats on, the latency line now reports time to the acknowledgement as well as
the total, which are no longer the same measurement.
lz4 is optional, as COMPRESS implies
pfui_wireimportedlz4.frameat module scope, so the package was mandatorywhatever
COMPRESSsaid — the one dependency documented as droppable was notdroppable, on the resolver or on either daemon.
HAVE_LZ4reports what isavailable and the codec is resolved only when a payload is actually compressed.
Asking for compression without the package raises rather than falling back to
plain JSON. A fallback would put an uncompressed payload on the wire while the far
end expected a frame, turning a missing package into a
COMPRESSmismatch — amuch harder fault to read than its cause. Both ends check the combination when
their config loads: the resolver would otherwise raise inside every answer,
answering DNS normally while telling no firewall anything, and the daemon would
refuse every message as undecodable, which reads as a client fault.
The pfctl fallback could not exec under unveil
The unveil list granted
rxon/sbin/pfctland nothing else. pfctl isdynamically linked, so exec needs the loader and the libraries it maps.
CTL: IOCTLfalls back to pfctl on any ioctl error, so the broken path was thesafety net of the default configuration, reached exactly when the ioctl was
already failing.
platform.rsis compiled only for the target, so neither CI nor a developer'smachine ever evaluated that list — which is why the gap survived, and a missing
entry surfaces as a runtime failure on one code path. The list is now built as
data and applied separately, so its contents are tested everywhere. The grants
are still unverified on hardware: that needs a firewall running
CTL: PFCTL, ora forced ioctl failure, and it is the one item here I would check before trusting.
An unbounded TTL could pin an address forever
A TTL decides how long an address stays in the PF table and it is
attacker-influenced twice over: an authoritative server chooses it, and the
listener is unauthenticated, so anything
pf.conflets reach the port can sendone. The Redis backstop and the scan loop both derived their windows from it and
neither capped the result, so one large value held its address for the life of the
daemon.
Both now clamp through the same helpers, which they have to: disagreeing would
leave an entry lingering in whichever store read it as the fresher. Seven days is
far above any legitimate TTL times
TTL_MULTIPLIERand far below forever. Thestored
ttlandexpiresstay exactly as sent, so the record remains faithful tothe answer; the bound applies where it is evaluated. A cache expiry near
i64::MINalso overflowed the subtraction sizing the Redis backstop — a panic indebug, a near-eternal lifetime in release.
A panicking job retired the worker that ran it
Any repeatable fault retired the pool one thread at a time, and once the last had
gone every message was shed: a firewall that logs, accepts connections and
whitelists nothing. Jobs are caught individually now, with the payload logged. The
channel cannot be poisoned by this, because the receive lock is released before
the job runs.
try_submitalso reported success for a disconnected channel, so a job droppedafter the pool had gone looked handled and the accept loop kept taking work that
nothing would ever run. The three outcomes are distinct now, and a stopped pool
closes the listener instead of spinning.
The expiry threads ran before the daemon committed to serving
They were spawned before the UDP gate and before any bind, so a daemon that went
on to refuse UDP mode or fail a bind had already been rewriting PF tables, Redis
and the persist files.
run()then returned its exit code without setting the termflag or joining them, and
process::exitkilled them wherever they had reached.Starting them after the binds closes a second window as well:
bind_unixnarrowsthe process-global umask, so a persist tempfile created during it came out without
its group read bit. A sync thread that now fails to spawn stops the ones already
running and removes the socket before exiting.
A shared PF table was fought over every scan
sync_pf_tablewithdraws every table address that has no Redis key, andip_ofreported the first address of any entry as though it were a host.
10.0.0.0/8read back as
10.0.0.0and!192.0.2.1as192.0.2.1, so both looked orphanedand drew a host delete on every scan that could never match the entry it came
from.
Tables are shared:
pf.confloads the persist file into one an operator may alsoadd to by hand. PFUI adds host-width, non-negated addresses and nothing else, so
restricting the read to those hides none of its own entries and stops it fighting
anyone else's.
Config integers were cast into their destination, not checked
SOCKET_PORT: 65536as u16REDIS_DB: 256as u8REDIS_PORT,SOCKET_BACKLOG,TTL_MULTIPLIERAll range-checked now, with the key named in the error.
MAX_WORKERSis boundedtoo: it is a thread count spawned up front, and
thread::spawnpanics rather thanreturning an error.
Testing
rather than reloading the shared one, which the resolver and both daemons hold
own_groupin the unix tests required the caller's primary group to have aname. On a directory-joined host it often does not, which failed all ten tests
there for want of a grant target; any membership will do
One pre-existing failure is unrelated and not addressed here:
a_message_over_the_local_socket_is_acknowledgedfails on macOS withaccept()EINVAL on Darwin's AF_UNIX poll path. It passes on Linux CI, and OpenBSD is the
target.
Still open
COMPRESSis one global key per end, so it cannot be off for a local socket andon for a remote resolver over TCP
two-figure latency line exists to settle it against a live resolver