Skip to content

Commit e95aa90

Browse files
fix(privilege): stop the app-ops fallback reporting a restriction it never applied
The review of this branch found that one rung still did the thing the branch exists to delete. `Shizuku.setAppRestricted`'s reflection fallback ended in `reflectionRan && opReadsBackAsExpected() != false`, defended by a comment claiming `setMode` throws when it is denied. It does not, anywhere in Thor's range: before API 30 a package/uid mismatch met `Slog.w("Bad call: ...")` and a null out of `getOpsRawLocked`, so `setMode` changed nothing and returned normally; from API 30 the check throws, but from inside `verifyAndGetBypass`, where `setMode` catches it (`Slog.e(TAG, "Cannot setMode", e); return;`). The only refusal that ever reaches the caller is `enforceManageAppOpsModes`. So on a denied write `reflectionRan` is true, an unreadable read-back is null, and `true && (null != false)` reported success with nothing restricted. Flipped to `== true`. `reflectionRan` stays in the expression as a guard rather than as evidence: for `restricted = false` the expected mode is MODE_ALLOWED, which is also RUN_ANY_IN_BACKGROUND's platform default, so an op nobody ever wrote reads back as expected and the conjunct is what keeps a throwing `setMode` from being reported as success by the default mode alone. The rule the two polarities follow is now stated once, at the read-back: a rung may fail OPEN on an unreadable read-back only if it has a self-report that is evidence independent of the read-back; with no such evidence it fails CLOSED. Rung 1 has that evidence (`appops set` exits non-zero for an unknown package or op) and keeps `!= false`. Rung 2 has none, because a void return is not a mode. The clear-data sites fall out of the same rule. Also corrected three claims this branch's own comments got wrong: - `verifyIncomingUid` covers API 28-29 only; `checkOperation` dropped it in 30 and current AOSP passes `shouldVerifyUid = false` there explicitly. Routing the read through the privileged binder is still right, for reasons that do span 28..37 — `verifyIncomingPackage` from 31, and `checkOperationUnchecked` answering `opToDefaultMode(code)` rather than throwing when its own `verifyAndGetBypass` fails, which is worse than answering null. - `DhizukuHelper.forceStopApp` rung 1 is not "alive". `execute` runs `am` inside the Dhizuku app via an AIDL call to `IDhizuku.remoteProcess`, at Dhizuku's own app uid, and `ActivityManagerService.forceStopPackage` opens on `FORCE_STOP_PACKAGES`, which device owner does not confer — the same shape already measured one subcommand over in this file (`am get-current-user` denied to uid 10231). The read-back stays: it costs nothing on a rung that short-circuits and guards the ROM that does hold the permission. - `buildSuspendDialogInfo`'s KDoc had Shizuku's answer pasted into Dhizuku. The suspender named there is `BuildConfig.APPLICATION_ID`, not `com.android.shell`. Both `readBackgroundMode` KDocs now record the uid-level blind spot: `checkOperationUnchecked` returns a uid-level mode before it reaches the package entry, both of Thor's writes are package-level, and `checkOperationRaw` is not the fix. Mechanism read out of AppOpsService; no uid-level writer of RUN_ANY_IN_BACKGROUND identified, so it is recorded as a blind spot rather than a bug. `RootSystemGateway.forceStopApp` now keeps the `ApplicationInfo` from its last read so the failure message can say which of `isStoppedNow`'s two falses occurred — "could not read the package" and "it is still running" need different fixes and were being reported as the same sentence. Both surface tests gained a source-text sweep, because reflection is blind to an extension function, a companion member and a `@JvmName` rename while every call site keeps reading the same. Each sweep runs its guards first: corpus size, the anchor file non-empty, the declaration pattern proven against a declaration known to be present, and the extension pattern proven against its own shape. `surfaceNames()` now unions `declaredMethods` with `methods` so moving a member to a super-interface does not take it out of view.
1 parent 3cbb43e commit e95aa90

5 files changed

Lines changed: 598 additions & 81 deletions

File tree

