refactor(keystore): strip unique-id at parse time
Decide the effective generateKey params once via .let when the caller lacks gen_unique_id / REQUEST_UNIQUE_ID_ATTESTATION, instead of mutating var params/parsedParams deep in handleGenerateKey and re-parsing KeyMintAttestation a second time. isAttestKeyRequest now derives from the final parsedParams, closing the staleness flagged in PR #27 review r3308356496. Behavior is unchanged: no gate between the parse and the old strip site reads INCLUDE_UNIQUE_ID, and the && short-circuits so the permission lookups still run only when the tag is present.
This commit is contained in:
+29
-33
@@ -547,8 +547,35 @@ class KeyMintSecurityLevelInterceptor(
|
||||
SystemLogger.debug(
|
||||
"Handling generateKey ${keyDescriptor.alias}, attestKey=${attestationKey?.alias}"
|
||||
)
|
||||
var params = data.createTypedArray(KeyParameter.CREATOR)!!
|
||||
var parsedParams = KeyMintAttestation(params)
|
||||
// INCLUDE_UNIQUE_ID requires SELinux gen_unique_id OR
|
||||
// REQUEST_UNIQUE_ID_ATTESTATION (AOSP security_level.rs:478-485). AOSP
|
||||
// rejects with PERMISSION_DENIED when neither is held, but that breaks
|
||||
// Google Wallet card binding and regresses Play Integrity: Wallet's
|
||||
// generateKey carries the tag without the permission. Strip it at parse
|
||||
// so the key generates and the attestation omits unique_id (pre-PR157
|
||||
// behavior). Deciding here keeps params/parsedParams val and derives
|
||||
// isAttestKeyRequest from the effective parameters.
|
||||
val params =
|
||||
data.createTypedArray(KeyParameter.CREATOR)!!.let { raw ->
|
||||
val stripUniqueId =
|
||||
raw.any { it.tag == Tag.INCLUDE_UNIQUE_ID } &&
|
||||
!ConfigurationManager.checkSELinuxPermission(
|
||||
callingPid,
|
||||
"keystore_key",
|
||||
"gen_unique_id",
|
||||
) &&
|
||||
!ConfigurationManager.hasPermissionForUid(
|
||||
callingUid,
|
||||
"android.permission.REQUEST_UNIQUE_ID_ATTESTATION",
|
||||
)
|
||||
if (stripUniqueId) {
|
||||
SystemLogger.debug(
|
||||
"[TX_ID: $txId] Stripping INCLUDE_UNIQUE_ID for uid=$callingUid pid=$callingPid (no permission)"
|
||||
)
|
||||
raw.filter { it.tag != Tag.INCLUDE_UNIQUE_ID }.toTypedArray()
|
||||
} else raw
|
||||
}
|
||||
val parsedParams = KeyMintAttestation(params)
|
||||
val isAttestKeyRequest = parsedParams.isAttestKey()
|
||||
|
||||
val hasDeviceIdAttestation =
|
||||
@@ -662,37 +689,6 @@ class KeyMintSecurityLevelInterceptor(
|
||||
return InterceptorUtils.createErrorReply(KEYMINT_CANNOT_ATTEST_IDS)
|
||||
}
|
||||
|
||||
// INCLUDE_UNIQUE_ID requires SELinux gen_unique_id OR
|
||||
// android.permission.REQUEST_UNIQUE_ID_ATTESTATION (AOSP
|
||||
// security_level.rs:478-485). AOSP returns PERMISSION_DENIED
|
||||
// when neither is held — but doing so breaks Google Wallet
|
||||
// card binding (Wallet's generateKey carries the tag without
|
||||
// holding the permission, and Play Integrity also fails when
|
||||
// unique_id ends up in the attestation). Silently strip the
|
||||
// tag so the key generates normally and the resulting
|
||||
// attestation simply omits the unique_id field. This mirrors
|
||||
// the pre-PR157 behavior where the tag had no effect.
|
||||
if (params.any { it.tag == Tag.INCLUDE_UNIQUE_ID }) {
|
||||
val hasSELinux =
|
||||
ConfigurationManager.checkSELinuxPermission(
|
||||
callingPid,
|
||||
"keystore_key",
|
||||
"gen_unique_id",
|
||||
)
|
||||
val hasAndroid =
|
||||
ConfigurationManager.hasPermissionForUid(
|
||||
callingUid,
|
||||
"android.permission.REQUEST_UNIQUE_ID_ATTESTATION",
|
||||
)
|
||||
if (!hasSELinux && !hasAndroid) {
|
||||
SystemLogger.debug(
|
||||
"[TX_ID: $txId] Stripping INCLUDE_UNIQUE_ID for uid=$callingUid pid=$callingPid (no permission)"
|
||||
)
|
||||
params = params.filter { it.tag != Tag.INCLUDE_UNIQUE_ID }.toTypedArray()
|
||||
parsedParams = KeyMintAttestation(params)
|
||||
}
|
||||
}
|
||||
|
||||
val isSymmetric =
|
||||
parsedParams.algorithm == Algorithm.AES ||
|
||||
parsedParams.algorithm == Algorithm.HMAC ||
|
||||
|
||||
Reference in New Issue
Block a user