fix(interception): Add permission checks for KeyMintSecurityLevelInterceptor and fix some regression#3
Conversation
…rceptor and fix some regression 1. Add permission checks for KeyMintSecurityLevelInterceptor to ensure that only authorized users can access sensitive information about the security level of the key mint. 2. Fix regression where device id attestation was allowed for all users by adding appropriate permission checks. 3. Update .gitignore to exclude build artifacts and generated files to keep the repository clean and prevent accidental commits of unnecessary files.
📝 WalkthroughWalkthroughThis PR adds permission enforcement to Android attestation operations. A new AndroidPermissionUtils utility provides permission checking via reflection to obtain system context, while KeyMintSecurityLevelInterceptor is modified to enforce device attestation and unique ID attestation permissions during key generation, rejecting requests lacking proper permissions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip You can disable the changed files summary in the walkthrough.Disable the |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/src/main/java/org/matrix/TEESimulator/util/AndroidPermissionUtils.kt (1)
11-37: Cache the resolved global context to avoid repeated reflection per check.
getGlobalContext()currently reflects on every permission check. This runs on keygen paths and is easy to memoize.♻️ Proposed refactor
object AndroidPermissionUtils { + `@Volatile` + private var cachedContext: Context? = null + `@SuppressLint`("PrivateApi", "DiscouragedPrivateApi") private fun getGlobalContext(): Context? { + cachedContext?.let { return it } return try { // 1. Get the hidden ActivityThread class via reflection val activityThreadClass = Class.forName("android.app.ActivityThread") @@ - if (application != null) return application + if (application != null) { + cachedContext = application + return application + } @@ - getSystemContextMethod.invoke(activityThread) as? Context + (getSystemContextMethod.invoke(activityThread) as? Context)?.also { + cachedContext = it + } } catch (e: Exception) { SystemLogger.error("Reflection failed to get global context for permission check", e) null } }Also applies to: 47-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/org/matrix/TEESimulator/util/AndroidPermissionUtils.kt` around lines 11 - 37, getGlobalContext() performs reflection on every call; memoize the resolved Context to avoid repeated reflection by adding a single cached field (e.g., a private volatile var cachedGlobalContext: Context?) and returning it early if non-null; on first resolution (inside getGlobalContext() after successfully obtaining application or system context) assign the cache and return it. Ensure thread-safety when initializing the cache (use a synchronized block or double-checked locking) so concurrent calls won't repeat reflection. Update any other callers of getGlobalContext() to rely on the cached value implicitly; keep the existing reflection fallback logic intact for the initial resolution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt`:
- Around line 271-287: The permission checks in KeyMintSecurityLevelInterceptor
currently use the marshaled callingUid (from
BinderInterceptor.handlePreTransact) which can be spoofed; instead obtain the
real caller identity via android.os.Binder.getCallingUid() and use that UID for
AndroidPermissionUtils.hasUniqueIdAttestationPermission(...) and
AndroidPermissionUtils.hasDeviceAttestationPermission(...). Update the code
paths that check Tag.DEVICE_UNIQUE_ATTESTATION and the hasDeviceIdAttestation
block to call Binder.getCallingUid(), replace uses of the marshaled callingUid
with this actualCallingUid, and (optionally) log and reject if the marshaled
callingUid differs from the real caller (include txId in logs).
- Around line 276-282: Remove Tag.DEVICE_UNIQUE_ATTESTATION from the device-ID
attestation boolean so it isn't double-gated: in KeyMintSecurityLevelInterceptor
(where hasDeviceIdAttestation is computed), stop including
Tag.DEVICE_UNIQUE_ATTESTATION in the params.any { ... } check because
DEVICE_UNIQUE_ATTESTATION is already handled separately by the
REQUEST_UNIQUE_ID_ATTESTATION gating logic elsewhere in the class; update the
hasDeviceIdAttestation expression to only check the IMEI/MEID/SERIAL/SECOND_IMEI
tags.
---
Nitpick comments:
In `@app/src/main/java/org/matrix/TEESimulator/util/AndroidPermissionUtils.kt`:
- Around line 11-37: getGlobalContext() performs reflection on every call;
memoize the resolved Context to avoid repeated reflection by adding a single
cached field (e.g., a private volatile var cachedGlobalContext: Context?) and
returning it early if non-null; on first resolution (inside getGlobalContext()
after successfully obtaining application or system context) assign the cache and
return it. Ensure thread-safety when initializing the cache (use a synchronized
block or double-checked locking) so concurrent calls won't repeat reflection.
Update any other callers of getGlobalContext() to rely on the cached value
implicitly; keep the existing reflection fallback logic intact for the initial
resolution.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f11b39f8-1caf-4c97-8272-09e6c4136c7e
📒 Files selected for processing (3)
.gitignoreapp/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.ktapp/src/main/java/org/matrix/TEESimulator/util/AndroidPermissionUtils.kt
| if (params.any { it.tag == Tag.DEVICE_UNIQUE_ATTESTATION } && !AndroidPermissionUtils.hasUniqueIdAttestationPermission(callingUid)) { | ||
| SystemLogger.warning("[TX_ID: $txId] Rejecting DEVICE_UNIQUE_ATTESTATION for uid=$callingUid") | ||
| return InterceptorUtils.createErrorReply(KEYMINT_CANNOT_ATTEST_IDS) | ||
| } | ||
|
|
||
| val hasDeviceIdAttestation = params.any { | ||
| it.tag == Tag.ATTESTATION_ID_IMEI || | ||
| it.tag == Tag.ATTESTATION_ID_MEID || | ||
| it.tag == Tag.ATTESTATION_ID_SERIAL || | ||
| it.tag == Tag.DEVICE_UNIQUE_ATTESTATION || | ||
| it.tag == Tag.ATTESTATION_ID_SECOND_IMEI | ||
| } | ||
|
|
||
| if(hasDeviceIdAttestation && !AndroidPermissionUtils.hasDeviceAttestationPermission(callingUid)) { | ||
| SystemLogger.warning("[TX_ID: $txId] Rejecting DEVICE_ID_ATTESTATION for uid=$callingUid") | ||
| return InterceptorUtils.createErrorReply(KEYMINT_CANNOT_ATTEST_IDS) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Trace source of callingUid in BinderInterceptor =="
fd 'BinderInterceptor.kt$' -t f | while read -r f; do
echo "-- $f"
rg -n -C4 'val callingUid = data.readInt\(\)|onPreTransact\(' "$f"
done
echo
echo "== Trace permission checks consuming callingUid =="
rg -n --type=kt -C4 'hasUniqueIdAttestationPermission\(callingUid\)|hasDeviceAttestationPermission\(callingUid\)'
echo
echo "== Check whether trusted Binder identity is used nearby =="
rg -n --type=kt -C3 'Binder\.getCallingUid\('Repository: Enginex0/TEESimulator-RS
Length of output: 1970
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Permission checks in KeyMintSecurityLevelInterceptor =="
rg -n -C4 'hasUniqueIdAttestationPermission|hasDeviceAttestationPermission' --type-list
echo
echo "== Search without type flag =="
rg -n -C3 'hasUniqueIdAttestationPermission\(callingUid\)|hasDeviceAttestationPermission\(callingUid\)' app/
echo
echo "== Check for Binder.getCallingUid() usage =="
rg -n -C3 'Binder\.getCallingUid\(\)' app/
echo
echo "== Check context around line 271-287 in KeyMintSecurityLevelInterceptor =="
sed -n '260,295p' app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt | cat -nRepository: Enginex0/TEESimulator-RS
Length of output: 533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Permission checks in KeyMintSecurityLevelInterceptor =="
rg -n -C3 'hasUniqueIdAttestationPermission\(callingUid\)|hasDeviceAttestationPermission\(callingUid\)' app/
echo
echo "== Check for Binder.getCallingUid() usage =="
rg -n -C3 'Binder\.getCallingUid\(\)' app/
echo
echo "== Check context around line 271-287 in KeyMintSecurityLevelInterceptor =="
sed -n '260,295p' app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt | cat -n
echo
echo "== Verify AndroidPermissionUtils implementation =="
rg -n 'fun hasUniqueIdAttestationPermission|fun hasDeviceAttestationPermission' app/Repository: Enginex0/TEESimulator-RS
Length of output: 2578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find AndroidPermissionUtils implementation =="
fd 'AndroidPermissionUtils' app/ -t f
echo
echo "== Read AndroidPermissionUtils.kt =="
find app -name AndroidPermissionUtils.kt -exec cat -n {} \;
echo
echo "== Check where callingUid parameter comes from in KeyMintSecurityLevelInterceptor =="
sed -n '200,280p' app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt | cat -n
echo
echo "== Search for onPreTransact override in KeyMintSecurityLevelInterceptor =="
rg -n -A 15 'override fun onPreTransact' app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.ktRepository: Enginex0/TEESimulator-RS
Length of output: 8794
Do not authorize attestation based on marshaled callingUid alone.
Lines 271 and 284 enforce permissions using callingUid, which is propagated from parcel decoding in BinderInterceptor.handlePreTransact (line 133: val callingUid = data.readInt()). This marshaled value is passed directly to AndroidPermissionUtils.hasUniqueIdAttestationPermission(callingUid) and AndroidPermissionUtils.hasDeviceAttestationPermission(callingUid) without verification against the actual Binder calling identity. If that marshaled value is tampered in the hook path, a caller can spoof a privileged UID and bypass these permission checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt`
around lines 271 - 287, The permission checks in KeyMintSecurityLevelInterceptor
currently use the marshaled callingUid (from
BinderInterceptor.handlePreTransact) which can be spoofed; instead obtain the
real caller identity via android.os.Binder.getCallingUid() and use that UID for
AndroidPermissionUtils.hasUniqueIdAttestationPermission(...) and
AndroidPermissionUtils.hasDeviceAttestationPermission(...). Update the code
paths that check Tag.DEVICE_UNIQUE_ATTESTATION and the hasDeviceIdAttestation
block to call Binder.getCallingUid(), replace uses of the marshaled callingUid
with this actualCallingUid, and (optionally) log and reject if the marshaled
callingUid differs from the real caller (include txId in logs).
| val hasDeviceIdAttestation = params.any { | ||
| it.tag == Tag.ATTESTATION_ID_IMEI || | ||
| it.tag == Tag.ATTESTATION_ID_MEID || | ||
| it.tag == Tag.ATTESTATION_ID_SERIAL || | ||
| it.tag == Tag.DEVICE_UNIQUE_ATTESTATION || | ||
| it.tag == Tag.ATTESTATION_ID_SECOND_IMEI | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In current AOSP Keystore/KeyMint generateKey permission checks, what permission is required for Tag.DEVICE_UNIQUE_ATTESTATION, and is READ_PRIVILEGED_PHONE_STATE also required for that tag (separate from ATTESTATION_ID_IMEI/MEID/SERIAL/SECOND_IMEI)?
💡 Result:
Tag::DEVICE_UNIQUE_ATTESTATIONrequiresandroid.permission.READ_PRIVILEGED_PHONE_STATEin the Keystore2/KeyMintgenerateKeypath, because Keystore2 classifiesDEVICE_UNIQUE_ATTESTATIONas a “device ID attestation tag” and gates any such tag withcheck_device_attestation_permissions(), which checksREAD_PRIVILEGED_PHONE_STATE. [1][2]- Yes,
READ_PRIVILEGED_PHONE_STATEis required forDEVICE_UNIQUE_ATTESTATIONeven if you are not requestingATTESTATION_ID_IMEI/MEID/SERIAL/SECOND_IMEI—it’s enforced for the whole group sinceDEVICE_UNIQUE_ATTESTATIONis explicitly included in the sameis_device_id_attestation_tag(...)set. [1][2]
(Separately: Tag::INCLUDE_UNIQUE_ID is gated by either the keystore SELinux key permission GenUniqueId or the Android permission android.permission.REQUEST_UNIQUE_ID_ATTESTATION.) [2][1]
🏁 Script executed:
cat -n app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt | sed -n '260,290p'Repository: Enginex0/TEESimulator-RS
Length of output: 2258
🏁 Script executed:
fd -t f AndroidPermissionUtils.kt && cat -n $(fd -t f AndroidPermissionUtils.kt) | head -100Repository: Enginex0/TEESimulator-RS
Length of output: 3530
DEVICE_UNIQUE_ATTESTATION is double-gated with conflicting permission requirements.
Line 271 enforces REQUEST_UNIQUE_ID_ATTESTATION for Tag.DEVICE_UNIQUE_ATTESTATION, but including the same tag in hasDeviceIdAttestation (line 280) also requires READ_PRIVILEGED_PHONE_STATE via line 284. This creates a double-gating scenario that may reject valid callers. In AOSP Keystore/KeyMint semantics, DEVICE_UNIQUE_ATTESTATION is a device ID attestation tag that should be gated by READ_PRIVILEGED_PHONE_STATE, separate from the unique ID attestation permission (REQUEST_UNIQUE_ID_ATTESTATION).
Remove DEVICE_UNIQUE_ATTESTATION from the hasDeviceIdAttestation check since it is already handled separately at line 271:
Suggested fix
val hasDeviceIdAttestation = params.any {
it.tag == Tag.ATTESTATION_ID_IMEI ||
it.tag == Tag.ATTESTATION_ID_MEID ||
it.tag == Tag.ATTESTATION_ID_SERIAL ||
- it.tag == Tag.DEVICE_UNIQUE_ATTESTATION ||
it.tag == Tag.ATTESTATION_ID_SECOND_IMEI
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val hasDeviceIdAttestation = params.any { | |
| it.tag == Tag.ATTESTATION_ID_IMEI || | |
| it.tag == Tag.ATTESTATION_ID_MEID || | |
| it.tag == Tag.ATTESTATION_ID_SERIAL || | |
| it.tag == Tag.DEVICE_UNIQUE_ATTESTATION || | |
| it.tag == Tag.ATTESTATION_ID_SECOND_IMEI | |
| } | |
| val hasDeviceIdAttestation = params.any { | |
| it.tag == Tag.ATTESTATION_ID_IMEI || | |
| it.tag == Tag.ATTESTATION_ID_MEID || | |
| it.tag == Tag.ATTESTATION_ID_SERIAL || | |
| it.tag == Tag.ATTESTATION_ID_SECOND_IMEI | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt`
around lines 276 - 282, Remove Tag.DEVICE_UNIQUE_ATTESTATION from the device-ID
attestation boolean so it isn't double-gated: in KeyMintSecurityLevelInterceptor
(where hasDeviceIdAttestation is computed), stop including
Tag.DEVICE_UNIQUE_ATTESTATION in the params.any { ... } check because
DEVICE_UNIQUE_ATTESTATION is already handled separately by the
REQUEST_UNIQUE_ID_ATTESTATION gating logic elsewhere in the class; update the
hasDeviceIdAttestation expression to only check the IMEI/MEID/SERIAL/SECOND_IMEI
tags.
Summary by CodeRabbit
Security
Chores