From a816c1915bd10ab6907fda0f95c0d4749748d120 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Tue, 12 Jan 2021 22:42:34 +0530 Subject: [PATCH 1/7] HAL fixes shared by NXP team. --- .../4.1/JavacardKeymaster4Device.cpp | 104 +++++++++--------- .../4.1/JavacardOperationContext.cpp | 17 ++- HAL/keymaster/Android.bp | 5 + 3 files changed, 73 insertions(+), 53 deletions(-) diff --git a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp index 23583024..71350e9b 100644 --- a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp +++ b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp @@ -304,14 +304,10 @@ Return JavacardKeymaster4Device::getHardwareInfo(getHardwareInfo_cb _hidl_ hidl_string jcKeymasterAuthor; ErrorCode ret = sendData(Instruction::INS_GET_HW_INFO_CMD, input, resp); - if(ret != ErrorCode::OK) { - //Socket not connected. - _hidl_cb(SecurityLevel::STRONGBOX, JAVACARD_KEYMASTER_NAME, JAVACARD_KEYMASTER_AUTHOR); - return Void(); - } else { + if (ret == ErrorCode::OK) { //Skip last 2 bytes in cborData, it contains status. std::tie(item, ret) = cborConverter_.decodeData(std::vector(resp.begin(), resp.end()-2), - true); + false); if (item != nullptr) { std::vector temp; if(!cborConverter_.getUint64(item, 0, securityLevel) || @@ -324,29 +320,23 @@ Return JavacardKeymaster4Device::getHardwareInfo(getHardwareInfo_cb _hidl_ _hidl_cb(static_cast(securityLevel), jcKeymasterName, jcKeymasterAuthor); return Void(); } +#ifdef TRANSCEIVE_VIA_SOCKET + else { + //TODO Should we throw fatal error here.? + _hidl_cb(SecurityLevel::STRONGBOX, JAVACARD_KEYMASTER_NAME, JAVACARD_KEYMASTER_AUTHOR); + return Void(); + } +#endif } Return JavacardKeymaster4Device::getHmacSharingParameters(getHmacSharingParameters_cb _hidl_cb) { - /* TODO temporary fix: vold daemon calls performHmacKeyAgreement. At that time when vold calls this API there is no - * network connectivity and socket cannot be connected. So as a hack we are calling softkeymaster to getHmacSharing - * parameters. - */ std::vector cborData; std::vector input; std::unique_ptr item; HmacSharingParameters hmacSharingParameters; ErrorCode errorCode = ErrorCode::UNKNOWN_ERROR; errorCode = sendData(Instruction::INS_GET_HMAC_SHARING_PARAM_CMD, input, cborData); - if(errorCode != ErrorCode::OK) { - auto response = softKm_->GetHmacSharingParameters(); - ::android::hardware::keymaster::V4_0::HmacSharingParameters params; - params.seed.setToExternal(const_cast(response.params.seed.data), - response.params.seed.data_length); - static_assert(sizeof(response.params.nonce) == params.nonce.size(), "Nonce sizes don't match"); - memcpy(params.nonce.data(), response.params.nonce, params.nonce.size()); - _hidl_cb(legacy_enum_conversion(response.error), params); - return Void(); - } else { + if (ErrorCode::OK == errorCode) { //Skip last 2 bytes in cborData, it contains status. std::tie(item, errorCode) = cborConverter_.decodeData(std::vector(cborData.begin(), cborData.end()-2), true); @@ -355,16 +345,26 @@ Return JavacardKeymaster4Device::getHmacSharingParameters(getHmacSharingPa errorCode = ErrorCode::UNKNOWN_ERROR; } } - _hidl_cb(errorCode, hmacSharingParameters); - return Void(); } +#ifdef TRANSCEIVE_VIA_SOCKET + /* TODO temporary fix: vold daemon calls performHmacKeyAgreement. At that time when vold calls this API there is no + * network connectivity and socket cannot be connected. So as a hack we are calling softkeymaster to getHmacSharing + * parameters. + */ + else { + auto response = softKm_->GetHmacSharingParameters(); + hmacSharingParameters.seed.setToExternal(const_cast(response.params.seed.data), + response.params.seed.data_length); + static_assert(sizeof(response.params.nonce) == hmacSharingParameters.nonce.size(), "Nonce sizes don't match"); + memcpy(hmacSharingParameters.nonce.data(), response.params.nonce, hmacSharingParameters.nonce.size()); + errorCode = legacy_enum_conversion(response.error); + } +#endif + _hidl_cb(errorCode, hmacSharingParameters); + return Void(); } Return JavacardKeymaster4Device::computeSharedHmac(const hidl_vec& params, computeSharedHmac_cb _hidl_cb) { - /* TODO temporary fix: vold daemon calls performHmacKeyAgreement. At that time when vold calls this API there is no - * network connectivity and socket cannot be connected. So as a hack we are calling softkeymaster to - * computeSharedHmac. - */ cppbor::Array array; std::unique_ptr item; std::vector cborOutData; @@ -387,7 +387,25 @@ Return JavacardKeymaster4Device::computeSharedHmac(const hidl_vec cborData = array.encode(); errorCode = sendData(Instruction::INS_COMPUTE_SHARED_HMAC_CMD, cborData, cborOutData); - if(errorCode != ErrorCode::OK) { + if (ErrorCode::OK == errorCode) { + //Skip last 2 bytes in cborData, it contains status. + std::tie(item, errorCode) = cborConverter_.decodeData(std::vector(cborOutData.begin(), cborOutData.end()-2), + true); + if (item != nullptr) { + std::vector bstr; + if(!cborConverter_.getBinaryArray(item, 1, bstr)) { + errorCode = ErrorCode::UNKNOWN_ERROR; + } else { + sharingCheck = bstr; + } + } + } +#ifdef TRANSCEIVE_VIA_SOCKET + /* TODO temporary fix: vold daemon calls performHmacKeyAgreement. At that time when vold calls this API there is no + * network connectivity and socket cannot be connected. So as a hack we are calling softkeymaster to + * computeSharedHmac. + */ + else { ComputeSharedHmacRequest request; request.params_array.params_array = new keymaster::HmacSharingParameters[params.size()]; request.params_array.num_params = params.size(); @@ -401,29 +419,13 @@ Return JavacardKeymaster4Device::computeSharedHmac(const hidl_vecComputeSharedHmac(request); - hidl_vec sharing_check; - if (response.error == KM_ERROR_OK) sharing_check = kmBlob2hidlVec(response.sharing_check); - - _hidl_cb(legacy_enum_conversion(response.error), sharing_check); - return Void(); - - } else { - //Skip last 2 bytes in cborData, it contains status. - std::tie(item, errorCode) = cborConverter_.decodeData(std::vector(cborOutData.begin(), cborOutData.end()-2), - true); - if (item != nullptr) { - std::vector bstr; - if(!cborConverter_.getBinaryArray(item, 1, bstr)) { - errorCode = ErrorCode::UNKNOWN_ERROR; - } else { - sharingCheck = bstr; - } - } - _hidl_cb(errorCode, sharingCheck); - return Void(); + if (response.error == KM_ERROR_OK) sharingCheck = kmBlob2hidlVec(response.sharing_check); + errorCode = legacy_enum_conversion(response.error); } - -} +#endif + _hidl_cb(errorCode, sharingCheck); + return Void(); + } Return JavacardKeymaster4Device::verifyAuthorization(uint64_t , const hidl_vec& , const HardwareAuthToken& , verifyAuthorization_cb _hidl_cb) { VerificationToken verificationToken; @@ -1061,8 +1063,10 @@ Return JavacardKeymaster4Device::finish(uint64_t operationHandle, const hi sendDataCallback))) { output = tempOut; } + if (ErrorCode::OK != errorCode) { + abort(operationHandle); + } } - abort(operationHandle); _hidl_cb(errorCode, outParams, output); return Void(); } diff --git a/HAL/keymaster/4.1/JavacardOperationContext.cpp b/HAL/keymaster/4.1/JavacardOperationContext.cpp index 457a6d87..1872f75b 100644 --- a/HAL/keymaster/4.1/JavacardOperationContext.cpp +++ b/HAL/keymaster/4.1/JavacardOperationContext.cpp @@ -183,9 +183,18 @@ ErrorCode OperationContext::finish(uint64_t operHandle, const std::vector newInput(first, end); - if(ErrorCode::OK != (errorCode = handleInternalUpdate(operHandle, newInput.data(), newInput.size(), - Operation::Update, cb))) { - return errorCode; + if(extraData == 0 && (i == noOfChunks - 1)) { + //Last chunk + if(ErrorCode::OK != (errorCode = handleInternalUpdate(operHandle, newInput.data(), newInput.size(), + Operation::Finish, cb, true))) { + return errorCode; + } + + } else { + if(ErrorCode::OK != (errorCode = handleInternalUpdate(operHandle, newInput.data(), newInput.size(), + Operation::Update, cb))) { + return errorCode; + } } } if(extraData > 0) { @@ -217,6 +226,8 @@ ErrorCode OperationContext::getBlockAlignedData(uint64_t operHandle, uint8_t* in blockSize = AES_BLOCK_SIZE; } else if(Algorithm::TRIPLE_DES == operationTable[operHandle].info.alg) { blockSize = DES_BLOCK_SIZE; + } else { + return ErrorCode::INCOMPATIBLE_ALGORITHM; } if(opr == Operation::Finish) { diff --git a/HAL/keymaster/Android.bp b/HAL/keymaster/Android.bp index 8b34023d..eb4fa49c 100644 --- a/HAL/keymaster/Android.bp +++ b/HAL/keymaster/Android.bp @@ -49,6 +49,11 @@ cc_binary { "libjc_transport", "libcrypto", ], + product_variables: { + debuggable: { + cflags: ["-DTRANSCEIVE_VIA_SOCKET"] + } + } } cc_library { From 8a36d0282090084a04006ed62cfe5454d21df03a Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Wed, 13 Jan 2021 23:14:09 +0530 Subject: [PATCH 2/7] Create operation handle map in HAL to differentiate between public and strongbox operations in update, finish and abort operations. --- .../4.1/JavacardKeymaster4Device.cpp | 169 ++++++++++++++---- 1 file changed, 134 insertions(+), 35 deletions(-) diff --git a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp index 71350e9b..ab8ee0ed 100644 --- a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp +++ b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp @@ -49,6 +49,9 @@ #define INS_BEGIN_KM_CMD 0x00 #define INS_END_KM_PROVISION_CMD 0x20 #define INS_END_KM_CMD 0x7F +#define MAX_COUNTER_VALUE 25 +#define SW_KM_OPR 0UL +#define SB_KM_OPR 1UL namespace keymaster { namespace V4_1 { @@ -56,6 +59,7 @@ namespace javacard { static std::unique_ptr pTransportFactory = nullptr; constexpr size_t kOperationTableSize = 4; +std::map operationTable; struct KM_AUTH_LIST_Delete { void operator()(KM_AUTH_LIST* p) { KM_AUTH_LIST_free(p); } @@ -116,6 +120,63 @@ static inline bool getTag(const hidl_vec& params, Tag tag, KeyPara return false; } +static ErrorCode generateOperationHandle(uint64_t& oprHandle) { + std::map::iterator it; + //Check if oprHandleCnt is present in operationTable + uint64_t cnt = 1; + uint64_t mask; + for(; cnt < MAX_COUNTER_VALUE; cnt++) { + mask = cnt | (SB_KM_OPR << 56); + it = operationTable.find(mask); + if (it == operationTable.end()) { + //NO HW Operation found with this operation handle. + //Check for SW. + mask = cnt | (SW_KM_OPR << 56); + it = operationTable.find(mask); + if (it != operationTable.end()) + continue; + else + break; + } else { + continue; + } + } + if (cnt == MAX_COUNTER_VALUE) { + LOG(ERROR) << "generateOperationHandle TOO_MANY_OPERATIONS"; + return ErrorCode::TOO_MANY_OPERATIONS; + } + oprHandle = cnt; + return ErrorCode::OK; +} + +static ErrorCode createOprHandleEntry(uint64_t origOprHandle, uint64_t mask/* SW or HW */, uint64_t& generatedOprHandle) { + ErrorCode errorCode = ErrorCode::OK; + if (ErrorCode::OK != (errorCode = generateOperationHandle(generatedOprHandle))) { + return errorCode; + } + //mask the operationhandle + generatedOprHandle |= (mask << 56); + operationTable[generatedOprHandle] = origOprHandle; + return errorCode; +} + +static ErrorCode getOrigOperationHandle(uint64_t generatedOprHandle, uint64_t& origOprHandle) { + std::map::iterator it = operationTable.find(generatedOprHandle); + if (it == operationTable.end()) { + return ErrorCode::INVALID_OPERATION_HANDLE; + } + origOprHandle = it->second; + return ErrorCode::OK; +} + +static bool isStrongboxOperation(uint64_t generatedOprHandle) { + return (SB_KM_OPR == (generatedOprHandle >> 56)); +} + +static void deleteOprHandleEntry(uint64_t generatedOprHandle) { + operationTable.erase(generatedOprHandle); +} + ErrorCode encodeParametersVerified(const VerificationToken& verificationToken, std::vector& asn1ParamsVerified) { if (verificationToken.parametersVerified.size() > 0) { AuthorizationSet paramSet; @@ -793,12 +854,13 @@ Return JavacardKeymaster4Device::begin(KeyPurpose purpose, const hidl_vec< hidl_vec outParams; uint64_t operationHandle = 0; hidl_vec resultParams; + uint64_t generatedOpHandle = 0; if(keyBlob.size() == 0) { _hidl_cb(ErrorCode::INVALID_ARGUMENT, resultParams, operationHandle); return Void(); } - + /* Asymmetric public key operations are handled by softkeymaster. */ if (KeyPurpose::ENCRYPT == purpose || KeyPurpose::VERIFY == purpose) { BeginOperationRequest request; request.purpose = legacy_enum_conversion(purpose); @@ -806,13 +868,17 @@ Return JavacardKeymaster4Device::begin(KeyPurpose purpose, const hidl_vec< request.additional_params.Reinitialize(KmParamSet(inParams)); BeginOperationResponse response; + /* For Symmetric key operation, the BeginOperation returns KM_ERROR_INCOMPATIBLE_ALGORITHM error. */ softKm_->BeginOperation(request, &response); if (response.error == KM_ERROR_OK) { resultParams = kmParamSet2Hidl(response.output_params); } if (response.error != KM_ERROR_INCOMPATIBLE_ALGORITHM) { /*Incompatible algorithm could be handled by JavaCard*/ - _hidl_cb(legacy_enum_conversion(response.error), resultParams, response.op_handle); + errorCode = legacy_enum_conversion(response.error); + if (errorCode == ErrorCode::OK) + errorCode = createOprHandleEntry(response.op_handle, SW_KM_OPR, generatedOpHandle); + _hidl_cb(errorCode, resultParams, generatedOpHandle); return Void(); } } @@ -871,29 +937,40 @@ Return JavacardKeymaster4Device::begin(KeyPurpose purpose, const hidl_vec< } } } - _hidl_cb(errorCode, outParams, operationHandle); + if (ErrorCode::OK == errorCode) + errorCode = createOprHandleEntry(operationHandle, SB_KM_OPR, generatedOpHandle); + _hidl_cb(errorCode, outParams, generatedOpHandle); return Void(); } -Return JavacardKeymaster4Device::update(uint64_t operationHandle, const hidl_vec& inParams, const hidl_vec& input, const HardwareAuthToken& authToken, const VerificationToken& verificationToken, update_cb _hidl_cb) { +Return JavacardKeymaster4Device::update(uint64_t halGeneratedOprHandle, const hidl_vec& inParams, const hidl_vec& input, const HardwareAuthToken& authToken, const VerificationToken& verificationToken, update_cb _hidl_cb) { ErrorCode errorCode = ErrorCode::UNKNOWN_ERROR; - UpdateOperationRequest request; - request.op_handle = operationHandle; - request.input.Reinitialize(input.data(), input.size()); - request.additional_params.Reinitialize(KmParamSet(inParams)); - - UpdateOperationResponse response; - softKm_->UpdateOperation(request, &response); - uint32_t inputConsumed = 0; hidl_vec outParams; hidl_vec output; - errorCode = legacy_enum_conversion(response.error); - if (response.error == KM_ERROR_OK) { - inputConsumed = response.input_consumed; - outParams = kmParamSet2Hidl(response.output_params); - output = kmBuffer2hidlVec(response.output); - } else if(response.error == KM_ERROR_INVALID_OPERATION_HANDLE) { + uint64_t operationHandle; + UpdateOperationResponse response; + if (ErrorCode::OK != (errorCode = getOrigOperationHandle(halGeneratedOprHandle, operationHandle))) { + _hidl_cb(errorCode, inputConsumed, outParams, output); + return Void(); + } + + if (!isStrongboxOperation(halGeneratedOprHandle)) { + /* SW keymaster (Public key operation) */ + UpdateOperationRequest request; + request.op_handle = operationHandle; + request.input.Reinitialize(input.data(), input.size()); + request.additional_params.Reinitialize(KmParamSet(inParams)); + + softKm_->UpdateOperation(request, &response); + errorCode = legacy_enum_conversion(response.error); + if (response.error == KM_ERROR_OK) { + inputConsumed = response.input_consumed; + outParams = kmParamSet2Hidl(response.output_params); + output = kmBuffer2hidlVec(response.output); + } + } else { + /* Strongbox Keymaster operation */ std::vector tempOut; /* OperationContext calls this below sendDataCallback callback function. This callback * may be called multiple times if the input data is larger than MAX_ALLOWED_INPUT_SIZE. @@ -954,32 +1031,48 @@ Return JavacardKeymaster4Device::update(uint64_t operationHandle, const hi inputConsumed = input.size(); output = tempOut; } + if(ErrorCode::OK != errorCode) { + abort(operationHandle); + } } if(ErrorCode::OK != errorCode) { - abort(operationHandle); + deleteOprHandleEntry(halGeneratedOprHandle); } + _hidl_cb(errorCode, inputConsumed, outParams, output); return Void(); } -Return JavacardKeymaster4Device::finish(uint64_t operationHandle, const hidl_vec& inParams, const hidl_vec& input, const hidl_vec& signature, const HardwareAuthToken& authToken, const VerificationToken& verificationToken, finish_cb _hidl_cb) { +Return JavacardKeymaster4Device::finish(uint64_t halGeneratedOprHandle, const hidl_vec& inParams, const hidl_vec& input, const hidl_vec& signature, const HardwareAuthToken& authToken, const VerificationToken& verificationToken, finish_cb _hidl_cb) { ErrorCode errorCode = ErrorCode::UNKNOWN_ERROR; - FinishOperationRequest request; - request.op_handle = operationHandle; - request.input.Reinitialize(input.data(), input.size()); - request.signature.Reinitialize(signature.data(), signature.size()); - request.additional_params.Reinitialize(KmParamSet(inParams)); - - FinishOperationResponse response; - softKm_->FinishOperation(request, &response); - + uint64_t operationHandle; hidl_vec outParams; hidl_vec output; - errorCode = legacy_enum_conversion(response.error); - if (response.error == KM_ERROR_OK) { - outParams = kmParamSet2Hidl(response.output_params); - output = kmBuffer2hidlVec(response.output); - } else if (response.error == KM_ERROR_INVALID_OPERATION_HANDLE) { + FinishOperationResponse response; + + if (ErrorCode::OK != (errorCode = getOrigOperationHandle(halGeneratedOprHandle, operationHandle))) { + _hidl_cb(errorCode, outParams, output); + return Void(); + } + + if (!isStrongboxOperation(halGeneratedOprHandle)) { + /* SW keymaster (Public key operation) */ + FinishOperationRequest request; + request.op_handle = operationHandle; + request.input.Reinitialize(input.data(), input.size()); + request.signature.Reinitialize(signature.data(), signature.size()); + request.additional_params.Reinitialize(KmParamSet(inParams)); + + //FinishOperationResponse response; + softKm_->FinishOperation(request, &response); + + errorCode = legacy_enum_conversion(response.error); + if (response.error == KM_ERROR_OK) { + outParams = kmParamSet2Hidl(response.output_params); + output = kmBuffer2hidlVec(response.output); + } + } else { + /* Strongbox Keymaster operation */ std::vector tempOut; bool aadTag = false; /* OperationContext calls this below sendDataCallback callback function. This callback @@ -1067,12 +1160,17 @@ Return JavacardKeymaster4Device::finish(uint64_t operationHandle, const hi abort(operationHandle); } } + deleteOprHandleEntry(halGeneratedOprHandle); _hidl_cb(errorCode, outParams, output); return Void(); } -Return JavacardKeymaster4Device::abort(uint64_t operationHandle) { +Return JavacardKeymaster4Device::abort(uint64_t halGeneratedOprHandle) { ErrorCode errorCode = ErrorCode::UNKNOWN_ERROR; + uint64_t operationHandle; + if (ErrorCode::OK != (errorCode = getOrigOperationHandle(halGeneratedOprHandle, operationHandle))) { + return errorCode; + } AbortOperationRequest request; request.op_handle = operationHandle; @@ -1099,6 +1197,7 @@ Return JavacardKeymaster4Device::abort(uint64_t operationHandle) { } /* Delete the entry on this operationHandle */ oprCtx_->clearOperationData(operationHandle); + deleteOprHandleEntry(halGeneratedOprHandle); return errorCode; } From 39357b51c3a168e970f3fd62a82d9d157feb3ca8 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Thu, 14 Jan 2021 00:44:08 +0530 Subject: [PATCH 3/7] Pass hal generated operation handle to abort() call from update and finish. --- HAL/keymaster/4.1/JavacardKeymaster4Device.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp index ab8ee0ed..cbcbdfb7 100644 --- a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp +++ b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp @@ -142,7 +142,7 @@ static ErrorCode generateOperationHandle(uint64_t& oprHandle) { } } if (cnt == MAX_COUNTER_VALUE) { - LOG(ERROR) << "generateOperationHandle TOO_MANY_OPERATIONS"; + LOG(ERROR) << "generateOperationHandle TOO_MANY_OPERATIONS error."; return ErrorCode::TOO_MANY_OPERATIONS; } oprHandle = cnt; @@ -1032,7 +1032,7 @@ Return JavacardKeymaster4Device::update(uint64_t halGeneratedOprHandle, co output = tempOut; } if(ErrorCode::OK != errorCode) { - abort(operationHandle); + abort(halGeneratedOprHandle); } } if(ErrorCode::OK != errorCode) { @@ -1129,7 +1129,6 @@ Return JavacardKeymaster4Device::finish(uint64_t halGeneratedOprHandle, co cborConverter_.addHardwareAuthToken(array, authToken); cborConverter_.addVerificationToken(array, verificationToken, asn1ParamsVerified); std::vector cborData = array.encode(); - errorCode = sendData(ins, cborData, cborOutData); if(errorCode == ErrorCode::OK) { @@ -1157,7 +1156,7 @@ Return JavacardKeymaster4Device::finish(uint64_t halGeneratedOprHandle, co output = tempOut; } if (ErrorCode::OK != errorCode) { - abort(operationHandle); + abort(halGeneratedOprHandle); } } deleteOprHandleEntry(halGeneratedOprHandle); From 5c630f2a70d2349754a63bc2f66d30c7213f9f76 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Thu, 14 Jan 2021 12:11:49 +0530 Subject: [PATCH 4/7] Added comments for the newly added code. --- .../4.1/JavacardKeymaster4Device.cpp | 43 +++++++++++++------ 1 file changed, 29 insertions(+), 14 deletions(-) diff --git a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp index cbcbdfb7..f36fcc67 100644 --- a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp +++ b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp @@ -120,17 +120,18 @@ static inline bool getTag(const hidl_vec& params, Tag tag, KeyPara return false; } +/* Generate new operation handle */ static ErrorCode generateOperationHandle(uint64_t& oprHandle) { std::map::iterator it; - //Check if oprHandleCnt is present in operationTable + //Check if operationHandle is present in operationTable uint64_t cnt = 1; uint64_t mask; for(; cnt < MAX_COUNTER_VALUE; cnt++) { mask = cnt | (SB_KM_OPR << 56); it = operationTable.find(mask); if (it == operationTable.end()) { - //NO HW Operation found with this operation handle. - //Check for SW. + //No Strongbox Operation found with this operation handle. + //Check for Software operation. mask = cnt | (SW_KM_OPR << 56); it = operationTable.find(mask); if (it != operationTable.end()) @@ -142,26 +143,28 @@ static ErrorCode generateOperationHandle(uint64_t& oprHandle) { } } if (cnt == MAX_COUNTER_VALUE) { - LOG(ERROR) << "generateOperationHandle TOO_MANY_OPERATIONS error."; + LOG(ERROR) << "operation handle count reached max operations"; return ErrorCode::TOO_MANY_OPERATIONS; } oprHandle = cnt; return ErrorCode::OK; } -static ErrorCode createOprHandleEntry(uint64_t origOprHandle, uint64_t mask/* SW or HW */, uint64_t& generatedOprHandle) { +/* Create a new operation handle entry in operation table.*/ +static ErrorCode createOprHandleEntry(uint64_t origOprHandle, uint64_t mask/* SW or HW */, uint64_t& newOperationHandle) { ErrorCode errorCode = ErrorCode::OK; - if (ErrorCode::OK != (errorCode = generateOperationHandle(generatedOprHandle))) { + if (ErrorCode::OK != (errorCode = generateOperationHandle(newOperationHandle))) { return errorCode; } //mask the operationhandle - generatedOprHandle |= (mask << 56); - operationTable[generatedOprHandle] = origOprHandle; + newOperationHandle |= (mask << 56); + operationTable[newOperationHandle] = origOprHandle; return errorCode; } -static ErrorCode getOrigOperationHandle(uint64_t generatedOprHandle, uint64_t& origOprHandle) { - std::map::iterator it = operationTable.find(generatedOprHandle); +/* Get original operation handle generated by softkeymaster/strongboxkeymaster. */ +static ErrorCode getOrigOperationHandle(uint64_t halGeneratedOperationHandle, uint64_t& origOprHandle) { + std::map::iterator it = operationTable.find(halGeneratedOperationHandle); if (it == operationTable.end()) { return ErrorCode::INVALID_OPERATION_HANDLE; } @@ -169,12 +172,14 @@ static ErrorCode getOrigOperationHandle(uint64_t generatedOprHandle, uint64_t& o return ErrorCode::OK; } -static bool isStrongboxOperation(uint64_t generatedOprHandle) { - return (SB_KM_OPR == (generatedOprHandle >> 56)); +/* Tells if the operation handle belongs to strongbox keymaster. */ +static bool isStrongboxOperation(uint64_t halGeneratedOperationHandle) { + return (SB_KM_OPR == (halGeneratedOperationHandle >> 56)); } -static void deleteOprHandleEntry(uint64_t generatedOprHandle) { - operationTable.erase(generatedOprHandle); +/* Delete the operation handle entry from operation table. */ +static void deleteOprHandleEntry(uint64_t halGeneratedOperationHandle) { + operationTable.erase(halGeneratedOperationHandle); } ErrorCode encodeParametersVerified(const VerificationToken& verificationToken, std::vector& asn1ParamsVerified) { @@ -876,6 +881,10 @@ Return JavacardKeymaster4Device::begin(KeyPurpose purpose, const hidl_vec< } if (response.error != KM_ERROR_INCOMPATIBLE_ALGORITHM) { /*Incompatible algorithm could be handled by JavaCard*/ errorCode = legacy_enum_conversion(response.error); + /* Create a new operation handle and add a entry inside the operation table map with + * key - new operation handle + * value - hal generated operation handle. + */ if (errorCode == ErrorCode::OK) errorCode = createOprHandleEntry(response.op_handle, SW_KM_OPR, generatedOpHandle); _hidl_cb(errorCode, resultParams, generatedOpHandle); @@ -937,6 +946,10 @@ Return JavacardKeymaster4Device::begin(KeyPurpose purpose, const hidl_vec< } } } + /* Create a new operation handle and add a entry inside the operation table map with + * key - new operation handle + * value - hal generated operation handle. + */ if (ErrorCode::OK == errorCode) errorCode = createOprHandleEntry(operationHandle, SB_KM_OPR, generatedOpHandle); _hidl_cb(errorCode, outParams, generatedOpHandle); @@ -1036,6 +1049,7 @@ Return JavacardKeymaster4Device::update(uint64_t halGeneratedOprHandle, co } } if(ErrorCode::OK != errorCode) { + /* Delete the entry from operation table. */ deleteOprHandleEntry(halGeneratedOprHandle); } @@ -1159,6 +1173,7 @@ Return JavacardKeymaster4Device::finish(uint64_t halGeneratedOprHandle, co abort(halGeneratedOprHandle); } } + /* Delete the entry from operation table. */ deleteOprHandleEntry(halGeneratedOprHandle); _hidl_cb(errorCode, outParams, output); return Void(); From ed9c07af66ba07c09c3612f966d06fbb4c90b571 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Fri, 15 Jan 2021 20:44:40 +0530 Subject: [PATCH 5/7] Used Rand_bytes for hal generated operation handle. Updated comments --- .../4.1/JavacardKeymaster4Device.cpp | 70 ++++++++----------- .../4.1/JavacardOperationContext.cpp | 3 +- HAL/keymaster/Android.bp | 4 +- 3 files changed, 34 insertions(+), 43 deletions(-) diff --git a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp index f36fcc67..5ac1a431 100644 --- a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp +++ b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include @@ -49,7 +50,6 @@ #define INS_BEGIN_KM_CMD 0x00 #define INS_END_KM_PROVISION_CMD 0x20 #define INS_END_KM_CMD 0x7F -#define MAX_COUNTER_VALUE 25 #define SW_KM_OPR 0UL #define SB_KM_OPR 1UL @@ -59,7 +59,10 @@ namespace javacard { static std::unique_ptr pTransportFactory = nullptr; constexpr size_t kOperationTableSize = 4; -std::map operationTable; +/* Key is the newly generated operation handle. Value is a pair with first element having + * original operation handle and second element represents SW or SB operation. + */ +std::map> operationTable; struct KM_AUTH_LIST_Delete { void operator()(KM_AUTH_LIST* p) { KM_AUTH_LIST_free(p); } @@ -122,59 +125,44 @@ static inline bool getTag(const hidl_vec& params, Tag tag, KeyPara /* Generate new operation handle */ static ErrorCode generateOperationHandle(uint64_t& oprHandle) { - std::map::iterator it; - //Check if operationHandle is present in operationTable - uint64_t cnt = 1; - uint64_t mask; - for(; cnt < MAX_COUNTER_VALUE; cnt++) { - mask = cnt | (SB_KM_OPR << 56); - it = operationTable.find(mask); - if (it == operationTable.end()) { - //No Strongbox Operation found with this operation handle. - //Check for Software operation. - mask = cnt | (SW_KM_OPR << 56); - it = operationTable.find(mask); - if (it != operationTable.end()) - continue; - else - break; - } else { - continue; + std::map>::iterator it; + do { + keymaster_error_t err = GenerateRandom(reinterpret_cast(&oprHandle), (size_t)sizeof(oprHandle)); + if (err != KM_ERROR_OK) { + return legacy_enum_conversion(err); } - } - if (cnt == MAX_COUNTER_VALUE) { - LOG(ERROR) << "operation handle count reached max operations"; - return ErrorCode::TOO_MANY_OPERATIONS; - } - oprHandle = cnt; + it = operationTable.find(oprHandle); + } while (it != operationTable.end()); return ErrorCode::OK; } /* Create a new operation handle entry in operation table.*/ -static ErrorCode createOprHandleEntry(uint64_t origOprHandle, uint64_t mask/* SW or HW */, uint64_t& newOperationHandle) { +static ErrorCode createOprHandleEntry(uint64_t origOprHandle, uint64_t keymasterSrc, uint64_t& newOperationHandle) { ErrorCode errorCode = ErrorCode::OK; if (ErrorCode::OK != (errorCode = generateOperationHandle(newOperationHandle))) { return errorCode; } - //mask the operationhandle - newOperationHandle |= (mask << 56); - operationTable[newOperationHandle] = origOprHandle; + operationTable[newOperationHandle] = std::make_pair(origOprHandle, keymasterSrc); return errorCode; } /* Get original operation handle generated by softkeymaster/strongboxkeymaster. */ static ErrorCode getOrigOperationHandle(uint64_t halGeneratedOperationHandle, uint64_t& origOprHandle) { - std::map::iterator it = operationTable.find(halGeneratedOperationHandle); + std::map>::iterator it = operationTable.find(halGeneratedOperationHandle); if (it == operationTable.end()) { return ErrorCode::INVALID_OPERATION_HANDLE; } - origOprHandle = it->second; + origOprHandle = it->second.first; return ErrorCode::OK; } /* Tells if the operation handle belongs to strongbox keymaster. */ static bool isStrongboxOperation(uint64_t halGeneratedOperationHandle) { - return (SB_KM_OPR == (halGeneratedOperationHandle >> 56)); + std::map>::iterator it = operationTable.find(halGeneratedOperationHandle); + if (it == operationTable.end()) { + return false; + } + return (SB_KM_OPR == it->second.second); } /* Delete the operation handle entry from operation table. */ @@ -385,14 +373,12 @@ Return JavacardKeymaster4Device::getHardwareInfo(getHardwareInfo_cb _hidl_ } _hidl_cb(static_cast(securityLevel), jcKeymasterName, jcKeymasterAuthor); return Void(); - } -#ifdef TRANSCEIVE_VIA_SOCKET - else { - //TODO Should we throw fatal error here.? + } else { + // It should not come here, but incase if for any reason SB keymaster fails to getHardwareInfo + // return proper values from HAL. _hidl_cb(SecurityLevel::STRONGBOX, JAVACARD_KEYMASTER_NAME, JAVACARD_KEYMASTER_AUTHOR); return Void(); } -#endif } Return JavacardKeymaster4Device::getHmacSharingParameters(getHmacSharingParameters_cb _hidl_cb) { @@ -412,7 +398,7 @@ Return JavacardKeymaster4Device::getHmacSharingParameters(getHmacSharingPa } } } -#ifdef TRANSCEIVE_VIA_SOCKET +#ifdef VTS_EMULATOR /* TODO temporary fix: vold daemon calls performHmacKeyAgreement. At that time when vold calls this API there is no * network connectivity and socket cannot be connected. So as a hack we are calling softkeymaster to getHmacSharing * parameters. @@ -466,7 +452,7 @@ Return JavacardKeymaster4Device::computeSharedHmac(const hidl_vec JavacardKeymaster4Device::begin(KeyPurpose purpose, const hidl_vec< _hidl_cb(ErrorCode::INVALID_ARGUMENT, resultParams, operationHandle); return Void(); } - /* Asymmetric public key operations are handled by softkeymaster. */ + /* Asymmetric public key operations like RSA Verify, RSA Encrypt, ECDSA verify + * are handled by softkeymaster. + */ if (KeyPurpose::ENCRYPT == purpose || KeyPurpose::VERIFY == purpose) { BeginOperationRequest request; request.purpose = legacy_enum_conversion(purpose); diff --git a/HAL/keymaster/4.1/JavacardOperationContext.cpp b/HAL/keymaster/4.1/JavacardOperationContext.cpp index 1872f75b..e3f4c800 100644 --- a/HAL/keymaster/4.1/JavacardOperationContext.cpp +++ b/HAL/keymaster/4.1/JavacardOperationContext.cpp @@ -123,6 +123,7 @@ ErrorCode OperationContext::validateInputData(uint64_t operHandle, Operation opr memset(oprData.data.buf, 0x00, sizeof(oprData.data.buf)); oprData.data.buf_len = 0; } + return ErrorCode::OK; } } input = actualInput; @@ -342,7 +343,7 @@ ErrorCode OperationContext::handleInternalUpdate(uint64_t operHandle, uint8_t* d return errorCode; } } else { - //For strongbox keymaster, in NoDigest case the length of the input message for RSA should be more than + //For strongbox keymaster, in NoDigest case the length of the input message for RSA should not be more than //256 and for EC it should not be more than 32. This validation is already happening in //validateInputData function. Just for safety sake we are checking the length to MAX_BUF_SIZE. if(operationTable[operHandle].data.buf_len <= MAX_BUF_SIZE) { diff --git a/HAL/keymaster/Android.bp b/HAL/keymaster/Android.bp index eb4fa49c..d92809e8 100644 --- a/HAL/keymaster/Android.bp +++ b/HAL/keymaster/Android.bp @@ -51,7 +51,9 @@ cc_binary { ], product_variables: { debuggable: { - cflags: ["-DTRANSCEIVE_VIA_SOCKET"] + cflags: [ + "-DVTS_EMULATOR", + ] } } } From 20ca29b22f785b7d3e0488645f3b0e6e5690f4f9 Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Fri, 15 Jan 2021 21:53:46 +0530 Subject: [PATCH 6/7] Clearing the operation data inside the operation context --- HAL/keymaster/4.1/JavacardKeymaster4Device.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp index 5ac1a431..445f3737 100644 --- a/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp +++ b/HAL/keymaster/4.1/JavacardKeymaster4Device.cpp @@ -1163,6 +1163,7 @@ Return JavacardKeymaster4Device::finish(uint64_t halGeneratedOprHandle, co } /* Delete the entry from operation table. */ deleteOprHandleEntry(halGeneratedOprHandle); + oprCtx_->clearOperationData(operationHandle); _hidl_cb(errorCode, outParams, output); return Void(); } From 8e97a450ef51e0311d439d896d48126e0e895e3d Mon Sep 17 00:00:00 2001 From: BKSSM Venkateswarlu Date: Sun, 17 Jan 2021 19:50:33 +0530 Subject: [PATCH 7/7] Added comments for getBlockAlignedData function --- .../4.1/JavacardOperationContext.cpp | 40 +++++++++++-------- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/HAL/keymaster/4.1/JavacardOperationContext.cpp b/HAL/keymaster/4.1/JavacardOperationContext.cpp index e3f4c800..7319cb3c 100644 --- a/HAL/keymaster/4.1/JavacardOperationContext.cpp +++ b/HAL/keymaster/4.1/JavacardOperationContext.cpp @@ -23,7 +23,6 @@ #define DES_BLOCK_SIZE 8 #define RSA_INPUT_MSG_LEN 256 #define EC_INPUT_MSG_LEN 32 -#define MAX_RSA_BUFFER_SIZE 256 #define MAX_EC_BUFFER_SIZE 32 namespace keymaster { @@ -104,11 +103,10 @@ ErrorCode OperationContext::validateInputData(uint64_t operHandle, Operation opr } if(KeyPurpose::DECRYPT == oprData.info.purpose && Algorithm::RSA == oprData.info.alg) { - if((oprData.data.buf_len+actualInput.size()) > MAX_RSA_BUFFER_SIZE) { + if((oprData.data.buf_len+actualInput.size()) > RSA_INPUT_MSG_LEN) { return ErrorCode::INVALID_INPUT_LENGTH; } } - if(opr == Operation::Finish) { //If it is observed in finish operation that buffered data + input data exceeds the MAX_ALLOWED_INPUT_SIZE then //combine both the data in a single buffer. This helps in making sure that no data is left out in the buffer after @@ -171,7 +169,6 @@ ErrorCode OperationContext::update(uint64_t operHandle, const std::vector& actualInput, sendDataToSE_cb cb) { ErrorCode errorCode = ErrorCode::OK; std::vector input; - /* Validate the input data */ if(ErrorCode::OK != (errorCode = validateInputData(operHandle, Operation::Finish, actualInput, input))) { return errorCode; @@ -214,15 +211,25 @@ ErrorCode OperationContext::finish(uint64_t operHandle, const std::vector& out) { - size_t dataToSELen = 0; + size_t dataToSELen = 0;/*Length of the data to be send to the Applet.*/ size_t inputConsumed = 0;/*Length of the data consumed from input */ size_t blockSize = 0; BufferedData& data = operationTable[operHandle].data; int bufIndex = data.buf_len; - if(Algorithm::AES == operationTable[operHandle].info.alg) { blockSize = AES_BLOCK_SIZE; } else if(Algorithm::TRIPLE_DES == operationTable[operHandle].info.alg) { @@ -265,6 +272,8 @@ ErrorCode OperationContext::getBlockAlignedData(uint64_t operHandle, uint8_t* in if(dataToSELen > 0) { //If buffer length is greater than the data length to be send to SE, then input data consumed is 0. //That means all the data to be send to SE is consumed from the buffer. + //The buffer length might become greater than dataToSELen in the cases where we are saving the last block of + //data i.e. AES/TDES Decryption with PKC7Padding or AES GCM Decryption operations. inputConsumed = (data.buf_len > dataToSELen) ? 0 : (dataToSELen - data.buf_len); //Copy the buffer to be send to SE. @@ -301,7 +310,6 @@ ErrorCode OperationContext::handleInternalUpdate(uint64_t operHandle, uint8_t* d sendDataToSE_cb cb, bool finish) { ErrorCode errorCode = ErrorCode::OK; std::vector out; - if(Algorithm::AES == operationTable[operHandle].info.alg || Algorithm::TRIPLE_DES == operationTable[operHandle].info.alg) { /*Symmetric */ @@ -345,16 +353,14 @@ ErrorCode OperationContext::handleInternalUpdate(uint64_t operHandle, uint8_t* d } else { //For strongbox keymaster, in NoDigest case the length of the input message for RSA should not be more than //256 and for EC it should not be more than 32. This validation is already happening in - //validateInputData function. Just for safety sake we are checking the length to MAX_BUF_SIZE. - if(operationTable[operHandle].data.buf_len <= MAX_BUF_SIZE) { - size_t bufIndex = operationTable[operHandle].data.buf_len; - size_t pos = 0; - for(; (pos < len) && (pos < (MAX_BUF_SIZE-bufIndex)); pos++) - { - operationTable[operHandle].data.buf[bufIndex+pos] = data[pos]; - } - operationTable[operHandle].data.buf_len += pos; + //validateInputData function. + size_t bufIndex = operationTable[operHandle].data.buf_len; + size_t pos = 0; + for(; pos < len; ++pos) + { + operationTable[operHandle].data.buf[bufIndex+pos] = data[pos]; } + operationTable[operHandle].data.buf_len += pos; } } else { /* With Digest */ for(size_t j=0; j < len; ++j)