From d6c817225d022487f5d6982dde5b59e4b9d86e5c Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Wed, 19 Aug 2026 22:39:35 +0200 Subject: [PATCH 1/2] Decide routing once, in Rust, with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The per-app routing fork had two implementations and no test harness on the side that actually runs it. Move the decision into a Rust `policy` module inside wgbridge-rs, where CI already runs `cargo test`, and reduce the C to a caller. `route.c` becomes `policy.c`: dlopen/dlsym against libwgbridge under a pthread_once, an ABI check, and the four shims — no policy logic, with one marked exception. `jni_init` resolves it eagerly so neither the cost nor the error message lands on the packet path. Resolving rather than linking is deliberate: wgbridgeBuild is attached to merge*JniLibFolders, not preBuild, specifically so JVM unit tests do not cross-compile four ABIs, and a DT_NEEDED would invert that and stop libnetguard loading at all whenever the bridge is absent. The packet path does not cross the boundary in the common case. The three facts it reads per packet — whether any override exists, the global default, whether DNS follows it — stay in C atomics written at push time, because rediscovering them per packet is what made this fork expensive enough to surface as degraded DNS. Only a configured override crosses, and only to look a UID up. The per-flow verdict cache stays in C too: it is a cache, not policy, and it is owned by the tunnel thread. The one duplication left is route_wants_tunnel's three branches, which policy.c also implements for the case where the bridge could not be resolved — a packet still has to go somewhere. That path also pins every UID to the global default, so a failure means a per-app choice is ignored, which is what shipped before per-app routing existed: never a leak out of the tunnel, never a new drop. It logs at ERROR with dlerror(), and logs the resolved handle on success so a device test can tell the two apart. The Rust tests pin the decision table, including the two behaviours that are surprising rather than wrong: a loopback DNS query is handed to the tunnel, and the default mode is byte-identical to the pre-feature expression `!local_dest || is_dns`. CI now asserts the five exported symbols are in .dynsym for every ABI of the F-Droid APK. Without that, a stripped or renamed export degrades silently into "no per-app routing" instead of failing the build. Co-Authored-By: Claude Opus 5 --- .github/workflows/test.yml | 15 + app/CMakeLists.txt | 2 +- app/src/main/jni/netguard/netguard.c | 4 + app/src/main/jni/netguard/netguard.h | 7 +- .../main/jni/netguard/{route.c => policy.c} | 223 +++++----- wgbridge-rs/README.md | 20 + wgbridge-rs/src/lib.rs | 1 + wgbridge-rs/src/policy.rs | 415 ++++++++++++++++++ 8 files changed, 568 insertions(+), 119 deletions(-) rename app/src/main/jni/netguard/{route.c => policy.c} (51%) create mode 100644 wgbridge-rs/src/policy.rs diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 1649dce4a..b94cbbe72 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -76,6 +76,21 @@ jobs: test "$(unzip -Z1 "$APK" "lib/$ABI/libwgbridge.so")" = "lib/$ABI/libwgbridge.so" done + # The routing policy is reached by dlsym from libnetguard, so a + # stripped or renamed export degrades silently into "no per-app + # routing" rather than failing the build. readelf reads any + # architecture's ELF, unlike the host nm. + for ABI in armeabi-v7a arm64-v8a x86 x86_64; do + unzip -p "$APK" "lib/$ABI/libwgbridge.so" > /tmp/libwgbridge-$ABI.so + SYMS=$(readelf --dyn-syms --wide "/tmp/libwgbridge-$ABI.so" | awk '{print $NF}') + for SYM in tc_policy_abi_version tc_policy_set_route_uids \ + tc_policy_clear_route_uids tc_policy_is_tunnel_uid \ + tc_policy_wants_tunnel; do + echo "$SYMS" | grep -qx "$SYM" \ + || { echo "missing dynamic symbol $SYM in $ABI"; exit 1; } + done + done + instrumentation: runs-on: ubuntu-latest diff --git a/app/CMakeLists.txt b/app/CMakeLists.txt index cf32e1800..6a3612988 100644 --- a/app/CMakeLists.txt +++ b/app/CMakeLists.txt @@ -12,7 +12,7 @@ add_library( netguard src/main/jni/netguard/netguard.c src/main/jni/netguard/session.c src/main/jni/netguard/ip.c - src/main/jni/netguard/route.c + src/main/jni/netguard/policy.c src/main/jni/netguard/tls.c src/main/jni/netguard/tcp.c src/main/jni/netguard/udp.c diff --git a/app/src/main/jni/netguard/netguard.c b/app/src/main/jni/netguard/netguard.c index e0f2969d3..3a26e9eac 100644 --- a/app/src/main/jni/netguard/netguard.c +++ b/app/src/main/jni/netguard/netguard.c @@ -140,6 +140,10 @@ void JNI_OnUnload(JavaVM *vm, void *reserved) { JNIEXPORT jlong JNICALL Java_eu_faircode_netguard_ServiceSinkhole_jni_1init( JNIEnv *env, jobject instance, jint sdk) { + // Resolve the routing policy now: the packet path must never pay a dlopen, + // and a failure should be logged while there is still something to read it. + policy_ensure(); + struct context *ctx = ng_calloc(1, sizeof(struct context), "init"); ctx->sdk = sdk; diff --git a/app/src/main/jni/netguard/netguard.h b/app/src/main/jni/netguard/netguard.h index ae25a1c8f..fd655283b 100644 --- a/app/src/main/jni/netguard/netguard.h +++ b/app/src/main/jni/netguard/netguard.h @@ -537,8 +537,11 @@ void dns_resolved(const struct arguments *args, jboolean is_domain_blocked(const struct arguments *args, const char *name); -// Per-app remote routing (route.c). Java pushes down only the UIDs whose -// routing differs from the global default; the packet path only reads them. +// Per-app remote routing (policy.c, deciding in wgbridge-rs/src/policy.rs). +// Java pushes down only the UIDs whose routing differs from the global default; +// the packet path only reads them. +void policy_ensure(); + void set_route_uids(const jint *uids, int count, int default_tunnel, int dns_direct); void clear_route_uids(); diff --git a/app/src/main/jni/netguard/route.c b/app/src/main/jni/netguard/policy.c similarity index 51% rename from app/src/main/jni/netguard/route.c rename to app/src/main/jni/netguard/policy.c index 3bedda1b1..061f7813c 100644 --- a/app/src/main/jni/netguard/route.c +++ b/app/src/main/jni/netguard/policy.c @@ -12,162 +12,152 @@ * Copyright © 2026 */ +// Routing policy for both egress paths. The decision itself lives in +// wgbridge-rs/src/policy.rs, where `cargo test` covers it in CI; this file +// reaches it, caches the handful of facts the packet path needs so the common +// case never crosses the boundary at all, and remembers a flow's verdict. +// +// Nothing here decides anything, with one exception marked below. + #include "netguard.h" +#include #include #include -// Sorted array of the UIDs whose routing *differs* from the global default, -// plus that default. Written from the Java thread during a reload and read by -// the tunnel thread on the packet path, so both sides take route_lock. The -// packet path is otherwise single-threaded (one tunnelThread runs jni_run), -// which is why nothing else here needs a lock. -// -// Only the exceptions are pushed, never the whole tunnelled set. In the default -// mode every applied app is tunnelled, so a "tunnelled UIDs" array held every -// installed app and was indistinguishable from a heavily-overridden one — which -// made route_uid_relevant() below always true and cost every user, WireGuard or -// not, a per-packet lock and session-table walk. -static pthread_mutex_t route_lock = PTHREAD_MUTEX_INITIALIZER; -static jint *route_uids = NULL; -static int route_uid_count = 0; -static int route_default_tunnel = 1; - -// Fast-path mirrors of the facts the packet path needs before it knows whether -// resolving a UID is worth anything. All are read per packet, so they are -// atomics rather than lock-protected: with no per-app override configured — -// the shipped default — every UID gets the same answer, and the packet path -// must not pay a mutex or a session-table walk to rediscover that. -static _Atomic int route_has_overrides = 0; -static _Atomic int route_default_tunnel_fast = 1; - -// Whether direct apps' DNS is redirected to the system resolver. Its own flag -// rather than a reuse of args->fwd53: that one is also set by an unrelated -// port-53 forward (Secure DNS runs one whenever WireGuard carries no DNS line -// of its own), and borrowing it silently switched off the rule that every -// resolver query takes the tunnel. -static _Atomic int route_dns_direct_fast = 0; - -static int compare_uid(const void *a, const void *b) { - jint ua = *(const jint *) a; - jint ub = *(const jint *) b; - return (ua > ub) - (ua < ub); -} +#define POLICY_ABI_VERSION 1 + +static pthread_once_t policy_once = PTHREAD_ONCE_INIT; +static int policy_ok = 0; + +static int (*p_abi_version)(void) = NULL; +static void (*p_set_route_uids)(const jint *uids, int count, int default_tunnel) = NULL; +static void (*p_clear_route_uids)(void) = NULL; +static int (*p_is_tunnel_uid)(jint uid) = NULL; +static int (*p_wants_tunnel)(int local_dest, int is_dns, int tunnel_uid, int dns_direct) = NULL; + +// Facts the packet path reads per packet. Mirrored here rather than queried +// across the boundary: with no per-app override configured — the shipped +// default — every UID gets the same answer, and rediscovering that per packet +// is what made the fork expensive enough to show up as degraded DNS. +static _Atomic int policy_has_overrides = 0; +static _Atomic int policy_default_tunnel = 1; +static _Atomic int policy_dns_direct = 0; + +static void policy_load() { + // Java loads libnetguard only, so the bridge is resolved here rather than + // linked: a DT_NEEDED would stop libnetguard loading at all whenever the + // Rust library is missing, and would couple the CMake output to a cargo + // one that is deliberately built late (see app/gradle/wgbridge.gradle). + // Java's own System.loadLibrary("wgbridge") returns this same soinfo when + // the tunnel starts, so there is exactly one policy table. Never dlclose. + void *handle = dlopen("libwgbridge.so", RTLD_NOW | RTLD_LOCAL); + if (handle == NULL) { + log_android(ANDROID_LOG_ERROR, "policy: cannot load libwgbridge: %s", dlerror()); + return; + } -void set_route_uids(const jint *uids, int count, int default_tunnel, int dns_direct) { - jint *copy = NULL; - if (count > 0) { - copy = ng_malloc(sizeof(jint) * (size_t) count, "route uids"); - if (copy == NULL) { - log_android(ANDROID_LOG_ERROR, "route uids alloc failed, keeping previous routing"); - return; - } - memcpy(copy, uids, sizeof(jint) * (size_t) count); - qsort(copy, (size_t) count, sizeof(jint), compare_uid); + p_abi_version = dlsym(handle, "tc_policy_abi_version"); + p_set_route_uids = dlsym(handle, "tc_policy_set_route_uids"); + p_clear_route_uids = dlsym(handle, "tc_policy_clear_route_uids"); + p_is_tunnel_uid = dlsym(handle, "tc_policy_is_tunnel_uid"); + p_wants_tunnel = dlsym(handle, "tc_policy_wants_tunnel"); + + if (p_abi_version == NULL || p_set_route_uids == NULL || p_clear_route_uids == NULL || + p_is_tunnel_uid == NULL || p_wants_tunnel == NULL) { + log_android(ANDROID_LOG_ERROR, "policy: missing symbol: %s", dlerror()); + return; } - if (pthread_mutex_lock(&route_lock)) { - log_android(ANDROID_LOG_ERROR, "route lock failed, keeping previous routing"); - if (copy != NULL) - ng_free(copy, __FILE__, __LINE__); + int abi = p_abi_version(); + if (abi != POLICY_ABI_VERSION) { + log_android(ANDROID_LOG_ERROR, "policy: ABI %d, expected %d", abi, POLICY_ABI_VERSION); return; } - jint *previous = route_uids; - route_uids = copy; - route_uid_count = count; - route_default_tunnel = default_tunnel; + policy_ok = 1; + log_android(ANDROID_LOG_WARN, "policy: libwgbridge %p ABI %d", handle, abi); +} + +void policy_ensure() { + pthread_once(&policy_once, policy_load); +} + +void set_route_uids(const jint *uids, int count, int default_tunnel, int dns_direct) { + policy_ensure(); + + if (count < 0 || uids == NULL) + count = 0; - atomic_store_explicit(&route_has_overrides, count > 0 ? 1 : 0, memory_order_release); - atomic_store_explicit(&route_default_tunnel_fast, default_tunnel, memory_order_release); - atomic_store_explicit(&route_dns_direct_fast, dns_direct, memory_order_release); + if (policy_ok) + p_set_route_uids(uids, count, default_tunnel); - if (pthread_mutex_unlock(&route_lock)) - log_android(ANDROID_LOG_ERROR, "route unlock failed"); + atomic_store_explicit(&policy_default_tunnel, default_tunnel, memory_order_release); + atomic_store_explicit(&policy_dns_direct, dns_direct, memory_order_release); + // Without the bridge there is no table to consult, so pin every UID to the + // global default. A per-app choice is then ignored, which is exactly the + // behaviour that shipped before per-app routing existed — never a leak out + // of the tunnel, never a new drop. + atomic_store_explicit(&policy_has_overrides, + (policy_ok && count > 0) ? 1 : 0, memory_order_release); // Verdicts cached against the previous rules must not survive them. route_flow_invalidate(); - - if (previous != NULL) - ng_free(previous, __FILE__, __LINE__); } void clear_route_uids() { - set_route_uids(NULL, 0, 1, 0); + policy_ensure(); + + if (policy_ok) + p_clear_route_uids(); + + atomic_store_explicit(&policy_has_overrides, 0, memory_order_release); + atomic_store_explicit(&policy_default_tunnel, 1, memory_order_release); + atomic_store_explicit(&policy_dns_direct, 0, memory_order_release); + + route_flow_invalidate(); } int is_tunnel_uid(jint uid) { - if (pthread_mutex_lock(&route_lock)) { - // Fall back to the safest answer: keep the app in the tunnel. - log_android(ANDROID_LOG_ERROR, "route lock failed, tunnelling uid %d", uid); - return 1; - } + if (policy_ok) + return p_is_tunnel_uid(uid); - int tunnel; - // Only UIDs the user gave an explicit, differing answer for are listed. - // Everything else — an unresolved UID, system traffic, an app installed - // since the last reload — follows the global default, which is what makes - // the default mode identical to the behaviour before per-app routing - // existed. - if (uid < 0 || route_uids == NULL) - tunnel = route_default_tunnel; - else if (bsearch(&uid, route_uids, (size_t) route_uid_count, sizeof(jint), compare_uid) - != NULL) - tunnel = !route_default_tunnel; - else - tunnel = route_default_tunnel; - - if (pthread_mutex_unlock(&route_lock)) - log_android(ANDROID_LOG_ERROR, "route unlock failed"); - - return tunnel; + return route_default_is_tunnel(); } /** * Whether resolving this packet's UID can change the routing answer. * * With no per-app override configured, every UID resolves to the same global - * default, so the packet path can skip both the UID lookup and the lock. This - * is the shipped default, and keeping it free is what holds the per-packet cost - * at what it was before per-app routing existed. + * default, so the packet path can skip the UID lookup and the boundary + * crossing. This is the shipped default, and keeping it free is what holds the + * per-packet cost at what it was before per-app routing existed. */ int route_uid_relevant() { - return atomic_load_explicit(&route_has_overrides, memory_order_acquire); + return atomic_load_explicit(&policy_has_overrides, memory_order_acquire); } /** The answer every UID gets when no override is configured. */ int route_default_is_tunnel() { - return atomic_load_explicit(&route_default_tunnel_fast, memory_order_acquire); + return atomic_load_explicit(&policy_default_tunnel, memory_order_acquire); } /** Whether direct apps' DNS is redirected, in which case DNS follows the UID. */ int route_dns_direct() { - return atomic_load_explicit(&route_dns_direct_fast, memory_order_acquire); + return atomic_load_explicit(&policy_dns_direct, memory_order_acquire); } -/** - * Whether this packet belongs in the tunnel. Pure, so the rule can be read - * straight through — the caller still decides what to do when the tunnel is - * down, because only the write attempt can say whether it is. - * - * @param local_dest loopback / link-local / multicast destination - * @param is_dns port 53, over UDP or TCP - * @param tunnel_uid whether this packet's UID is routed through the tunnel - * @param dns_direct whether direct apps' DNS is redirected to the system - * resolver; while false, DNS always takes the tunnel - */ int route_wants_tunnel(int local_dest, int is_dns, int tunnel_uid, int dns_direct) { - // Destinations WireGuard cannot meaningfully forward never take the tunnel, - // whichever app sent them. + if (policy_ok) + return p_wants_tunnel(local_dest, is_dns, tunnel_uid, dns_direct); + + // The one decision duplicated outside policy.rs, and only because a packet + // still has to be routed when the bridge could not be resolved. Keep it a + // literal transcription of policy::wants_tunnel. if (local_dest && !is_dns) return 0; - - // Unless a direct app's DNS is being redirected, every resolver query takes - // the tunnel: sending it out directly would expose the user's physical - // network to the resolver. if (is_dns && !dns_direct) return 1; - return tunnel_uid; } @@ -183,15 +173,16 @@ int route_wants_tunnel(int local_dest, int is_dns, int tunnel_uid, int dns_direc // a non-SYN segment, answered it with an RST. // // So remember the verdict per flow, keyed on the 5-tuple, and consult it before -// giving up. Written and read only by the tunnel thread, which is why nothing -// here takes a lock; a reload bumps route_flow_gen instead of clearing the -// table, so entries decided under superseded rules simply stop matching. +// giving up. This is a cache rather than policy, which is why it stays on this +// side of the boundary. Written and read only by the tunnel thread, which is +// why nothing here takes a lock; a reload bumps route_flow_gen instead of +// clearing the table, so entries decided under superseded rules stop matching. -#define ROUTE_FLOW_SIZE 1024 // power of two; ~40 KB resident +#define ROUTE_FLOW_SIZE 1024 // power of two; ~56 KB resident #define ROUTE_FLOW_MAX_AGE 300 // seconds idle before an entry is reusable struct route_flow_entry { - uint32_t gen; // 0 = free + uint32_t gen; // 0 = never written uint8_t version; uint8_t protocol; uint8_t tunnel; @@ -206,7 +197,7 @@ static struct route_flow_entry route_flows[ROUTE_FLOW_SIZE]; static _Atomic uint32_t route_flow_gen = 1; void route_flow_invalidate() { - // Wrapping past 0 would resurrect free slots, so skip it. + // Wrapping to 0 would make every never-written slot look current, so skip it. uint32_t next = atomic_fetch_add_explicit(&route_flow_gen, 1, memory_order_release) + 1; if (next == 0) atomic_store_explicit(&route_flow_gen, 1, memory_order_release); diff --git a/wgbridge-rs/README.md b/wgbridge-rs/README.md index dcd82499b..82afc7db2 100644 --- a/wgbridge-rs/README.md +++ b/wgbridge-rs/README.md @@ -105,6 +105,26 @@ The existing `gradle: [fdroid]` setting remains unchanged. The prebuild step runs while dependency downloads are allowed; Gradle subsequently compiles the locked crate graph with Cargo offline. +## C API surface + +`src/policy.rs` decides per-app tunnel routing for *both* egress paths, so the +same table answers the C hijack path and, eventually, a gotatun that reads the +tun itself. `jni/netguard/policy.c` resolves these by `dlopen`/`dlsym` rather +than linking, because `wgbridgeBuild` runs late (see `app/gradle/wgbridge.gradle`) +and a `DT_NEEDED` would stop `libnetguard` loading at all when the bridge is +missing. + +| Symbol | Purpose | +|---|---| +| `tc_policy_abi_version()` | Guards the shim against a mismatched library; currently `1`. | +| `tc_policy_set_route_uids(uids, count, default_tunnel)` | Replaces the override set. A null pointer or `count <= 0` means "no overrides". | +| `tc_policy_clear_route_uids()` | Back to tunnel-everything. | +| `tc_policy_is_tunnel_uid(uid)` | Whether one UID takes the tunnel. Absence from the set means "follow the default". | +| `tc_policy_wants_tunnel(local_dest, is_dns, tunnel_uid, dns_direct)` | The packet-level decision. | + +These are exported from the `cdylib` and survive `strip = true`; CI asserts they +are present in every ABI of the F-Droid APK. + ## Java/Kotlin API surface The Java classes in `app/src/main/java/net/kollnig/missioncontrol/wgbridge/` diff --git a/wgbridge-rs/src/lib.rs b/wgbridge-rs/src/lib.rs index 114f57e08..4768474b7 100644 --- a/wgbridge-rs/src/lib.rs +++ b/wgbridge-rs/src/lib.rs @@ -20,6 +20,7 @@ pub mod callbacks; pub mod config; pub mod dns; pub mod keys; +pub mod policy; pub mod transport; pub mod tunnel; diff --git a/wgbridge-rs/src/policy.rs b/wgbridge-rs/src/policy.rs new file mode 100644 index 000000000..24660ba86 --- /dev/null +++ b/wgbridge-rs/src/policy.rs @@ -0,0 +1,415 @@ +//! Per-app tunnel routing decision, shared between the C hijack path +//! (jni/netguard/policy.c) and this crate over a C ABI. Moving the decision +//! here gives it a `cargo test` harness; the C side becomes a thin caller. +//! +//! The route table is a process-global set of UID *overrides*: it is +//! deliberately not the full tunnelled set, because the C side only ever +//! pushes down UIDs whose routing differs from the current global default. +//! An app absent from the set follows the default, whatever it is — see +//! [`RouteTable::is_tunnel_uid`]. + +use std::sync::{PoisonError, RwLock}; + +/// Per-packet facts the tunnel decision is made from. Everything here is +/// cheap to compute on the C side before crossing the FFI boundary. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub struct PacketFacts { + /// Destination is loopback, link-local, or multicast. + pub local_dest: bool, + /// Port 53 traffic, over UDP or TCP. + pub is_dns: bool, + /// This packet's owning UID is routed through the tunnel. + pub tunnel_uid: bool, + /// Direct apps' DNS is being redirected to the system resolver. + pub dns_direct: bool, +} + +/// Decides whether a packet should be sent into the WireGuard tunnel. +/// +/// Branch order matters and mirrors the C original: +/// 1. A local destination that isn't DNS can't meaningfully be forwarded by +/// WireGuard (it has no route back to loopback/link-local/multicast), so +/// it always goes direct. +/// 2. Otherwise, every DNS query defaults to the tunnel — this keeps +/// resolution consistent for tunnelled apps and is also what lets a +/// direct app's DNS be redirected to the tunnel's resolver when +/// `dns_direct` is off. It is checked before the per-UID decision. +/// 3. Anything left just follows the packet's own UID routing. +pub fn wants_tunnel(facts: PacketFacts) -> bool { + if facts.local_dest && !facts.is_dns { + return false; + } + if facts.is_dns && !facts.dns_direct { + return true; + } + facts.tunnel_uid +} + +/// A set of UIDs whose tunnel routing differs from `default_tunnel`, plus +/// the default itself. Absence from `uids` means "follows the default". +pub struct RouteTable { + uids: Vec, + default_tunnel: bool, +} + +impl RouteTable { + /// The initial/fallback table: no overrides, everything tunnelled. This + /// is also the fail-safe state used when the global lock is poisoned. + pub const fn tunnel_all() -> Self { + RouteTable { + uids: Vec::new(), + default_tunnel: true, + } + } + + /// Builds a table from a UID slice, normalising it (sorted, deduped) so + /// `is_tunnel_uid` can binary-search it. + pub fn new(uids: &[i32], default_tunnel: bool) -> Self { + let mut uids = uids.to_vec(); + uids.sort_unstable(); + uids.dedup(); + RouteTable { + uids, + default_tunnel, + } + } + + /// Whether `uid` is routed through the tunnel. + /// + /// Negative UIDs (kernel/no-owner packets) always follow the default: + /// there is no per-app policy to look up for them. A UID present in the + /// override set gets the *opposite* of the default; everything else, + /// listed or not, follows the default unchanged. + pub fn is_tunnel_uid(&self, uid: i32) -> bool { + if uid < 0 { + return self.default_tunnel; + } + match self.uids.binary_search(&uid) { + Ok(_) => !self.default_tunnel, + Err(_) => self.default_tunnel, + } + } + + /// True when this table carries any per-UID overrides at all. + pub fn has_overrides(&self) -> bool { + !self.uids.is_empty() + } + + /// The global default routing for UIDs with no override. + pub fn default_tunnel(&self) -> bool { + self.default_tunnel + } +} + +/// Process-global route table, updated by the C side whenever the app's +/// per-app routing preferences change. `RwLock::new` is const, so this needs +/// no lazy initialisation. +static ROUTES: RwLock = RwLock::new(RouteTable::tunnel_all()); + +/// ABI version for the C shim to sanity-check against. Bump on any breaking +/// change to the exported signatures below. +const POLICY_ABI_VERSION: i32 = 1; + +#[no_mangle] +pub extern "C" fn tc_policy_abi_version() -> core::ffi::c_int { + POLICY_ABI_VERSION +} + +/// Replaces the global route table. `count <= 0` or a null `uids` is treated +/// as an empty override set rather than dereferenced. +/// +/// # Safety +/// If `uids` is non-null, it must point to at least `count` valid `i32`s. +#[no_mangle] +pub extern "C" fn tc_policy_set_route_uids( + uids: *const i32, + count: core::ffi::c_int, + default_tunnel: core::ffi::c_int, +) { + let slice: &[i32] = if uids.is_null() || count <= 0 { + &[] + } else { + // SAFETY: caller guarantees `uids` points to at least `count` + // valid, initialised i32s when non-null and count > 0. + unsafe { std::slice::from_raw_parts(uids, count as usize) } + }; + let table = RouteTable::new(slice, default_tunnel != 0); + let mut guard = ROUTES.write().unwrap_or_else(PoisonError::into_inner); + *guard = table; +} + +/// Drops all overrides and returns to tunnelling everything. +#[no_mangle] +pub extern "C" fn tc_policy_clear_route_uids() { + let mut guard = ROUTES.write().unwrap_or_else(PoisonError::into_inner); + *guard = RouteTable::tunnel_all(); +} + +#[no_mangle] +pub extern "C" fn tc_policy_is_tunnel_uid(uid: i32) -> core::ffi::c_int { + // A poisoned lock still must answer, fail-safe towards tunnelling: a + // panicked writer never leaves an inconsistent `RouteTable`, so reading + // through the poison is safe and keeps the app in the tunnel by default. + let guard = ROUTES.read().unwrap_or_else(PoisonError::into_inner); + guard.is_tunnel_uid(uid) as core::ffi::c_int +} + +#[no_mangle] +pub extern "C" fn tc_policy_wants_tunnel( + local_dest: core::ffi::c_int, + is_dns: core::ffi::c_int, + tunnel_uid: core::ffi::c_int, + dns_direct: core::ffi::c_int, +) -> core::ffi::c_int { + let facts = PacketFacts { + local_dest: local_dest != 0, + is_dns: is_dns != 0, + tunnel_uid: tunnel_uid != 0, + dns_direct: dns_direct != 0, + }; + wants_tunnel(facts) as core::ffi::c_int +} + +#[cfg(test)] +mod tests { + use super::*; + + fn facts(local_dest: bool, is_dns: bool, tunnel_uid: bool, dns_direct: bool) -> PacketFacts { + PacketFacts { + local_dest, + is_dns, + tunnel_uid, + dns_direct, + } + } + + #[test] + fn local_non_dns_never_tunnels() { + for tunnel_uid in [false, true] { + for dns_direct in [false, true] { + assert!(!wants_tunnel(facts(true, false, tunnel_uid, dns_direct))); + } + } + } + + #[test] + fn local_dns_still_tunnels_when_dns_is_redirected() { + // Surprising but intentional: a loopback-destined DNS query is still + // handed to the tunnel when dns_direct is off. This is the existing + // shipped behaviour (a local stub resolver forwarding onward), not a + // bug introduced by this module — pinned here deliberately. + assert!(wants_tunnel(facts(true, true, false, false))); + assert!(wants_tunnel(facts(true, true, true, false))); + } + + #[test] + fn non_local_non_dns_follows_uid_routing() { + assert!(wants_tunnel(facts(false, false, true, false))); + assert!(!wants_tunnel(facts(false, false, false, false))); + assert!(wants_tunnel(facts(false, false, true, true))); + assert!(!wants_tunnel(facts(false, false, false, true))); + } + + #[test] + fn dns_without_direct_redirect_always_tunnels() { + // Every resolver query takes the tunnel regardless of the packet's + // own UID routing, as long as direct apps' DNS isn't being + // redirected to the system resolver instead. + assert!(wants_tunnel(facts(false, true, false, false))); + assert!(wants_tunnel(facts(false, true, true, false))); + } + + #[test] + fn dns_with_direct_redirect_follows_uid_routing() { + assert!(wants_tunnel(facts(false, true, true, true))); + assert!(!wants_tunnel(facts(false, true, false, true))); + } + + #[test] + fn exhaustive_matches_three_branch_definition() { + for local_dest in [false, true] { + for is_dns in [false, true] { + for tunnel_uid in [false, true] { + for dns_direct in [false, true] { + let f = facts(local_dest, is_dns, tunnel_uid, dns_direct); + // Written out independently of wants_tunnel's own + // implementation, so this is a real cross-check and + // not just calling the function on itself. + let expected = if local_dest && !is_dns { + false + } else if is_dns && !dns_direct { + true + } else { + tunnel_uid + }; + assert_eq!( + wants_tunnel(f), + expected, + "mismatch for local_dest={local_dest} is_dns={is_dns} \ + tunnel_uid={tunnel_uid} dns_direct={dns_direct}" + ); + } + } + } + } + } + + #[test] + fn regression_default_mode_matches_pre_per_app_routing_behaviour() { + // Before per-app routing existed, the decision was simply + // `!local_dest || is_dns`. With tunnel_uid=true and dns_direct=false + // (the default-mode combination), wants_tunnel must reduce to + // exactly that expression for every local_dest/is_dns pair, proving + // the default mode is byte-identical to the old behaviour. + for local_dest in [false, true] { + for is_dns in [false, true] { + let f = facts(local_dest, is_dns, true, false); + assert_eq!(wants_tunnel(f), !local_dest || is_dns); + } + } + } + + #[test] + fn empty_table_follows_default_both_directions() { + let tunnel = RouteTable::new(&[], true); + let direct = RouteTable::new(&[], false); + assert!(tunnel.is_tunnel_uid(42)); + assert!(!direct.is_tunnel_uid(42)); + assert!(!tunnel.has_overrides()); + assert!(!direct.has_overrides()); + } + + #[test] + fn negative_uid_follows_default_even_with_overrides() { + let table = RouteTable::new(&[1, 2, 3], false); + assert!(!table.is_tunnel_uid(-1)); + assert_eq!(table.is_tunnel_uid(i32::MIN), table.default_tunnel()); + } + + #[test] + fn listed_uid_inverts_default_true() { + let table = RouteTable::new(&[10, 20], true); + assert!(!table.is_tunnel_uid(10)); + assert!(!table.is_tunnel_uid(20)); + assert!(table.is_tunnel_uid(30)); + } + + #[test] + fn listed_uid_inverts_default_false() { + let table = RouteTable::new(&[10, 20], false); + assert!(table.is_tunnel_uid(10)); + assert!(table.is_tunnel_uid(20)); + assert!(!table.is_tunnel_uid(30)); + } + + #[test] + fn unlisted_uid_in_nonempty_set_follows_default_not_absence_means_direct() { + // Regression pin: an earlier version treated absence from the + // pushed-down set as "direct", which sent a resolved-but-unlisted + // UID (e.g. an app installed after the last reload) out of the + // tunnel even when the global default was to tunnel everything. + // Absence must always mean "follow the default", not "direct". + let tunnel_default = RouteTable::new(&[7], true); + assert!(tunnel_default.is_tunnel_uid(999)); + + let direct_default = RouteTable::new(&[7], false); + assert!(!direct_default.is_tunnel_uid(999)); + } + + #[test] + fn unsorted_and_duplicate_input_is_normalised() { + let a = RouteTable::new(&[5, 1, 5, 3, 1, 3], true); + let b = RouteTable::new(&[1, 3, 5], true); + assert!(a.has_overrides()); + assert!(b.has_overrides()); + for uid in [0, 1, 2, 3, 4, 5, 6] { + assert_eq!(a.is_tunnel_uid(uid), b.is_tunnel_uid(uid)); + } + } + + #[test] + fn boundary_uids() { + let table = RouteTable::new(&[0, i32::MAX], true); + assert!(!table.is_tunnel_uid(0)); + assert!(!table.is_tunnel_uid(i32::MAX)); + // i32::MIN is negative, so it always follows the default rather + // than being looked up in the set. + assert!(table.is_tunnel_uid(i32::MIN)); + assert!(table.is_tunnel_uid(1)); + + let single = RouteTable::new(&[42], false); + assert!(single.is_tunnel_uid(42)); + assert!(!single.is_tunnel_uid(41)); + assert!(!single.is_tunnel_uid(43)); + } + + #[test] + fn has_overrides_reflects_set_emptiness() { + assert!(!RouteTable::new(&[], true).has_overrides()); + assert!(RouteTable::new(&[1], true).has_overrides()); + assert!(!RouteTable::tunnel_all().has_overrides()); + } + + // The FFI surface mutates a process-global (`ROUTES`), and `cargo test` + // runs tests in parallel, so every other test above exercises the pure + // RouteTable/wants_tunnel logic directly. This is the one test allowed + // to touch the global, and it does so as a single ordered sequence. + #[test] + fn ffi_sequence_matches_direct_route_table_use() { + assert_eq!(tc_policy_abi_version(), 1); + + // Set an override set with default_tunnel = false and check it + // against an equivalent RouteTable built directly. + let uids = [3i32, 1, 2, 1]; + let expected = RouteTable::new(&uids, false); + tc_policy_set_route_uids(uids.as_ptr(), uids.len() as core::ffi::c_int, 0); + for uid in [-5, 0, 1, 2, 3, 4, i32::MAX, i32::MIN] { + assert_eq!( + tc_policy_is_tunnel_uid(uid) != 0, + expected.is_tunnel_uid(uid) + ); + } + + // count = 0 with a real pointer yields an empty set. + tc_policy_set_route_uids(uids.as_ptr(), 0, 1); + assert_eq!(tc_policy_is_tunnel_uid(999) != 0, true); + assert_eq!(tc_policy_is_tunnel_uid(1) != 0, true); + + // count < 0 yields an empty set too. + tc_policy_set_route_uids(uids.as_ptr(), -1, 0); + assert_eq!(tc_policy_is_tunnel_uid(1) != 0, false); + + // Null pointer with count > 0 must not be dereferenced, and also + // yields an empty set. + tc_policy_set_route_uids(std::ptr::null(), 5, 1); + assert_eq!(tc_policy_is_tunnel_uid(123) != 0, true); + + // Re-apply overrides, then clear and confirm tunnel-everything. + tc_policy_set_route_uids(uids.as_ptr(), uids.len() as core::ffi::c_int, 1); + assert_eq!(tc_policy_is_tunnel_uid(1) != 0, false); + tc_policy_clear_route_uids(); + assert_eq!(tc_policy_is_tunnel_uid(1) != 0, true); + assert_eq!(tc_policy_is_tunnel_uid(-1) != 0, true); + + // wants_tunnel FFI wrapper returns literal 1/0 matching the direct + // function across representative inputs. + for local_dest in [0, 1] { + for is_dns in [0, 1] { + for tunnel_uid in [0, 1] { + for dns_direct in [0, 1] { + let want = wants_tunnel(facts( + local_dest != 0, + is_dns != 0, + tunnel_uid != 0, + dns_direct != 0, + )); + assert_eq!( + tc_policy_wants_tunnel(local_dest, is_dns, tunnel_uid, dns_direct), + want as core::ffi::c_int + ); + } + } + } + } + } +} From 976de5418d10f95f19ec583f06be6bb046f4ff44 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Thu, 20 Aug 2026 12:50:01 +0200 Subject: [PATCH 2/2] Fail closed when routing policy is unavailable --- app/src/main/jni/netguard/policy.c | 33 ++++++++++++++++++------------ 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/app/src/main/jni/netguard/policy.c b/app/src/main/jni/netguard/policy.c index 061f7813c..ab191b157 100644 --- a/app/src/main/jni/netguard/policy.c +++ b/app/src/main/jni/netguard/policy.c @@ -16,8 +16,9 @@ // wgbridge-rs/src/policy.rs, where `cargo test` covers it in CI; this file // reaches it, caches the handful of facts the packet path needs so the common // case never crosses the boundary at all, and remembers a flow's verdict. -// -// Nothing here decides anything, with one exception marked below. +// The pure packet decision is mirrored below so the packet path does not cross +// the Rust FFI boundary on every packet; the equivalent Rust function remains +// the tested definition of that decision. #include "netguard.h" @@ -92,12 +93,20 @@ void set_route_uids(const jint *uids, int count, int default_tunnel, int dns_dir if (policy_ok) p_set_route_uids(uids, count, default_tunnel); - atomic_store_explicit(&policy_default_tunnel, default_tunnel, memory_order_release); - atomic_store_explicit(&policy_dns_direct, dns_direct, memory_order_release); + // A missing/incompatible bridge cannot honour a selected-app policy. Keep + // the conservative state instead: all eligible traffic takes the tunnel, + // and direct-app DNS is not allowed to opt out of that protection. This is + // deliberately independent of the requested default; otherwise a failed + // load in selected mode would silently route the selected app directly. + int effective_default_tunnel = policy_ok ? (default_tunnel != 0) : 1; + int effective_dns_direct = policy_ok ? (dns_direct != 0) : 0; + atomic_store_explicit(&policy_default_tunnel, effective_default_tunnel, + memory_order_release); + atomic_store_explicit(&policy_dns_direct, effective_dns_direct, + memory_order_release); // Without the bridge there is no table to consult, so pin every UID to the - // global default. A per-app choice is then ignored, which is exactly the - // behaviour that shipped before per-app routing existed — never a leak out - // of the tunnel, never a new drop. + // safe global default. A per-app choice is ignored rather than leaking out + // of the tunnel. atomic_store_explicit(&policy_has_overrides, (policy_ok && count > 0) ? 1 : 0, memory_order_release); @@ -148,12 +157,10 @@ int route_dns_direct() { } int route_wants_tunnel(int local_dest, int is_dns, int tunnel_uid, int dns_direct) { - if (policy_ok) - return p_wants_tunnel(local_dest, is_dns, tunnel_uid, dns_direct); - - // The one decision duplicated outside policy.rs, and only because a packet - // still has to be routed when the bridge could not be resolved. Keep it a - // literal transcription of policy::wants_tunnel. + // This is the literal transcription of policy::wants_tunnel. Keep the + // pure branch local: crossing the Rust boundary for every packet adds + // overhead without consulting any mutable policy state. The Rust function + // remains exported and exhaustively tested as the policy definition. if (local_dest && !is_dns) return 0; if (is_dns && !dns_direct)