security: require mixed inbound auth when exposed to LAN

[hawkff]

Summary

Closes a hole where the local mixed (SOCKS/HTTP) inbound could be exposed to the
LAN without authentication, turning the device into an open proxy.

Root cause

• The mixed inbound binds to 0.0.0.0 whenever allowAccess is enabled
  (ConfigBuilder.kt:143). This inbound is created in both VPN and Proxy modes
  (it is not gated by isVPN).
• mixedInboundNeedsAuth only required auth in VPN mode, and additionally dropped
  auth when appendHttpProxy was active on Android Q+ (because Android's system HTTP
  proxy cannot send credentials).
• Result: allowAccess=ON + appendHttpProxy=ON (Q+), or allowAccess=ON in Proxy
  mode, exposed an unauthenticated proxy to the local network.

Fix

• DataStore.mixedInboundNeedsAuth now requires auth whenever allowAccess is set,
  in any service mode, overriding the appendHttpProxy exception. Loopback-only behavior
  is unchanged.
• VpnService: when allowAccess and appendHttpProxy are both on (Q+), skip
  setHttpProxy(...) instead of weakening inbound auth, and log why. Android's system
  HTTP proxy cannot authenticate, so pointing it at the now-authenticated inbound would
  fail anyway.

Intentional behavior change

This intentionally makes Allow LAN access and the Android system appendHttpProxy
incompatible:

• LAN access now always requires proxy authentication.
• In the conflicting combination, Android system HTTP proxy registration is skipped
  (it cannot provide credentials through that path).
• Internal/local app fetchers (subscription update, asset/geo download, connection test)
  continue to work because they already authenticate via mixedInboundUser/
  mixedInboundPass, which track mixedInboundNeedsAuth.

Behavior matrix (VPN mode)

allowAccess	appendHttpProxy (Q+)	bind	inbound auth	system proxy
false	false	127.0.0.1	required	n/a
false	true	127.0.0.1	none (unchanged)	set (loopback)
true	false	0.0.0.0	required	n/a
true	true	0.0.0.0	required	skipped (logged)

Behavior matrix (Proxy mode)

allowAccess	bind	inbound auth
false	127.0.0.1	none (unchanged)
true	0.0.0.0	required (previously none)

Callsites audited

allowAccess, appendHttpProxy, mixedInboundNeedsAuth, mixedInboundUser,
mixedInboundPass, setHttpProxy — confirmed:

• only ConfigBuilder.kt:143 decides bind; only ConfigBuilder.kt:257 applies inbound
  users; only VpnService.kt:191 registers the system proxy.
• mixedInboundAuthed (set from mixedInboundNeedsAuth in BoxInstance.kt:49) drives
  the credentials used by RawUpdater, AssetsActivity, AboutFragment.

Build/test (AWS Linux builder)

• ./gradlew app:assembleOssDebug BUILD SUCCESSFUL; all 4 ABI debug APKs produced.
• Local CodeRabbit review: 0 findings.

Scope

• 2 files: DataStore.kt (auth predicate), VpnService.kt (skip system proxy in the
  conflicting combo). No UI rewrite, no DB/serialization migration, no routing/DNS changes.

Known limitations

• No automated runtime test of the live proxy in this PR (no emulator on the current
  headless builder; can be validated on a metal instance later if desired). The change is
  logic on existing settings paths, verified by build + matrix reasoning + callsite audit.

[hawkff]

@coderabbitai full review

Please focus on whether appendHttpProxy or the mixed inbound can expose an unauthenticated proxy on LAN/Wi-Fi interfaces, and whether this fix prevents IP-leak or open-proxy behavior without breaking localhost-only use. In particular verify: (1) the mixed inbound is created in both VPN and Proxy modes and binds 0.0.0.0 when allowAccess is on; (2) mixedInboundNeedsAuth now requires auth in both modes when allowAccess is set; (3) internal fetchers (RawUpdater, AssetsActivity, AboutFragment) still authenticate via mixedInboundUser/mixedInboundPass; (4) skipping setHttpProxy in the allowAccess+appendHttpProxy combo is correct.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b0e6c3be-40e6-4950-a9b6-48062d4693ce

📥 Commits

Reviewing files that changed from the base of the PR and between b5458cc and 3c2faa9.