app/src/main/java/com/valhalla/thor/data/gateway/RootSystemGateway.kt

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,10 @@ class RootSystemGateway(
206206
// caller here treats false as "not proven stopped" and does more work, ending at the
207207
// failure below; so "could not read" costs a wasted killBackgroundProcesses and a reported
208208
// failure, never a success that did not happen.
209+
//
210+
// What it does cost is a *sentence*. "FLAG_STOPPED is clear" and "the package could not be
211+
// read" are the same `false` here, so the failure message must not speak for both — see the
212+
// last read below, which keeps its `ApplicationInfo` for exactly that.
209213
fun isStoppedNow(): Boolean = getApplicationInfoCompat(packageName)?.run {
210214
(flags and android.content.pm.ApplicationInfo.FLAG_STOPPED) != 0
211215
} ?: false
@@ -241,21 +245,36 @@ class RootSystemGateway(
241245
am?.killBackgroundProcesses(packageName)
242246
}
243247

244-
if (isStoppedNow()) return Result.success(Unit)
248+
// The last read is spelled out rather than asked for through [isStoppedNow], because the
249+
// failure message below has to say *which* of that function's two `false`s this is, and
250+
// re-reading to find out would describe a different moment than the one that decided.
251+
val postKillInfo = getApplicationInfoCompat(packageName)
252+
if (postKillInfo != null &&
253+
(postKillInfo.flags and android.content.pm.ApplicationInfo.FLAG_STOPPED) != 0
254+
) {
255+
return Result.success(Unit)
256+
}
245257

246258
// Two ways to arrive here now, and a bug report has to be able to tell them apart: the
247259
// shell command itself failed, or it exited 0 and the app kept running anyway. The old
248260
// message asserted the first unconditionally, which the guard above has just made false.
249261
val shellVerdict = if (shellResult.isSuccess) {
250-
"`am force-stop` exited 0 but the app never entered the stopped state"
262+
"`am force-stop` exited 0"
251263
} else {
252264
"the shell command failed"
253265
}
266+
// The same defect one level down, and the reason this is not simply "it is still running":
267+
// that claim rests on [isStoppedNow], where an unreadable `ApplicationInfo` and a genuinely
268+
// clear FLAG_STOPPED are indistinguishable. A bug report generated from "Thor could not
269+
// read the package" must not read as "the kill did not work" — they need different fixes.
270+
val stateVerdict = if (postKillInfo == null) {
271+
"the package's ApplicationInfo could not be read back after killBackgroundProcesses, " +
272+
"so whether it is still running is unknown"
273+
} else {
274+
"FLAG_STOPPED is still clear after killBackgroundProcesses, so it is still running"
275+
}
254276
return Result.failure(
255-
Exception(
256-
"Root force stop of $packageName failed: $shellVerdict, and it is still " +
257-
"running after killBackgroundProcesses."
258-
)
277+
Exception("Root force stop of $packageName failed: $shellVerdict, and $stateVerdict.")
259278
)
260279
}
261280

app/src/main/java/com/valhalla/thor/data/source/local/dhizuku/Dhizuku.kt

