fix(interception): strip INCLUDE_UNIQUE_ID instead of rejecting when permission missing by Andrea-lyz · Pull Request #27 · Enginex0/TEESimulator-RS

Andrea-lyz · 2026-05-27T04:02:59Z

Summary

The INCLUDE_UNIQUE_ID permission gate in handleGenerateKey rejects
the entire generateKey call with PERMISSION_DENIED when the caller
holds neither SELinux gen_unique_id nor
REQUEST_UNIQUE_ID_ATTESTATION. This breaks Google Wallet on real
devices: Wallet's attestation key request carries INCLUDE_UNIQUE_ID
without the permission, so the request is rejected and Wallet shows
"this phone does not meet the security requirements for Google Wallet".
Clearing GMS data only helps for a few seconds.

Why this fix instead of removing the gate

Naive removal regresses Play Integrity to three-red. AttestationBuilder
honours includeUniqueId == true by computing an HMAC-SHA256
unique_id and embedding it in the attestation extension. With the
gate removed, GMS attestation flows pick the tag through and the
extension ends up with a unique_id that the integrity server flags as
inconsistent for the caller.

Approach

When the permission check fails, silently strip INCLUDE_UNIQUE_ID from
the KeyParameter[] (and re-parse parsedParams) instead of rejecting.
The key generates normally, AttestationBuilder falls through to
else { ByteArray(0) }, and the resulting attestation simply omits the
unique_id field. This matches pre-PR157 behaviour, where the tag had
no effect.

Calls that do hold the permission are unaffected — unique_id is
still emitted as before.

Verification

Tested on a device that previously failed Wallet binding on the PR157
baseline:

Play Integrity: BASIC + DEVICE + STRONG all pass.
Google Wallet: card binding completes successfully.
Other PR157 compliance work (CALLER_NONCE, AuthorizeCreate ordering,
USAGE_COUNT_LIMIT counters, effectiveParams merging) untouched.

Bisect