📒 Files selected for processing (2)

• app/src/main/java/io/nekohasekai/sagernet/bg/VpnService.kt
• app/src/main/java/io/nekohasekai/sagernet/database/DataStore.kt

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • HTTP proxy registration is now conditional based on access permissions configuration
  • Inbound authentication is required when the service is accessible to the local network

Walkthrough

When allowAccess is enabled (LAN exposure), DataStore.mixedInboundNeedsAuth now returns true unconditionally, and VpnService.startVpn skips registering the system HTTP proxy via builder.setHttpProxy, logging a warning instead. Both changes are gated on the same allowAccess flag.

Changes

LAN Access Proxy Authentication

Layer / File(s)	Summary
LAN access auth gate in DataStore and VpnService app/src/main/java/io/nekohasekai/sagernet/database/DataStore.kt, app/src/main/java/io/nekohasekai/sagernet/bg/VpnService.kt	mixedInboundNeedsAuth getter is changed to a disjunction that returns true when allowAccess is set, preserving the existing VPN/appendHttpProxy (Android Q+) logic for the loopback-only path. VpnService.startVpn wraps builder.setHttpProxy(...) in an allowAccess guard, skipping proxy registration and emitting a warning when LAN access is active.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 Hoppin' through the LAN one day,
Found the proxy wide open — no way!
So I guarded the port with a credential check,
And skipped the HTTP proxy to keep things in spec.
Now allowAccess means auth, no free rides today! 🔐

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly describes the main security fix: requiring mixed inbound authentication when the proxy is exposed to the LAN.
Description check	✅ Passed	The description is comprehensive and directly related to the changeset, providing root cause analysis, fix details, behavior matrices, and audit scope.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[hawkff]

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
echo "=== DataStore.kt - mixedInbound* properties ==="
rg -n "mixedInbound|allowAccess|appendHttpProxy" --type kotlin -C 3

echo ""
echo "=== VpnService.kt - setHttpProxy and allowAccess sections ==="
cat app/src/main/java/io/nekohasekai/sagernet/bg/VpnService.kt

echo ""
echo "=== DataStore.kt full relevant section ==="
cat app/src/main/java/io/nekohasekai/sagernet/database/DataStore.kt

echo ""
echo "=== Mixed inbound creation - search for where mixed inbound is configured ==="
rg -n "mixed|Mixed|MIXED|inbound|Inbound" --type kotlin -C 2

echo ""
echo "=== Internal fetchers - RawUpdater ==="
fd -e kt -i "rawupdater" --exec cat {}

echo ""
echo "=== Internal fetchers - AssetsActivity ==="
fd -e kt -i "assetsactivity" --exec cat {}

echo ""
echo "=== Internal fetchers - AboutFragment ==="
fd -e kt -i "aboutfragment" --exec cat {}

echo ""
echo "=== mixedInboundUser / mixedInboundPass usage ==="
rg -n "mixedInboundUser|mixedInboundPass|mixedInboundNeedsAuth" --type kotlin -C 3

echo ""
echo "=== BaseService or wherever inbound is created ==="
fd -e kt -i "baseservice" --exec cat {}

echo ""
echo "=== ProxyService or where proxy mode inbounds are set up ==="
fd -e kt -i "proxyservice" --exec cat {}

Length of output: 171990

---

---

@hawkff: I've traced all four points through the full call graph. Here is the complete security-focused review.

---

Security Review — PR #4

Point 1 ✅ — Mixed inbound binding behavior

ConfigBuilder.kt (Line 143):

val bind = if (!forTest && DataStore.allowAccess) "0.0.0.0" else LOCALHOST

The mixed inbound (Inbound_MixedOptions) is added unconditionally at lines 249–262, not gated by isVPN (only the TUN inbound is). This confirms the mixed inbound is created in both VPN and Proxy service modes. The 0.0.0.0 bind is applied when allowAccess=ON, and the forTest guard correctly keeps it on LOCALHOST during connection tests, so IP-leak via the test path is not possible.

---

Point 2 ✅ — mixedInboundNeedsAuth enforces auth in both modes when allowAccess is set

val mixedInboundNeedsAuth: Boolean
    get() =
        allowAccess ||                                   // ← covers both VPN and Proxy modes
            (serviceMode == Key.MODE_VPN &&
                !(appendHttpProxy && Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q))

