From 3b55e2da6e046f429ecd367562c8e1aee8703733 Mon Sep 17 00:00:00 2001 From: Subrahmanyaman Date: Tue, 12 Apr 2022 19:17:46 +0000 Subject: [PATCH 1/3] Fixed compilation erros --- Applet/src/com/android/javacard/keymaster/KMByteTag.java | 2 +- .../src/com/android/javacard/keymaster/KMKeymasterApplet.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Applet/src/com/android/javacard/keymaster/KMByteTag.java b/Applet/src/com/android/javacard/keymaster/KMByteTag.java index e33cc4db..b7b8decd 100644 --- a/Applet/src/com/android/javacard/keymaster/KMByteTag.java +++ b/Applet/src/com/android/javacard/keymaster/KMByteTag.java @@ -130,6 +130,6 @@ private static boolean validateKey(short key, short byteBlob) { default: return false; } - return result; + return true; } } diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index 0eb32fe0..5171abde 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -1376,7 +1376,7 @@ private KMAttestationCert makeSelfSignedCert(short attPrivKey, short attPubKey, subject = KMByteTag.cast(subject).getValue(); } - if (KMEnumTag.cast(alg).getValue() == KMType.RSA) { + if (alg == KMType.RSA) { cert.rsaAttestKey(attPrivKey, attPubKey, (byte) mode); } else { cert.ecAttestKey(attPrivKey, (byte) mode); From 7a3760c4f463b0e49a4bb2d9d09638f7c23e8ee3 Mon Sep 17 00:00:00 2001 From: Subrahmanyaman Date: Tue, 12 Apr 2022 22:14:26 +0000 Subject: [PATCH 2/3] Addressed review comments --- .../android/javacard/keymaster/KMEncoder.java | 4 +-- .../javacard/keymaster/KMKeymasterApplet.java | 6 ++-- HAL/JavacardKeyMintDevice.cpp | 35 +++++++++++++++---- 3 files changed, 33 insertions(+), 12 deletions(-) diff --git a/Applet/src/com/android/javacard/keymaster/KMEncoder.java b/Applet/src/com/android/javacard/keymaster/KMEncoder.java index 0ba1986c..e47d799d 100644 --- a/Applet/src/com/android/javacard/keymaster/KMEncoder.java +++ b/Applet/src/com/android/javacard/keymaster/KMEncoder.java @@ -82,8 +82,8 @@ public short encode(short object, byte[] buffer, short startOff) { bufferRef[0] = buffer; scratchBuf[START_OFFSET] = startOff; short len = (short) (buffer.length - startOff); - if ((len < 0) || len > KMKeymasterApplet.MAX_LENGTH) { - scratchBuf[LEN_OFFSET] = KMKeymasterApplet.MAX_LENGTH; + if ((len < 0) || len > KMRepository.HEAP_SIZE) { + scratchBuf[LEN_OFFSET] = KMRepository.HEAP_SIZE; } else { scratchBuf[LEN_OFFSET] = (short) buffer.length; } diff --git a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java index 5171abde..66423fb9 100644 --- a/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java +++ b/Applet/src/com/android/javacard/keymaster/KMKeymasterApplet.java @@ -44,7 +44,6 @@ public class KMKeymasterApplet extends Applet implements AppletEvent, ExtendedLe public static final byte[] F4 = {0x01, 0x00, 0x01}; public static final byte AES_BLOCK_SIZE = 16; public static final byte DES_BLOCK_SIZE = 8; - public static final short MAX_LENGTH = 10000; public static final short MASTER_KEY_SIZE = 128; public static final short WRAPPING_KEY_SIZE = 32; public static final short MAX_OPERATIONS_COUNT = 4; @@ -3491,8 +3490,9 @@ private void processAttestKeyCmd(APDU apdu) { data[APP_DATA] = getApplicationData(data[KEY_PARAMETERS]); // Check if key requires upgrade. The KeyBlob is parsed inside isKeyUpgradeRequired // function itself. - parseEncryptedKeyBlob(data[KEY_BLOB], data[APP_ID], data[APP_DATA], scratchPad, - KEYBLOB_CURRENT_VERSION); + if (isKeyUpgradeRequired(data[KEY_BLOB], data[APP_ID], data[APP_DATA], scratchPad)) { + KMException.throwIt(KMError.KEY_REQUIRES_UPGRADE); + } // Validate KeyParams Mac if (!validateKeyParamsMac(data[KEY_PARAMETERS], keyParamsMac, scratchPad)) { KMException.throwIt(KMError.INVALID_KEY_BLOB); diff --git a/HAL/JavacardKeyMintDevice.cpp b/HAL/JavacardKeyMintDevice.cpp index 0cb1bec6..ae78079b 100644 --- a/HAL/JavacardKeyMintDevice.cpp +++ b/HAL/JavacardKeyMintDevice.cpp @@ -93,7 +93,7 @@ ScopedAStatus JavacardKeyMintDevice::generateKey(const vector& key // Call attestKey only Asymmetric algorithms. keymaster_algorithm_t algorithm; paramSet.GetTagValue(TAG_ALGORITHM, &algorithm); - if (algorithm == KM_ALGORITHM_RSA || algorithm == KM_ALGORITHM_EC) { + if (algorithm == KM_ALGORITHM_RSA || algorithm == KM_ALGORITHM_EC) { cppbor::Array attestKeyArray; attestKeyArray.add(creationResult->keyBlob); cbor_.addKeyparameters(attestKeyArray, keyParams); @@ -101,12 +101,19 @@ ScopedAStatus JavacardKeyMintDevice::generateKey(const vector& key attestKeyArray.add(keyParamsMac); auto [certItem, error] = card_->sendRequest(Instruction::INS_ATTEST_KEY_CMD, attestKeyArray); if (error != KM_ERROR_OK) { - LOG(ERROR) << "Failed in attestKey err: "; - return km_utils::kmError2ScopedAStatus(error); + 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)) { - LOG(ERROR) << "Error in decoding og response in generateKey."; - return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR); + LOG(ERROR) << "Error in decoding og response in generateKey."; + return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR); } } return ScopedAStatus::ok(); @@ -163,7 +170,14 @@ ScopedAStatus JavacardKeyMintDevice::importKey(const vector& keyPa attestKeyArray.add(keyParamsMac); auto [certItem, error] = card_->sendRequest(Instruction::INS_ATTEST_KEY_CMD, attestKeyArray); if (error != KM_ERROR_OK) { - LOG(ERROR) << "Failed in attestKey err: "; + 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)) { @@ -235,7 +249,14 @@ ScopedAStatus JavacardKeyMintDevice::importWrappedKey(const vector& wra cbor_.addAttestationKey(attestKeyArray, std::nullopt); auto [certItem, error] = card_->sendRequest(Instruction::INS_ATTEST_KEY_CMD, attestKeyArray); if (error != KM_ERROR_OK) { - LOG(ERROR) << "Failed in attestKey err: "; + 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)) { From 2e36786bc3fd2ec7fb7a489d44daefb0d1d74ed7 Mon Sep 17 00:00:00 2001 From: Subrahmanyaman Date: Wed, 13 Apr 2022 03:00:26 +0000 Subject: [PATCH 3/3] 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)) {