From 2e36786bc3fd2ec7fb7a489d44daefb0d1d74ed7 Mon Sep 17 00:00:00 2001 From: Subrahmanyaman Date: Wed, 13 Apr 2022 03:00:26 +0000 Subject: [PATCH] 1. In processAttestKey , call isKeyUpgradeRequired() for generated key also and return KEY_REQUIRES_UPGRADE error if upgrade condition passes. 2. using reclaimable buffer for encoding the key_paramters while creating mac for keyparamters. if the encoded key_paramters length exceeds 3K return INSUFFICIENT_BUFFER_SPACE error. 3. HAL does not handle KEY_REQUIRES_UPGRADE error. --- .../android/javacard/keymaster/KMError.java | 1 + .../javacard/keymaster/KMKeymasterApplet.java | 27 ++++++++++++++----- HAL/JavacardKeyMintDevice.cpp | 21 --------------- 3 files changed, 21 insertions(+), 28 deletions(-) diff --git a/Applet/src/com/android/javacard/keymaster/KMError.java b/Applet/src/com/android/javacard/keymaster/KMError.java index 52398824..5e93b49b 100644 --- a/Applet/src/com/android/javacard/keymaster/KMError.java +++ b/Applet/src/com/android/javacard/keymaster/KMError.java @@ -45,6 +45,7 @@ public class KMError { public static final short KEY_USER_NOT_AUTHENTICATED = 26; public static final short INVALID_OPERATION_HANDLE = 28; + public static final short INSUFFICIENT_BUFFER_SPACE = 29; public static final short VERIFICATION_FAILED = 30; public static final short TOO_MANY_OPERATIONS = 31; public static final short INVALID_KEY_BLOB = 33; diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index 66423fb9..6a250d3c 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -108,8 +108,9 @@ public class KMKeymasterApplet extends Applet implements AppletEvent, ExtendedLe }; public static final short MAX_COSE_BUF_SIZE = (short) 1024; - // Maximum possible encoded keyparams size - public static final short MAX_KEY_PARAMS_BUF_SIZE = (short) 2048; + // Maximum allowed buffer size for to encode the key parameters + // which is used while creating mac for key paramters. + public static final short MAX_KEY_PARAMS_BUF_SIZE = (short) 3072; // 3K // Top 32 commands are reserved for provisioning. private static final byte KEYMINT_CMD_APDU_START = 0x20; @@ -3491,6 +3492,10 @@ private void processAttestKeyCmd(APDU apdu) { // Check if key requires upgrade. The KeyBlob is parsed inside isKeyUpgradeRequired // function itself. if (isKeyUpgradeRequired(data[KEY_BLOB], data[APP_ID], data[APP_DATA], scratchPad)) { + // This condition occurs if either any of the system properties (OsVersion, OsPatchLevel, + // VendorPatchLevel or BootPatchLevel) changes or KeyBlob format changed. So return + // KEY_REQUIRES_UPGRADE error as this scenario is application to ATTEST_KEY_BLOB as well + // as ATTEST_KEY_BLOB got generated before the KEY_BLOB. KMException.throwIt(KMError.KEY_REQUIRES_UPGRADE); } // Validate KeyParams Mac @@ -3786,18 +3791,26 @@ private short macKeyParams(short keyParams, byte[] scratchPad) { if (SYM_KEY_TYPE == getKeyType(keyParams)) { return KMByteBlob.instance((short) 0); } - short len = encodeToApduBuffer(keyParams, scratchPad, (short) 0, MAX_KEY_PARAMS_BUF_SIZE); + short offset = repository.allocReclaimableMemory(MAX_KEY_PARAMS_BUF_SIZE); + short len = encoder.encode(keyParams, repository.getHeap(), offset); + if (len > MAX_KEY_PARAMS_BUF_SIZE) { + // KeyParamters exceeded the maximum allowed size. + KMException.throwIt(KMError.INSUFFICIENT_BUFFER_SPACE); + } short signLen = seProvider.hmacSign( KMByteBlob.cast(data[AUTH_TAG]).getBuffer(), KMByteBlob.cast(data[AUTH_TAG]).getStartOff(), KMByteBlob.cast(data[AUTH_TAG]).length(), - scratchPad, - (short) 0, + repository.getHeap(), + offset, len, scratchPad, - len); - return KMByteBlob.instance(scratchPad, len, signLen); + (short) 0); + //release memory + repository.reclaimMemory(MAX_KEY_PARAMS_BUF_SIZE); + + return KMByteBlob.instance(scratchPad, (short) 0, signLen); } private boolean validateKeyParamsMac(short keyParams, short keyParamsMac, byte[] scratchPad) { diff --git a/HAL/JavacardKeyMintDevice.cpp b/HAL/JavacardKeyMintDevice.cpp index ae78079b..03a1b004 100644 --- a/HAL/JavacardKeyMintDevice.cpp +++ b/HAL/JavacardKeyMintDevice.cpp @@ -102,13 +102,6 @@ ScopedAStatus JavacardKeyMintDevice::generateKey(const vector& key auto [certItem, error] = card_->sendRequest(Instruction::INS_ATTEST_KEY_CMD, attestKeyArray); if (error != KM_ERROR_OK) { LOG(ERROR) << "Failed in attestKey err: " << error; - if (error == KM_ERROR_KEY_REQUIRES_UPGRADE) { - // This is rare scenario where either the applet gets upgraded or system propeties - // like osVersion, OsPatch, VendorPatch and bootPatches values changes. - LOG(DEBUG) << "This error occurs in case if either the applets get upgraded or if any" - "system properties like OsVersion, OsPatch, VendorPatch or bootPatch values changes"; - error = KM_ERROR_UNKNOWN_ERROR; - } return km_utils::kmError2ScopedAStatus(error); } if (!cbor_.getCertificateChain(certItem, 1, creationResult->certificateChain)) { @@ -171,13 +164,6 @@ ScopedAStatus JavacardKeyMintDevice::importKey(const vector& keyPa auto [certItem, error] = card_->sendRequest(Instruction::INS_ATTEST_KEY_CMD, attestKeyArray); if (error != KM_ERROR_OK) { LOG(ERROR) << "Failed in attestKey err: " << error; - if (error == KM_ERROR_KEY_REQUIRES_UPGRADE) { - // This is rare scenario where either the applet gets upgraded or system propeties - // like osVersion, OsPatch, VendorPatch and bootPatches values changes. - LOG(DEBUG) << "This error occurs in case if either the applets get upgraded or if any" - "system properties like OsVersion, OsPatch, VendorPatch or bootPatch values changes"; - error = KM_ERROR_UNKNOWN_ERROR; - } return km_utils::kmError2ScopedAStatus(error); } if (!cbor_.getCertificateChain(certItem, 1, creationResult->certificateChain)) { @@ -250,13 +236,6 @@ ScopedAStatus JavacardKeyMintDevice::importWrappedKey(const vector& wra auto [certItem, error] = card_->sendRequest(Instruction::INS_ATTEST_KEY_CMD, attestKeyArray); if (error != KM_ERROR_OK) { LOG(ERROR) << "Failed in attestKey err: " << error; - if (error == KM_ERROR_KEY_REQUIRES_UPGRADE) { - // This is rare scenario where either the applet gets upgraded or system propeties - // like osVersion, OsPatch, VendorPatch and bootPatches values changes. - LOG(DEBUG) << "This error occurs in case if either the applets get upgraded or if any" - "system properties like OsVersion, OsPatch, VendorPatch or bootPatch values changes"; - error = KM_ERROR_UNKNOWN_ERROR; - } return km_utils::kmError2ScopedAStatus(error); } if (!cbor_.getCertificateChain(certItem, 1, creationResult->certificateChain)) {