Cover the bind vector below kernel 5.9, and stop a probe panic from killing the app - #310
Merged
Merged
Conversation
@sliva_ru reported `setsockopt SO_BINDTODEVICE tun0` leaking on a self-built LineageOS sm8350 (5.4.302-qgki) while the KPM was fully healthy — every hook installed, error 0x0. The vector was never covered there: the resolved-ifindex hook was wired only for kernels in [5.7, 5.9), and below that we deliberately relied on the kernel refusing an unprivileged bind without CAP_NET_RAW. That assumption came from upstream sources and the QEMU reference images, and it does not survive contact with vendor trees. His kernel has the gate compiled in — `sock_bindtoindex_locked` calls `ns_capable`, verified by disassembling the vmlinux rebuilt from his boot image — and an untrusted app still bound a socket to tun0. Whatever satisfies the check on that ROM, a kernel-side policy we do not control cannot back a coverage claim. - probe both helper names by symbol on any kernel below 5.9: upstream renamed sock_setbindtodevice_locked to sock_bindtoindex_locked in 5.8, and 5.4 vendor trees backport the newer one (his does) - when neither resolves, leave the hook bit clear rather than reporting a vector we do not cover - the partial-hooks warning now fires only when a missing hook costs a measured vector, so kernels that never had the symbol — and close the surface by capability or SELinux anyway — stay quiet - correct the invariant in detection-vectors.md and at the guard in socket_bind_before_common, with the counterexample, so it does not get reintroduced. The zygisk hook still carries the same version gate; its oracle argument holds only while every bind is refused, noted in the doc as wanting a runtime probe.
Two devices report the app dying at startup whenever a VPN interface is up, with nothing in the app's own logs. The probe crate built with `panic = "abort"`, and the full check suite runs on every cold start, so any panic in a probe — parsing whatever an arbitrary vendor kernel hands back — took the process down with SIGABRT and no Java trace. The JNI entry already wrapped the run in `catch_unwind` (via jni's `with_env` + ThrowRuntimeExAndDefault); abort made that dead code. - release and dev profiles unwind. Costs ~39 KB of unwind tables on arm64, the only ABI we ship - a panic hook logs the message and its file:line to logcat under VpnHide-Native, which the bundle's filter now captures. The default hook writes to stderr, which for an app process goes nowhere — this is why such a crash leaves no trace. Backtraces are deliberately not attempted: fat LTO + strip would make them unsymbolised addresses - the Kotlin caller swallows a thrown probe run, so the worst case is one empty check run instead of a dead app - pass the SIOCGIFFLAGS/SIOCGIFMTU ifreq by unique reference. The kernel writes the result through that pointer, so deriving it from a shared borrow is UB — and it is the tun0-visible branch that reads the value back, exactly the state these reports are about
…d path The SIOCGIF* pre-screen dereferenced the caller's `arg` before the real ioctl had a chance to validate it. A target app passing a bad or short pointer — the case the kernel answers with EFAULT — instead took a SIGSEGV inside its own process, caused by our hook. The setsockopt hook was written to avoid exactly this (copy_from_self, with the reasoning in its doc comment); the ioctl path was the inconsistent one. Reads the 16-byte name through copy_from_self and passes a non-socket fd straight through, since this ioctl family cannot apply to one. Fault containment itself is already covered by self_copy_contains_bad_caller_pointers. Found by a review pass over the project's unsafe code.
`writeSuperkeySetting` and `writeFilesystemHidingSetting` each change one field in `settings`, but took their base from `buildCanonicalConfigFromTargetsSnapshot` — rebuilding every app's roles from the snapshot's per-role sets and writing that back. One of those sets round-tripped through UIDs: `appHiding` was stored as resolved UIDs and mapped back through `pm list packages`, so a target the inventory could not see (a profile the scan failed to read — the case #293 added diagnostics for) disappeared from the projection and lost its role on disk. A toggle unrelated to the app list silently unconfigured an app. - both writers take `snapshot.canonicalConfig` and copy the one field - TargetsSnapshot no longer stores the five per-role sets beside the config they came from. They are projections of it now, so the object cannot carry two versions of one truth, and `observerNames` no longer passes through UIDs at all. `observerUids` stays for the consumers that need the wire's language, derived on demand - regression test: an app-hiding target absent from the inventory keeps its role Not reproduced on a device — the chain is read off the code, and the missing-from-inventory precondition is one users have hit.
The index-helper hook runs before that helper's own CAP_NET_RAW check, so it answered ENODEV even for callers the kernel was about to refuse with EPERM. On a tree where the check bites, a VPN name then reads differently from every other name — the exact oracle the Zygisk hook refuses to create — and widening the hook to all kernels below 5.9 in the previous commit widened that to the 4.x families where the check really does bite. Ask capable(CAP_NET_RAW) first and stay out of the way when it fails: the kernel refuses those callers itself, identically for every interface. Deny only the ones that would otherwise have bound, which is the case the LineageOS 5.4 report is about. capable() needs no struct offsets, so it cannot go stale on a vendor kernel the way an offset table can; a kernel where it will not resolve keeps the previous unconditional denial.
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
1 participant
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.
Four fixes off two bug reports and a review pass over the project's unsafe Rust.
@sliva_ru reported
setsockopt SO_BINDTODEVICE tun0leaking on a self-built LineageOS sm8350 (5.4.302-qgki) while the KPM was fully healthy — every hook installed, error 0x0. The vector was never covered there: the resolved-ifindex hook was wired only for kernels in [5.7, 5.9), and below that we relied on the kernel refusing an unprivileged bind without CAP_NET_RAW. That assumption came from upstream sources and the QEMU reference images. His kernel has the gate compiled in —sock_bindtoindex_lockedcallsns_capable, verified by disassembling the vmlinux rebuilt from his boot image — and an untrusted app still bound a socket to tun0. A kernel-side policy we do not control cannot back a coverage claim.He also has a second device where the app dies at startup whenever a VPN is up, with nothing in its logs. No crash log yet, but the probe crate built with
panic = "abort"and runs its whole check suite on every cold start, so any panic parsing an unexpected kernel reply took the process down silently.sock_setbindtodevice_lockedtosock_bindtoindex_lockedin 5.8; vendor 5.4 trees backport the newer one). A tree exporting neither leaves the hook bit clear instead of claiming a vector it does not covercatch_unwindits JNI entry already had; a panic hook logs the message and file:line to logcat underVpnHide-Native, and the Kotlin caller swallows a thrown runSIOCGIFFLAGS/SIOCGIFMTUpass their ifreq by unique reference — the kernel writes the result through that pointer, and it is the tun0-visible branch that reads it backSIOCGIF*pre-screen reads the interface name through the fault-contained path instead of dereferencing the caller'sargbefore the kernel validates it; a target app passing a bad pointer used to take a SIGSEGV from our hook where the kernel would have returned EFAULTSuperKey, experimental protection) take the stored config instead of rebuilding every app's roles from the snapshot's projections.appHidinground-tripped through UIDs, so a target the package inventory could not see lost its role on save —TargetsSnapshotnow projects the per-role sets from the config rather than storing them beside itThe bind fix is not yet confirmed on a device; @sliva_ru has the details. The docs correction and the guard comment carry the counterexample so the invariant does not come back. Zygisk still carries the same version gate — its oracle argument holds only while every bind is refused, noted in detection-vectors.md as wanting a runtime probe.