Lines changed: 63 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -176,19 +176,38 @@ object DhizukuHelper {
176176
fun forceStopApp(context: Context, packageName: String): Boolean {
177177
val pkgs = Packages(context)
178178
val userId = pkgs.myUserId
179-
// 1. Shell first — and its exit code is not evidence of anything. `am force-stop` cannot
180-
// fail: `ActivityManagerShellCommand.runForceStop` ends in an unconditional `return 0`, so
181-
// exit 0 says the command parsed, never that a process died. The honest verifier was
182-
// already written in this function — `pkgs.isAppStopped`, rung 3 below — and sat unreachable
183-
// behind this short-circuit, which always won. Do not "simplify" the readback back out.
179+
// 1. Shell first — and its exit code is not evidence of anything, in either direction.
180+
// `ActivityManagerShellCommand.runForceStop` ends in an unconditional `return 0`, so exit 0
181+
// says the command parsed, never that a process died. The honest verifier was already
182+
// written in this function — `pkgs.isAppStopped`, rung 3 below — and sat unreachable behind
183+
// this short-circuit. Do not "simplify" the readback back out.
184184
//
185-
// Verifying is worth doing *here*, unlike the reflection rungs of [clearCache] and
186-
// [clearAppData], which were only made honest: this rung is alive. `execute` runs `am`
187-
// inside the device-owner app via `DhizukuAPI.newProcess`, so it reaches
188-
// ActivityManagerService for real — the same fact [setAppDisabledDetailed]'s reflection rung
189-
// records for `pm`. A readback on a live rung turns a false success into a true one; a
190-
// readback on a transport-dead rung turns it into a guaranteed red, which is why those two
191-
// sites report failure instead of waiting for an answer that cannot arrive.
185+
// **Do not read this rung as the live one.** `execute` runs `am` inside the *Dhizuku* app:
186+
// `DhizukuAPI.newProcess` is an AIDL call to `IDhizuku.remoteProcess`, so the child is
187+
// spawned by the device-owner app at its own ordinary app uid. AMS gates what that child
188+
// then asks for — `ActivityManagerService.forceStopPackage` opens on
189+
// `checkCallingPermission(FORCE_STOP_PACKAGES)`, a `signature|privileged` permission that
190+
// holding device owner does not confer — so it throws SecurityException,
191+
// `ShellCommand.exec` prints that to stderr and leaves its `res` at -1, and `am` exits 255
192+
// without `runForceStop` ever reaching its `return 0`. The `&&` below therefore
193+
// short-circuits, and `pkgs.isAppStopped` is not called on this rung at all.
194+
//
195+
// That chain is AOSP-derived rather than measured for `force-stop` itself; its identity
196+
// half is measured on device. The same binary at the same Dhizuku uid is recorded further
197+
// down this file being refused `am get-current-user` — `Permission Denial … uid=10231`,
198+
// exit 255 — which is this exact shape one command over.
199+
//
200+
// The readback stays anyway: it costs nothing on a rung that short-circuits, and it is the
201+
// guard for the one case that would otherwise lie — an exit 0 from a transport that killed
202+
// nothing, which is what a ROM or a Dhizuku build that does hold the permission would
203+
// produce. Nothing is lost when it never runs, because rung 3 re-reads FLAG_STOPPED
204+
// unconditionally: one verifier, reached whichever rung did the work.
205+
//
206+
// The `newProcess` identity fact lives in [setAppDisabledDetailed]'s **KDoc**, where it is
207+
// a caveat — neither of its rungs reaches `PackageManagerService` as uid 2000, which is
208+
// precisely why nothing there trusts an exit code — and not in its reflection rung, whose
209+
// note says the opposite about *itself*: double-wrapped binder, dead on a Dhizuku-only
210+
// device.
192211
val result = execute("am force-stop --user $userId $packageName")
193212
if (result.first == 0 && pkgs.isAppStopped(packageName)) return true
194213

@@ -1001,9 +1020,16 @@ object DhizukuHelper {
10011020
* a `@StringRes int`). On API 29-30 that lookup threw `NoSuchMethodException` out of the caller's
10021021
* `runCatching` and killed the whole reflection path before it ever reached the suspend call.
10031022
*
1004-
* The `@StringRes int` overloads are deliberately not used as a pre-31 fallback: the system
1005-
* resolves such an id against the *suspending* package's resources, which in Dhizuku mode is
1006-
* `com.android.shell`, not us. A missing title is better than a wrong one.
1023+
* The `@StringRes int` overloads are deliberately not used as a pre-31 fallback: the system's
1024+
* `SuspendedAppActivity` resolves such an id against the resources of whichever package the
1025+
* platform *recorded* as the suspender. This helper's only caller is [setAppSuspended]'s
1026+
* reflection rung, which names `BuildConfig.APPLICATION_ID`, so the id would ordinarily land on
1027+
* Thor's own resources — but Dhizuku is the one privilege mode that cannot read the suspension
1028+
* record back at all (no `DUMP`; see [setAppSuspended]), so "ordinarily" is the strongest claim
1029+
* this file can make about where it lands. A literal string is right whoever renders it, and a
1030+
* missing title is better than a wrong one. (`com.android.shell` is
1031+
* `Shizuku.buildSuspendDialogInfo`'s answer, where the caller really is shell uid 2000; it does
1032+
* not transfer here.)
10071033
*/
10081034
@SuppressLint("PrivateApi")
10091035
private fun buildSuspendDialogInfo(context: Context): Any? = runCatching {
@@ -1138,9 +1164,28 @@ object DhizukuHelper {
11381164
* **Expected to answer `null` on a Dhizuku-only device, and that is not a defect.** This rides
11391165
* the same double-wrapped binder as the reflection rung — `asInterface` puts
11401166
* `ShizukuBinderWrapper` on top of Dhizuku's own wrapper — so where that rung is dead this is
1141-
* dead with it, and the shell rung's own honest report is what stands. The readback can only
1142-
* ever *add* confirmation here; it is not load-bearing, which is what makes failing open at the
1143-
* shell rung safe rather than optimistic.
1167+
* dead with it.
1168+
*
1169+
* What a `null` costs depends on which rung asked, and the two are opposites:
1170+
* - **Shell rung** — not load-bearing. `appops set` exits non-zero for an unknown package or
1171+
* op, so that rung's own report is already honest and a `null` leaves it standing. This
1172+
* readback can only *add* confirmation there, which is what makes failing open safe rather
1173+
* than optimistic.
1174+
* - **Reflection rung** — the sole verdict. "The invoke did not throw" is not a mode, so with
1175+
* no readback there is nothing left to believe and a `null` forces `false` — which is what
1176+
* that rung's own comment says. Fail-closed, as at the clear-data sites.
1177+
*
1178+
* **Known blind spot: a uid-level mode hides the package-level one this asks about.**
1179+
* `AppOpsService.checkOperationUnchecked` consults
1180+
* `mAppOpsCheckingService.getUidMode(uidState.uid, persistentDeviceId, code)` first and returns
1181+
* straight away whenever that differs from `AppOpsManager.opToDefaultMode(code)` — before the
1182+
* package entry is consulted at all. Both write paths in [setAppRestricted] are *package*-level
1183+
* (`appops set <pkg>` and `IAppOpsService.setMode(op, uid, packageName, mode)`), so wherever a
1184+
* uid-level mode exists for `OP_RUN_ANY_IN_BACKGROUND` this reports something unrelated to
1185+
* whether the write landed. `checkOperationRaw` is not the fix: it drops `evalMode`, not the
1186+
* uid short-circuit. The mechanism is AOSP-verified; what is *not* established is that anything
1187+
* writes this op at uid level — Settings' Battery ▸ Restricted uses the package-level form — so
1188+
* this is a known blind spot rather than an observed bug.
11441189
*
11451190
* `checkOperation(int, int, String)` is the signature this project has *not* verified across
11461191
* 28..37 — it has been stable in `IAppOpsService` for as long as anyone has needed it, but that

0 commit comments

Comments
 (0)