[caixingcheng]

No description provided.

[MelvinTo]

Summary

This PR turns the cache-flush control socket from PR #3 on by default at /run/dnscrypt/control.sock when a config has no control_socket key. Setting control_socket = '' still turns it off. If /run/dnscrypt is missing, DNS keeps running and the channel just stays off.

Verdict: findings (2 Medium, 1 Low). The main concern: because every instance now gets the same default path, -flush with a second config can connect to the first instance's socket and report success, so the second instance's cache never gets flushed. There is also an older symlink race in socket setup (it predates this PR). This PR now runs that code by default whenever the directory exists.

Reviewed at 9ad2774e00bd2d8d6a372434bf678d44127e9e72 (base 2.1.18 @ 1a28a31). Validation: native build, full package tests under -race, focused control/flush tests, and Linux/Windows/FreeBSD cross-builds all passed. Extra local review tests (Go overlays, PR source unchanged) reproduced findings 1 and 2.

---

1. Medium (pre-existing): an untrusted runtime directory allows a symlink race during socket setup

Location: dnscrypt-proxy/control.go:115 (chmod), after the bind at line 111. The same code exists on the base at control.go:109.

Scenario: The proxy runs as root without user_name, and /run/dnscrypt is writable by a less privileged account, either directly or through a parent symlink. After net.Listen binds, that account can swap control.sock for a symlink before os.Chmod(path, 0600) runs. Root then chmods the symlink's target, for example making an essential readable file 0600. The earlier Lstat only checks the final path component before binding. Parent ownership, mode, and symlinks are never checked. If the parent directory can't be modified by other users, this attack isn't possible.

Evidence: An isolated concurrent test swapped the bound socket for a symlink and saw an unrelated fixture change from 0644 to 0600, while listenControl still returned success. It reproduced on both head and base. The root impact is inferred from the path-based chmod (the test used owned temp fixtures). This PR also turns this code on for configs that omit the key, whenever the default directory exists.

Suggested fix: Before binding, require a trusted runtime directory: protected ancestors, owned by the effective UID, no untrusted writers, and no parent symlinks. Provision it safely, ideally private to the daemon. Don't rely on the final socket chmod to secure a path an attacker can write to. Also note that the socket's initial permissions at bind follow the process umask until the chmod runs.

2. Medium (PR-introduced): the shared default can report a successful flush against the wrong instance

Location: dnscrypt-proxy/config.go:120 (default), with client selection at config.go:369.

Scenario: Instances A and B listen for DNS on different ports, run as the same OS user, and neither config has control_socket. With /run/dnscrypt present, A owns the socket. B finds A's live socket, logs a control-channel error, and keeps serving DNS. Then dnscrypt-proxy -config B.toml -flush example.com connects to A, gets ok N, and exits 0. B's cache is untouched, so a health probe through B can still get cached answers while its upstream is down. On the base, an omitted key resolves to an empty string and Flush fails instead of reaching another instance.

Evidence: A review test confirmed both configs inherit the same default, then ran the listener and client against one private temp path. B's flush reached A and returned exit code 0.

Suggested fix: Derive a stable per-instance default from the config's identity and use the same derivation in the client. Alternatively, verify instance identity in the control protocol and reject mismatches. Add a two-instance regression test.

3. Low (PR-introduced): the default is always rejected on Windows

Location: dnscrypt-proxy/control.go:42 (default), checked at line 89.

Scenario: A Windows config without control_socket now tries /run/dnscrypt/control.sock on every startup. Windows filepath.IsAbs treats this path with no drive letter as not absolute, so startup logs an absolute-path error instead of the missing-directory notice. The default can never work there, though DNS keeps running. Before this PR, an omitted key skipped the channel entirely. This comes from reading the source and Go's Windows path code; it was not run on Windows.

Suggested fix: Use a platform-specific default (for example, disabled on Windows), or a supported absolute path with suitable access controls. Test the actual default through listenControl on each supported platform.

---

Other checks (no issue)