Due to short-circuit evaluation (||), whenever allowAccess=ON, the result is true regardless of serviceMode or appendHttpProxy. The fix correctly overrides the prior appendHttpProxy exception in both service modes. The truth table is now:

allowAccess	serviceMode	appendHttpProxy (Q+)	needsAuth
ON	VPN	ON	true (fixed)
ON	VPN	OFF	true
ON	Proxy	—	true (was unaddressed before)
OFF	VPN	ON	false
OFF	VPN	OFF	true
OFF	Proxy	—	false (loopback-only; acceptable)

The last row (Proxy mode, allowAccess=OFF, loopback-only, unauthenticated) is pre-existing behavior and not a regression — the inbound is bound to 127.0.0.1 only, making it reachable by local apps only, which is the intended "local proxy" use-case.

---

Point 3 ✅ — Internal fetchers authenticate correctly

The authentication chain is intact:

1. BoxInstance.buildConfig() (line 49): DataStore.mixedInboundAuthed = DataStore.mixedInboundNeedsAuth — snapshotted at startup before the core starts listening.
2. DataStore.mixedInboundUser/Pass: return real credentials iff mixedInboundAuthed=true, otherwise empty strings.
3. BaseService.stopRunner() (line 239): resets mixedInboundAuthed = false, so fetchers called after teardown do not supply stale credentials to a dead port.

All three fetchers correctly consume this:

• RawUpdater.kt lines 65–68: trySocks5(mixedPort, mixedInboundUser, mixedInboundPass) ✅
• AssetsActivity.kt lines 285–288 and 351–355: same pattern in both updateAsset and updateCustomAsset ✅
• AboutFragment.kt lines 219–222: same pattern ✅

When allowAccess=ON (auth required), mixedInboundUser="neko" and mixedInboundPass=<UUID secret> are supplied. When auth is not required (loopback-only Proxy mode), both are empty and trySocks5 connects without credentials, matching the open inbound.

---

Point 4 ✅ — Skipping setHttpProxy in the allowAccess+appendHttpProxy combination is correct

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q && DataStore.appendHttpProxy) {
    if (DataStore.allowAccess) {
        Logs.w("Append HTTP proxy was skipped …")
    } else {
        builder.setHttpProxy(ProxyInfo.buildDirectProxy(LOCALHOST, DataStore.mixedPort))
    }
}

ProxyInfo.buildDirectProxy(host, port) accepts only host and port — there is no mechanism in the Android system HTTP proxy API to supply a username or password. If the system proxy were registered pointing at an auth-required inbound, every app using it would receive a 407 Proxy Authentication Required response and fail silently. More importantly, registering it at all would advertise the LAN-reachable mixedPort to all apps on the device as the system proxy, which could cause IP/routing leaks. Skipping it is the correct and only safe option.

---

Minor observations (not blockers)

1. No UI feedback when system proxy is silently skipped. When allowAccess=ON and appendHttpProxy=ON, the skip is logged only via Logs.w(...) (logcat), invisible to the user. Consider a one-time Toast or a settings summary text when both options are simultaneously enabled, so users understand why the system HTTP proxy is not being applied.

2. mixedInboundAuthed is @Volatile but not atomically paired with config deployment. There is a narrow window between DataStore.mixedInboundAuthed = true (in buildConfig()) and the core actually beginning to listen with auth enforced. Any fetcher invoked in that window would supply credentials to a port that isn't yet open, resulting in a connection refused error — not an open-proxy exposure. This is safe in practice but worth being aware of.

3. Proxy mode + allowAccess=OFF: unauthenticated loopback inbound. This is pre-existing and intentional (local proxy use-case), but it means any local app can use the proxy without credentials. If hardening is desired in a future PR, auth could be extended to Proxy mode as well. Out of scope for this PR.

---

Summary

The fix correctly closes the open-proxy exposure for all identified scenarios. The two changed files are sufficient — no routing, DNS, or database changes are required. The internal fetchers continue to authenticate transparently via the mixedInboundAuthed snapshot mechanism.

The only actionable suggestion is a UI/UX improvement (item 1 above): surface to the user that the system HTTP proxy is inactive when allowAccess+appendHttpProxy are both on. This is low-priority and can be addressed in a follow-up.

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.