The regression was bisected commit-by-commit between v5.0-138 and
v5.1-153; isolated to commit 8eda2d8
("feat(interception): add AOSP authorize_create enforcement and wire
format fixes"); within that commit, narrowed to the
INCLUDE_UNIQUE_ID gate specifically.

Summary by CodeRabbit

Bug Fixes
- Key generation now handles unique-ID attestation requests more gracefully: when required permissions are missing, the system logs the condition, removes the unique-ID request, and proceeds rather than rejecting the operation.
- The processing of key parameters can be adjusted during handling so attestation settings are re-evaluated after modifications.

coderabbitai · 2026-05-27T04:03:09Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1030d116-9be9-4963-a9f0-f568ff232282

📥 Commits

Reviewing files that changed from the base of the PR and between 2382e87 and 4d81809.

📒 Files selected for processing (1)

app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt

🚧 Files skipped from review as they are similar to previous changes (1)

app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt

📝 Walkthrough

Walkthrough

KeyMintSecurityLevelInterceptor's key generation now gracefully degrades when Tag.INCLUDE_UNIQUE_ID is requested but required permissions are absent: instead of rejecting the request, the interceptor logs the condition, removes the tag from parameters, re-parses the attestation, and allows key generation to proceed.

Changes

Unique ID Attestation Fallback

Layer / File(s)	Summary
Strip INCLUDE_UNIQUE_ID when permissions missing `app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt`	`params` and `parsedParams` are changed to mutable `var` declarations. The comment block documenting `INCLUDE_UNIQUE_ID` prerequisites is rewritten to reflect stripping behavior. When the tag is present but neither SELinux nor Android permission is granted, an info message is logged, the tag is filtered from `params`, and `KeyMintAttestation` is re-parsed to continue key generation without the unique-id field.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Enginex0/TEESimulator-RS#3: Both PRs modify KeyMintSecurityLevelInterceptor.handleGenerateKey to change how missing attestation-related permissions affect UNIQUE_ID/unique-id attestation tags.

Poem

🐰 A tiny rabbit hops through code so neat,
When UNIQUE_ID is missing, it skips a beat.
It strips the tag, re-parses with flair,
Logs the change gently with attentive care.
Keys still grow strong — a quiet, clever feat.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately and specifically describes the main change: modifying the INCLUDE_UNIQUE_ID permission handling from rejection to stripping the tag when permissions are missing.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Updates the KeyMint security level interceptor to avoid failing generateKey when apps request INCLUDE_UNIQUE_ID without the required permissions, improving compatibility (notably with Google Wallet / Play Integrity flows).

Changes:

Make params/parsedParams mutable so the request can be modified in-flight.
Replace permission-denied behavior for Tag.INCLUDE_UNIQUE_ID with silent stripping of the tag when permissions are missing.
Expand inline documentation explaining the rationale and upstream AOSP behavior.

Comments suppressed due to low confidence (1)

app/src/main/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt:1

isAttestKeyRequest is derived from parsedParams before params can be modified (and parsedParams is later rebuilt after stripping INCLUDE_UNIQUE_ID). This can leave isAttestKeyRequest inconsistent with the final parsed parameters. Compute isAttestKeyRequest after any potential parameter mutation (or recompute it when parsedParams is reassigned).

package org.matrix.TEESimulator.interception.keystore.shim

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ssing permission The INCLUDE_UNIQUE_ID gate in handleGenerateKey (introduced as part of the PR157 AOSP-compliance work) returns PERMISSION_DENIED when the caller holds neither SELinux gen_unique_id nor REQUEST_UNIQUE_ID_ATTESTATION. This breaks Google Wallet card binding on real devices: Wallet's generateKey carries INCLUDE_UNIQUE_ID without holding the permission, so its attestation key request is rejected and Wallet surfaces the failure as "this phone does not meet the security requirements for Google Wallet". Symptoms reported by users: clearing GMS data only helps for a few seconds before the state regresses; no card can be added. Naive removal of the gate is not safe: AttestationBuilder honours `includeUniqueId == true` by computing an HMAC-SHA256 unique_id and embedding it in the attestation extension. With the gate gone, GMS attestation flows that include the tag end up with a unique_id in the extension that Play Integrity flags as inconsistent for the caller, turning all three integrity verdicts red. The fix here splits the difference: when the permission check fails, silently strip the INCLUDE_UNIQUE_ID tag from the KeyParameter array (and re-parse `parsedParams`) instead of rejecting the request. The key generates normally, AttestationBuilder takes the `else { ByteArray(0) }` branch, and the resulting attestation simply omits the unique_id field — matching pre-PR157 behaviour, where the tag effectively had no effect. Verified on a device that previously failed Wallet binding on the PR157 baseline: - Play Integrity: BASIC + DEVICE + STRONG all pass. - Google Wallet: card binding completes successfully. - Calls that DO hold the permission are unaffected (still emit unique_id as before). The rest of the PR157 compliance work (CALLER_NONCE handling, AuthorizeCreate ordering, USAGE_COUNT_LIMIT counters, effectiveParams merging) is preserved.

Grant-plane coherence (Android 16 incl.), Google Wallet + fingerprint compatibility (PR #26/#27), and removal of the in-module PIF/bulletin resolvers. Frozen at gitCommitCount 251.

cokemine · 2026-05-31T14:31:06Z

Thanks. This actually fixed my usage of Google Wallet.

Copilot AI review requested due to automatic review settings May 27, 2026 04:03

Copilot AI reviewed May 27, 2026

View reviewed changes

Comment thread ...n/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt

Comment thread ...n/java/org/matrix/TEESimulator/interception/keystore/shim/KeyMintSecurityLevelInterceptor.kt Outdated

Andrea-lyz force-pushed the pr/fix-gpay-include-unique-id branch from 2382e87 to 4d81809 Compare May 27, 2026 04:07

Enginex0 merged commit d669ec7 into Enginex0:main May 30, 2026
1 check passed

Andrea-lyz deleted the pr/fix-gpay-include-unique-id branch May 30, 2026 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(interception): strip INCLUDE_UNIQUE_ID instead of rejecting when permission missing#27

fix(interception): strip INCLUDE_UNIQUE_ID instead of rejecting when permission missing#27
Enginex0 merged 1 commit into
Enginex0:mainfrom
Andrea-lyz:pr/fix-gpay-include-unique-id

Andrea-lyz commented May 27, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 27, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cokemine commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Andrea-lyz commented May 27, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Why this fix instead of removing the gate

Approach

Verification

Bisect

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Poem

❌ Failed checks (1 warning)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cokemine commented May 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Andrea-lyz commented May 27, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 27, 2026 •

edited

Loading