• Directory creation: the control channel never creates, chowns, or chmods /run/dnscrypt, and this repo has no service or tmpfiles setup for it. Deployment has to provide it.
• Ownership/mode: the final socket mode is 0600, owned by the effective UID at bind time. user_name drops privileges in ConfigLoad before the control channel starts, so a root-owned 0755 directory blocks the dropped user and the channel stays off while DNS continues. Running as root without dropping privileges creates a root-owned socket.
• Missing directory: on Unix (including macOS/BSD without /run), StartProxy logs a notice and continues. Other control errors are also nonfatal. Creating the directory later does not trigger a retry.
• Behavior change: the commit message and example config say default-on is intended. control_socket = '' turns it off, and an offline startup check confirmed this.
• Tests: TestControlSocketDefault (control_test.go:428) covers the omitted key, the explicit opt-out, and a missing temp parent. It does not test the real default path, unsafe directories, privilege changes, or multiple instances.

[MelvinTo]

Summary

This PR defaults control_socket to /run/dnscrypt/control.sock when the key is omitted, so configs need not set it. Verdict: needs a fix before merge — a Medium issue where two instances sharing the default socket can make a flush for one config silently flush the other.

Findings

Medium — A flush for one configuration can silently flush another instance

Location: dnscrypt-proxy/config.go:120 (default path), with client routing at config.go:369 and nonfatal listener failure at proxy.go:291.

What goes wrong: Run instances A and B as the same OS user, listening for DNS on different ports, with neither configuration specifying control_socket. When the default directory exists, A acquires the common socket. B fails to bind it but continues serving DNS. dnscrypt-proxy -config B.toml -flush review.invalid then connects to A and exits successfully after flushing A’s cache. B’s cache remains intact, defeating a cache-bypassing health probe through B. On the base branch, an omitted key left the path empty and the flush failed instead of reaching another instance.

Verification: A two-process regression using the actual listenControl, Flush and cache functions reproduced this: B’s listener was refused, its flush returned exit code 0, A’s entry disappeared, and B’s independent cached entry remained.

Suggested fix: Resolve a stable per-configuration socket identity consistently in the daemon and client, or include and verify configuration identity in the control protocol so a mismatched instance cannot report success. Preserve explicit path overrides and add a two-instance regression test.

Other notes (not blocking by themselves)

• Omitted-key decoding, explicit empty-value opt-out and custom absolute-path overrides work. Example config stays loadable by older builds.
• Existing socket tests pass for mode 0600, stale-socket recovery, ordinary-file protection and refusal to take over a live socket. Socket creation occurs after privilege dropping.
• Firewalla packaging (extension/dnscrypt) runs as pi and does not provision /run/dnscrypt; deployment must provide a trusted directory writable by that user for the default to become active. Missing-directory fallback is intentional.

Reviewed head 9ad2774e00bd2d8d6a372434bf678d44127e9e72 (base 2.1.18).

[caixingcheng]

@MelvinTo thanks for the review. I'm keeping the PR as is; here is why for each finding.

1. Symlink race during socket setup (pre-existing). The race itself is real: there is a window between net.Listen and the path-based os.Chmod. Exploiting it needs two things that this fork's only deployment does not have:

• The proxy running as root. On Firewalla, dnscrypt.service runs it as User=pi without user_name, so the chmod runs with the same privileges as the account that would swap the path. That account could already chmod those files itself, so it gains nothing.
• A socket directory writable by a less privileged account. flush the DoH health check names from the dnscrypt cache before probing, so a dead upstream is detected firewalla#10131 creates the directory through systemd with RuntimeDirectory=dnscrypt and RuntimeDirectoryMode=0700. On a box this gives /run/dnscrypt pi:pi 0700 under a root-owned 0755 /run, so only the proxy's own user and root can modify it.

So the default path always lives in a trusted directory that is private to the daemon, which is the provisioning you suggest. This also answers the note that the Firewalla packaging does not provision /run/dnscrypt: it does, as of #10131. I'd rather not add directory checks for a root-without-privilege-drop setup that we don't ship.

2. Shared default socket across instances. Each box runs a single dnscrypt-proxy instance (dnscrypt.service), so two instances never compete for the default path. Anyone who runs several instances can give each one its own control_socket. In that setup, a second instance already logs the "in use" error at startup instead of taking the socket over.

3. Windows. This fork is built and shipped for Linux only (x86_64 and aarch64 Firewalla boxes). We don't build or support Windows, so a default that is rejected there has no effect on anything we ship.

[MelvinTo]

Approved.