fix: restrict UID spoofing exclusively to the TEESimulator daemon to prevent root process crashes#42
Open
aartzz wants to merge 2 commits into
Open
fix: restrict UID spoofing exclusively to the TEESimulator daemon to prevent root process crashes#42aartzz wants to merge 2 commits into
aartzz wants to merge 2 commits into
Conversation
…emon Previously, all binder transactions with a sender_euid of 0 (root) were indiscriminately spoofed to UID 1000. While this successfully bypassed keystore permissions for the TEESimulator daemon, it unintentionally intercepted and spoofed legitimate transactions from other root processes such as cameraserver, vold, and init. This indiscriminate spoofing caused severe side effects on some devices, such as breaking the camera HAL on MTK devices (crashing cameraserver) and causing soft bootloops due to vold crashes when accessing keystore2. This commit resolves the issue by memorizing the daemon's precise PID during the backdoor registration phase. UID spoofing is now strictly limited to transactions originating from the daemon's PID. All other root processes fall through without being hijacked or spoofed, retaining their original UID 0 and passing directly to keystore2.
📝 WalkthroughWalkthroughThe binder interceptor records the daemon PID during the backdoor handshake and restricts root UID spoofing to transactions from that daemon. The update manifest changes the release version code and download URL. ChangesBinder daemon identity handling
Release metadata update
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/cpp/binder_interceptor.cpp`:
- Around line 375-385: The daemon PID state in the binder interception flow is
never invalidated after daemon termination. Update the teardown path associated
with the backdoor handshake to reset g_daemon_pid when the daemon exits, or
validate that the recorded PID is still the live daemon before the sender_euid
spoofing branch uses it; ensure recycled PIDs cannot trigger UID 1000 spoofing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 601d2547-29a7-47d8-96f7-8c3a61ac2d89
📒 Files selected for processing (1)
app/src/main/cpp/binder_interceptor.cpp
Comment on lines
+375
to
+385
| // Save the daemon's PID | ||
| g_daemon_pid = txn_data->sender_pid; | ||
| // Check 2: Bypass interception for root requests, but spoof UID for the daemon | ||
| } else if (txn_data->sender_euid == 0) { | ||
| // The kernel driver fills sender_euid. | ||
| // libbinder.so trusts this value to populate IPCThreadState. | ||
| txn_data->sender_euid = 1000; | ||
| LOGV("[Hook] Spoofing UID for transaction: 0 -> %d", txn_data->sender_euid); | ||
| hijack = false; // Never hijack to avoid recursion | ||
| if (txn_data->sender_pid == g_daemon_pid) { | ||
| // The kernel driver fills sender_euid. | ||
| // libbinder.so trusts this value to populate IPCThreadState. | ||
| txn_data->sender_euid = 1000; | ||
| LOGV("[Hook] Spoofing UID for transaction: 0 -> %d", txn_data->sender_euid); | ||
| } | ||
| hijack = false; // Never hijack root processes (vold, init, cameraserver) to avoid recursion and bootloops |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for any code that resets or clears g_daemon_pid
rg -n 'g_daemon_pid' app/src/main/cpp/Repository: Enginex0/TEESimulator-RS
Length of output: 435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== binder_interceptor.cpp around the g_daemon_pid logic =="
sed -n '200,410p' app/src/main/cpp/binder_interceptor.cpp
echo
echo "== Search for daemon lifecycle / cleanup / unregister paths =="
rg -n 'g_daemon_pid|unregister|unregister.*daemon|death|linkToDeath|unlinkToDeath|cleanup|reset.*pid|pid.*reset|daemon.*die|TEESimulator|mount\(' app/src/main/cpp/ app/src/main/ -SRepository: Enginex0/TEESimulator-RS
Length of output: 48360
Reset g_daemon_pid on daemon death. g_daemon_pid is only set during the backdoor handshake and never cleared, so a recycled PID could cause unrelated root transactions to get spoofed to UID 1000. Clear it on teardown or add a liveness check before spoofing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/cpp/binder_interceptor.cpp` around lines 375 - 385, The daemon
PID state in the binder interception flow is never invalidated after daemon
termination. Update the teardown path associated with the backdoor handshake to
reset g_daemon_pid when the daemon exits, or validate that the recorded PID is
still the live daemon before the sender_euid spoofing branch uses it; ensure
recycled PIDs cannot trigger UID 1000 spoofing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@module/update.json`:
- Around line 3-4: Regenerate module/update.json using the packaging workflow
driven by app/build.gradle.kts, rather than editing its fields individually.
Ensure the generated version, versionCode, and zipUrl consistently use the same
canonical release metadata and artifact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+3
to
+4
| "versionCode": 304, | ||
| "zipUrl": "https://github.com/aartzz/TEESimulator-RS/releases/download/v6.0.1-304/TEESimulator-RS-v6.0.1-309-Release.zip", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'module/update.json' 'app/build.gradle.kts' || true
printf '\n== module/update.json ==\n'
if [ -f module/update.json ]; then
cat -n module/update.json
fi
printf '\n== app/build.gradle.kts outline ==\n'
if [ -f app/build.gradle.kts ]; then
ast-grep outline app/build.gradle.kts --view expanded || true
fi
printf '\n== search for version/tag/url generation ==\n'
rg -n --no-heading -S 'versionCode|zipUrl|version|release|download|aartzz|Enginex0|TEESimulator-RS' app/build.gradle.kts module/update.json . || trueRepository: Enginex0/TEESimulator-RS
Length of output: 47406
Regenerate module/update.json from the packaging workflow
version, versionCode, and zipUrl are out of sync here: the JSON mixes v6.0.1-307, 304, aartzz, and v6.0.1-309. That can make update checks treat the release as a downgrade or point users at the wrong artifact. app/build.gradle.kts already writes this file from the canonical release metadata, so update this file through that pipeline instead of editing the fields separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@module/update.json` around lines 3 - 4, Regenerate module/update.json using
the packaging workflow driven by app/build.gradle.kts, rather than editing its
fields individually. Ensure the generated version, versionCode, and zipUrl
consistently use the same canonical release metadata and artifact.
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.
Problem
Currently, the
binder_interceptorindiscriminately spoofs all binder transactions originating fromsender_euid == 0(root) toUID 1000. While this approach successfully allows the TEESimulator daemon to bypass keystore permissions, it unintentionally intercepts and alters legitimate transactions from other vital system root processes (such ascameraserver,vold,init, etc.) when they attempt to communicate withkeystore2.On many devices (especially MediaTek devices), this indiscriminate spoofing causes severe side effects:
cameraserverrequests to keystore fail authorization because they are spoofed asUID 1000, leading to aSIGABRTcrash (queryMinBufNumfailure in MTK camera pipelines).voldcrash during boot when accessing keystore2 due to intercepted and hijacked requests.Solution
This PR introduces a more precise UID spoofing mechanism:
g_daemon_pid) during the initial backdoor registration phase (intercept::kBackdoorCode).sender_euid = 1000) is now strictly limited to transactions that originate from this specificg_daemon_pid.UID 0and are explicitly excluded from hijacking (hijack = false), allowing them to interact securely and directly withkeystore2without tampering.Testing
cameraserver) works flawlessly alongside the module (noSIGABRT).vold-related soft bootloops.Summary by CodeRabbit