review: clean error codes, defensive symmetric fallback, v3 doc
Address Copilot/CodeRabbit review feedback on the persistence PR. 1. SoftwareOperation: replace requireNotNull(keyPair) in SIGN/VERIFY/AGREE_KEY branches with ServiceSpecificException(invalidArgument). The original requireNotNull throws IllegalArgumentException, which the binder layer wraps as KEYMINT_UNKNOWN_ERROR — defeating the goal of surfacing a clean keystore-style error. Aligns with how ENCRYPT/DECRYPT already handle missing key material in the same when block. 2. loadPersistedKeys: when a symmetric record has empty metadataBytes (e.g. a save where Parcel.marshall() was empty for any reason), rebuild a minimal KeyMetadata from PersistedKeyData primitive fields instead of skipping the record. Skipping silently dropped the AES key, which is the same 'logged out after reboot' behavior the PR is trying to fix. The rebuilt metadata is structurally minimal but preserves the secret material, which is the dominant correctness concern. 3. Comment fix: rebuildResponseFromRecord docs referred to 'v2 metadata snapshot' — the format in this PR is v3.
This commit is contained in:
Andrea-lyz
+91
-7
@@ -911,16 +911,22 @@ class KeyMintSecurityLevelInterceptor(
|
||||
}
|
||||
}.getOrElse { e ->
|
||||
SystemLogger.warning(
|
||||
"Failed to restore symmetric metadata for ${record.alias}",
|
||||
"Failed to restore symmetric metadata for ${record.alias}, falling back to primitive rebuild",
|
||||
e,
|
||||
)
|
||||
return@runCatching
|
||||
rebuildSymmetricResponse(record)
|
||||
}
|
||||
} else {
|
||||
// Pre-v3 file with symmetric key — should not happen
|
||||
// because v3 always saves metadata, but be defensive.
|
||||
SystemLogger.warning("Symmetric record ${record.alias} missing metadata bytes, skipping")
|
||||
return@runCatching
|
||||
// because v3 always saves metadata, but be defensive:
|
||||
// rebuild a minimal KeyMetadata from primitives so
|
||||
// the secret material is still restored. Without
|
||||
// this, dropping the record would silently log the
|
||||
// user out the next time the alias is used.
|
||||
SystemLogger.info(
|
||||
"Symmetric record ${record.alias} missing metadata bytes, rebuilding from primitives"
|
||||
)
|
||||
rebuildSymmetricResponse(record)
|
||||
}
|
||||
generatedKeys[keyId] = GeneratedKeyInfo(
|
||||
keyPair = null,
|
||||
@@ -1020,11 +1026,11 @@ class KeyMintSecurityLevelInterceptor(
|
||||
}
|
||||
|
||||
/**
|
||||
* Fallback rebuild path used when no v2 metadata snapshot is available
|
||||
* Fallback rebuild path used when no v3 metadata snapshot is available
|
||||
* (key was saved by an older build, or the snapshot failed to deserialize).
|
||||
* Rebuilds KeyEntryResponse from primitive fields. This loses any
|
||||
* authorization tags that weren't captured at save time, which is why we
|
||||
* prefer the byte-identical v2 snapshot whenever possible.
|
||||
* prefer the byte-identical v3 snapshot whenever possible.
|
||||
*/
|
||||
private fun rebuildResponseFromRecord(
|
||||
record: PersistedKeyData,
|
||||
@@ -1078,6 +1084,84 @@ class KeyMintSecurityLevelInterceptor(
|
||||
return buildKeyEntryResponse(record.uid, certChain, attestation, descriptor)
|
||||
}
|
||||
|
||||
/**
|
||||
* Defensive fallback for symmetric key records that somehow ended up
|
||||
* without a metadata snapshot (e.g. a save where Parcel.marshall()
|
||||
* threw and persisted an empty mdBytes, or a future format where the
|
||||
* snapshot is lazily populated). Without this fallback, loadAll would
|
||||
* skip the record and the secret material would be effectively lost,
|
||||
* silently logging the user out the next time the alias is used.
|
||||
*
|
||||
* The rebuilt KeyMetadata is structurally minimal — only the primitive
|
||||
* authorization tags we captured at save time. That's worse than a
|
||||
* byte-identical snapshot for apps that fingerprint metadata, but it
|
||||
* still keeps the AES key alive across reboots, which is the
|
||||
* dominant correctness concern.
|
||||
*/
|
||||
private fun rebuildSymmetricResponse(record: PersistedKeyData): KeyEntryResponse {
|
||||
val attestation = KeyMintAttestation(
|
||||
keySize = record.keySize,
|
||||
algorithm = record.algorithm,
|
||||
ecCurve = record.ecCurve,
|
||||
ecCurveName = "",
|
||||
origin = null,
|
||||
blockMode = emptyList(),
|
||||
padding = emptyList(),
|
||||
purpose = record.purposes,
|
||||
digest = record.digests,
|
||||
rsaPublicExponent = null,
|
||||
certificateSerial = null,
|
||||
certificateSubject = null,
|
||||
certificateNotBefore = null,
|
||||
certificateNotAfter = null,
|
||||
attestationChallenge = null,
|
||||
brand = null,
|
||||
device = null,
|
||||
product = null,
|
||||
serial = null,
|
||||
imei = null,
|
||||
meid = null,
|
||||
manufacturer = null,
|
||||
model = null,
|
||||
secondImei = null,
|
||||
activeDateTime = null,
|
||||
originationExpireDateTime = null,
|
||||
usageExpireDateTime = null,
|
||||
usageCountLimit = null,
|
||||
callerNonce = null,
|
||||
nonce = null,
|
||||
unlockedDeviceRequired = null,
|
||||
includeUniqueId = null,
|
||||
rollbackResistance = null,
|
||||
earlyBootOnly = null,
|
||||
allowWhileOnBody = null,
|
||||
trustedUserPresenceRequired = null,
|
||||
trustedConfirmationRequired = null,
|
||||
noAuthRequired = null,
|
||||
maxUsesPerBoot = null,
|
||||
maxBootLevel = null,
|
||||
minMacLength = null,
|
||||
rsaOaepMgfDigest = emptyList(),
|
||||
)
|
||||
val metadata = KeyMetadata().apply {
|
||||
keySecurityLevel = securityLevel
|
||||
key = KeyDescriptor().apply {
|
||||
domain = Domain.KEY_ID
|
||||
nspace = record.nspace
|
||||
alias = null
|
||||
blob = null
|
||||
}
|
||||
certificate = null
|
||||
certificateChain = null
|
||||
authorizations = attestation.toAuthorizations(record.uid, securityLevel)
|
||||
modificationTimeMs = System.currentTimeMillis()
|
||||
}
|
||||
return KeyEntryResponse().apply {
|
||||
this.metadata = metadata
|
||||
iSecurityLevel = original
|
||||
}
|
||||
}
|
||||
|
||||
companion object {
|
||||
private val secureRandom = SecureRandom()
|
||||
|
||||
|
||||
+15
-12
@@ -242,16 +242,18 @@ class SoftwareOperation(
|
||||
primitive =
|
||||
when (purpose) {
|
||||
KeyPurpose.SIGN -> {
|
||||
requireNotNull(keyPair) {
|
||||
"[SoftwareOp TX_ID: $txId] SIGN requested but keyPair is null"
|
||||
}
|
||||
Signer(keyPair, params)
|
||||
val kp = keyPair ?: throw ServiceSpecificException(
|
||||
KeystoreErrorCodes.invalidArgument,
|
||||
"[SoftwareOp TX_ID: $txId] SIGN requested but keyPair is null",
|
||||
)
|
||||
Signer(kp, params)
|
||||
}
|
||||
KeyPurpose.VERIFY -> {
|
||||
requireNotNull(keyPair) {
|
||||
"[SoftwareOp TX_ID: $txId] VERIFY requested but keyPair is null"
|
||||
}
|
||||
Verifier(keyPair, params)
|
||||
val kp = keyPair ?: throw ServiceSpecificException(
|
||||
KeystoreErrorCodes.invalidArgument,
|
||||
"[SoftwareOp TX_ID: $txId] VERIFY requested but keyPair is null",
|
||||
)
|
||||
Verifier(kp, params)
|
||||
}
|
||||
KeyPurpose.ENCRYPT -> {
|
||||
val key: java.security.Key = secretKey ?: keyPair?.public
|
||||
@@ -270,10 +272,11 @@ class SoftwareOperation(
|
||||
CipherPrimitive(key, params, Cipher.DECRYPT_MODE)
|
||||
}
|
||||
KeyPurpose.AGREE_KEY -> {
|
||||
requireNotNull(keyPair) {
|
||||
"[SoftwareOp TX_ID: $txId] AGREE_KEY requested but keyPair is null"
|
||||
}
|
||||
KeyAgreementPrimitive(keyPair)
|
||||
val kp = keyPair ?: throw ServiceSpecificException(
|
||||
KeystoreErrorCodes.invalidArgument,
|
||||
"[SoftwareOp TX_ID: $txId] AGREE_KEY requested but keyPair is null",
|
||||
)
|
||||
KeyAgreementPrimitive(kp)
|
||||
}
|
||||
else ->
|
||||
throw ServiceSpecificException(
|
||||
|
||||
Reference in New Issue
Block a user