From 9aa150cee7121fa2f10323aa202553edc8c8e5b9 Mon Sep 17 00:00:00 2001 From: Andrew Scull Date: Tue, 1 Nov 2022 20:00:57 +0000 Subject: [PATCH 1/2] Adjust CSRv3 CDDL after implementation experience Rename from AuthenticatedMessage to AuthenticatedRequest in order to make the direction of the message clear. Move the challenge out of the endpoint-specific message and up into the common authentication wrapper as it is uesd in the authentication protocol. Simplify the versioning by having the CSR version continue sequentially, making the current version 3. Have the AuthenticatedMessage version start from 1 as it's value isn't used to distinguish v2 and v3 CSRs anyway and it will avoid confusion with the CSR version which has already moved beyond this value. Bug: 250910137 Test: n/a -- comments only Change-Id: I13836e90fa76b1b22cb6627f3d987828ffeb0adc --- .../keymint/IRemotelyProvisionedComponent.aidl | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl b/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl index 86c17171f8..78969d1d32 100644 --- a/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl +++ b/security/rkp/aidl/android/hardware/security/keymint/IRemotelyProvisionedComponent.aidl @@ -315,13 +315,12 @@ interface IRemotelyProvisionedComponent { * * @return the following CBOR Certificate Signing Request (Csr) serialized into a byte array: * - * Csr = AuthenticatedMessage + * Csr = AuthenticatedRequest * * CsrPayload = [ ; CBOR Array defining the payload for Csr - * version: 1, ; The CsrPayload CDDL Schema version. + * version: 3, ; The CsrPayload CDDL Schema version. * CertificateType, ; The type of certificate being requested. * DeviceInfo, ; Defined in DeviceInfo.aidl - * challenge: bstr .size (32..64), ; Provided by the method parameters * KeysToSign, ; Provided by the method parameters * ] * @@ -335,11 +334,14 @@ interface IRemotelyProvisionedComponent { * * KeysToSign = [ * PublicKey ] ; Please see MacedPublicKey.aidl for the PublicKey definition. * - * AuthenticatedMessage = [ - * version: 3, ; The AuthenticatedMessage CDDL Schema version. - * UdsCerts, - * DiceCertChain, - * SignedData, + * AuthenticatedRequest = [ + * version: 1, ; The AuthenticatedRequest CDDL Schema version. + * UdsCerts, + * DiceCertChain, + * SignedData<[ + * challenge: bstr .size (32..64), ; Provided by the method parameters + * bstr .cbor T, + * ]>, * ] * * ; COSE_Sign1 (untagged) From fb49ad2f3cb8e3ac7f2de81c7524ccaa8bb75e15 Mon Sep 17 00:00:00 2001 From: Andrew Scull Date: Wed, 9 Nov 2022 21:02:48 +0000 Subject: [PATCH 2/2] Update the VTS test for CSRv3 updates Conform to the latest CDDL changes. Organize parsing to observe the AuthenticatedRequest structure. Return the deserialized CSR payload rather than the DICE chain keys because it simplified the return types. The return value is only used by one VTS test that checks sequential CSRs consist of the same request. The test was incomplete before and it now only looks as the CSR payload whereas it previously only look at the DICE chain keys. Bug: 250910137 Test: atest libkeymint_remote_prov_support_test librkp_factory_extraction_test Test: atest VtsHalRemotelyProvisionedComponentTargetTest Change-Id: I1ba2e0cec22e25312fb890923a4c93043e9046cd --- .../VtsRemotelyProvisionedComponentTests.cpp | 16 +- .../include/remote_prov/remote_prov_utils.h | 9 +- .../keymint/support/remote_prov_utils.cpp | 189 ++++++++++-------- 3 files changed, 121 insertions(+), 93 deletions(-) diff --git a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp index 4f361bb464..5b11741cef 100644 --- a/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp +++ b/security/keymint/aidl/vts/functional/VtsRemotelyProvisionedComponentTests.cpp @@ -701,7 +701,8 @@ TEST_P(CertificateRequestV2Test, NonEmptyRequest) { } /** - * Generate a non-empty certificate request. Make sure contents are reproducible. + * Generate a non-empty certificate request. Make sure contents are reproducible but allow for the + * signature to be different since algorithms including ECDSA P-256 can include a random value. */ TEST_P(CertificateRequestV2Test, NonEmptyRequestReproducible) { generateKeys(false /* testMode */, 1 /* numKeys */); @@ -711,19 +712,16 @@ TEST_P(CertificateRequestV2Test, NonEmptyRequestReproducible) { auto status = provisionable_->generateCertificateRequestV2(keysToSign_, challenge_, &csr); ASSERT_TRUE(status.isOk()) << status.getMessage(); - auto firstBcc = verifyProductionCsr(cborKeysToSign_, csr, provisionable_.get(), challenge_); - ASSERT_TRUE(firstBcc) << firstBcc.message(); + auto firstCsr = verifyProductionCsr(cborKeysToSign_, csr, provisionable_.get(), challenge_); + ASSERT_TRUE(firstCsr) << firstCsr.message(); status = provisionable_->generateCertificateRequestV2(keysToSign_, challenge_, &csr); ASSERT_TRUE(status.isOk()) << status.getMessage(); - auto secondBcc = verifyProductionCsr(cborKeysToSign_, csr, provisionable_.get(), challenge_); - ASSERT_TRUE(secondBcc) << secondBcc.message(); + auto secondCsr = verifyProductionCsr(cborKeysToSign_, csr, provisionable_.get(), challenge_); + ASSERT_TRUE(secondCsr) << secondCsr.message(); - ASSERT_EQ(firstBcc->size(), secondBcc->size()); - for (auto i = 0; i < firstBcc->size(); i++) { - ASSERT_EQ(firstBcc->at(i).pubKey, secondBcc->at(i).pubKey); - } + ASSERT_EQ(**firstCsr, **secondCsr); } /** diff --git a/security/keymint/support/include/remote_prov/remote_prov_utils.h b/security/keymint/support/include/remote_prov/remote_prov_utils.h index 6871e1b272..1b94c62d66 100644 --- a/security/keymint/support/include/remote_prov/remote_prov_utils.h +++ b/security/keymint/support/include/remote_prov/remote_prov_utils.h @@ -181,14 +181,13 @@ ErrMsgOr> verifyProductionProtectedData( * Verify the CSR as if the device is still early in the factory process and may not * have all device identifiers provisioned yet. */ -ErrMsgOr> verifyFactoryCsr(const cppbor::Array& keysToSign, - const std::vector& csr, - IRemotelyProvisionedComponent* provisionable, - const std::vector& challenge); +ErrMsgOr> verifyFactoryCsr( + const cppbor::Array& keysToSign, const std::vector& csr, + IRemotelyProvisionedComponent* provisionable, const std::vector& challenge); /** * Verify the CSR as if the device is a final production sample. */ -ErrMsgOr> verifyProductionCsr( +ErrMsgOr> verifyProductionCsr( const cppbor::Array& keysToSign, const std::vector& csr, IRemotelyProvisionedComponent* provisionable, const std::vector& challenge); diff --git a/security/keymint/support/remote_prov_utils.cpp b/security/keymint/support/remote_prov_utils.cpp index f7ab3ac6fd..7e164fd51a 100644 --- a/security/keymint/support/remote_prov_utils.cpp +++ b/security/keymint/support/remote_prov_utils.cpp @@ -521,11 +521,10 @@ ErrMsgOr> parseAndValidateDeviceInfo( return errMsg; } - std::unique_ptr parsed(parsedVerifiedDeviceInfo->asMap()); + std::unique_ptr parsed(parsedVerifiedDeviceInfo.release()->asMap()); if (!parsed) { return "DeviceInfo must be a CBOR map."; } - parsedVerifiedDeviceInfo.release(); if (parsed->clone()->asMap()->canonicalize().encode() != deviceInfoBytes) { return "DeviceInfo ordering is non-canonical."; @@ -846,54 +845,79 @@ std::string validateUdsCerts(const cppbor::Map& udsCerts, const bytevec& udsPub) return ""; } -ErrMsgOr parseAndValidateCsrPayload(const cppbor::Array& keysToSign, - const std::vector& csrPayload, - IRemotelyProvisionedComponent* provisionable, - const std::vector& challenge, - bool isFactory) { +ErrMsgOr> parseAndValidateCsrPayload( + const cppbor::Array& keysToSign, const std::vector& csrPayload, + IRemotelyProvisionedComponent* provisionable, bool isFactory) { auto [parsedCsrPayload, _, errMsg] = cppbor::parse(csrPayload); if (!parsedCsrPayload) { return errMsg; } - if (!parsedCsrPayload->asArray()) { + + std::unique_ptr parsed(parsedCsrPayload.release()->asArray()); + if (!parsed) { return "CSR payload is not a CBOR array."; } - if (parsedCsrPayload->asArray()->size() != 5U) { - return "CSR payload must contain version, certificate type, device info, challenge, keys. " + + if (parsed->size() != 4U) { + return "CSR payload must contain version, certificate type, device info, keys. " "However, the parsed CSR payload has " + - std::to_string(parsedCsrPayload->asArray()->size()) + " entries."; + std::to_string(parsed->size()) + " entries."; } - auto& signedVersion = parsedCsrPayload->asArray()->get(0); - auto& signedCertificateType = parsedCsrPayload->asArray()->get(1); - auto& signedDeviceInfo = parsedCsrPayload->asArray()->get(2); - auto& signedChallenge = parsedCsrPayload->asArray()->get(3); - auto& signedKeys = parsedCsrPayload->asArray()->get(4); + auto signedVersion = parsed->get(0)->asUint(); + auto signedCertificateType = parsed->get(1)->asTstr(); + auto signedDeviceInfo = parsed->get(2)->asMap(); + auto signedKeys = parsed->get(3)->asArray(); - if (!signedVersion || !signedVersion->asUint() || signedVersion->asUint()->value() != 1U) { - return "CSR payload version must be an unsigned integer and must be equal to 1."; + if (!signedVersion || signedVersion->value() != 3U) { + return "CSR payload version must be an unsigned integer and must be equal to 3."; } - if (!signedCertificateType || !signedCertificateType->asTstr()) { + if (!signedCertificateType) { // Certificate type is allowed to be extendend by vendor, i.e. we can't // enforce its value. return "Certificate type must be a Tstr."; } - if (!signedDeviceInfo || !signedDeviceInfo->asMap()) { + if (!signedDeviceInfo) { return "Device info must be an Map."; } - if (!signedChallenge || !signedChallenge->asBstr()) { - return "Challenge must be a Bstr."; - } - if (!signedKeys || !signedKeys->asArray()) { + if (!signedKeys) { return "Keys must be an Array."; } - auto result = parseAndValidateDeviceInfo(signedDeviceInfo->asMap()->encode(), provisionable, - isFactory); + auto result = parseAndValidateDeviceInfo(signedDeviceInfo->encode(), provisionable, isFactory); if (!result) { return result.message(); } + if (signedKeys->encode() != keysToSign.encode()) { + return "Signed keys do not match."; + } + + return std::move(parsed); +} + +ErrMsgOr parseAndValidateAuthenticatedRequestSignedPayload( + const std::vector& signedPayload, const std::vector& challenge) { + auto [parsedSignedPayload, _, errMsg] = cppbor::parse(signedPayload); + if (!parsedSignedPayload) { + return errMsg; + } + if (!parsedSignedPayload->asArray()) { + return "SignedData payload is not a CBOR array."; + } + if (parsedSignedPayload->asArray()->size() != 2U) { + return "SignedData payload must contain the challenge and request. However, the parsed " + "SignedData payload has " + + std::to_string(parsedSignedPayload->asArray()->size()) + " entries."; + } + + auto signedChallenge = parsedSignedPayload->asArray()->get(0)->asBstr(); + auto signedRequest = parsedSignedPayload->asArray()->get(1)->asBstr(); + + if (!signedChallenge) { + return "Challenge must be a Bstr."; + } + if (challenge.size() < 32 || challenge.size() > 64) { return "Challenge size must be between 32 and 64 bytes inclusive. " "However, challenge is " + @@ -901,68 +925,57 @@ ErrMsgOr parseAndValidateCsrPayload(const cppbor::Array& keysToSi } auto challengeBstr = cppbor::Bstr(challenge); - if (*signedChallenge->asBstr() != challengeBstr) { + if (*signedChallenge != challengeBstr) { return "Signed challenge does not match." "\n Actual: " + cppbor::prettyPrint(signedChallenge->asBstr(), 64 /* maxBStrSize */) + "\nExpected: " + cppbor::prettyPrint(&challengeBstr, 64 /* maxBStrSize */); } - if (signedKeys->asArray()->encode() != keysToSign.encode()) { - return "Signed keys do not match."; + if (!signedRequest) { + return "Request must be a Bstr."; } - return std::move(*parsedCsrPayload->asArray()); + return signedRequest->value(); } -ErrMsgOr> verifyCsr(const cppbor::Array& keysToSign, - const std::vector& csr, - IRemotelyProvisionedComponent* provisionable, - const std::vector& challenge, - bool isFactory) { - auto [parsedCsr, _, csrErrMsg] = cppbor::parse(csr); - if (!parsedCsr) { +ErrMsgOr parseAndValidateAuthenticatedRequest(const std::vector& request, + const std::vector& challenge) { + auto [parsedRequest, _, csrErrMsg] = cppbor::parse(request); + if (!parsedRequest) { return csrErrMsg; } - if (!parsedCsr->asArray()) { - return "CSR is not a CBOR array."; + if (!parsedRequest->asArray()) { + return "AuthenticatedRequest is not a CBOR array."; } - if (parsedCsr->asArray()->size() != 4U) { - return "CSR must contain version, UDS certificates, DICE chain, and signed data. " - "However, the parsed CSR has " + - std::to_string(parsedCsr->asArray()->size()) + " entries."; + if (parsedRequest->asArray()->size() != 4U) { + return "AuthenticatedRequest must contain version, UDS certificates, DICE chain, and " + "signed data. However, the parsed AuthenticatedRequest has " + + std::to_string(parsedRequest->asArray()->size()) + " entries."; } - auto& version = parsedCsr->asArray()->get(0); - auto& udsCerts = parsedCsr->asArray()->get(1); - auto& diceCertChain = parsedCsr->asArray()->get(2); - auto& signedData = parsedCsr->asArray()->get(3); + auto version = parsedRequest->asArray()->get(0)->asUint(); + auto udsCerts = parsedRequest->asArray()->get(1)->asMap(); + auto diceCertChain = parsedRequest->asArray()->get(2)->asArray(); + auto signedData = parsedRequest->asArray()->get(3)->asArray(); - if (!version || !version->asUint() || version->asUint()->value() != 3U) { - return "Version must be an unsigned integer and must be equal to 3."; + if (!version || version->value() != 1U) { + return "AuthenticatedRequest version must be an unsigned integer and must be equal to 1."; } - if (!udsCerts || !udsCerts->asMap()) { - return "UdsCerts must be an Map."; + if (!udsCerts) { + return "AuthenticatedRequest UdsCerts must be an Map."; } - if (!diceCertChain || !diceCertChain->asArray()) { - return "DiceCertChain must be an Array."; + if (!diceCertChain) { + return "AuthenticatedRequest DiceCertChain must be an Array."; } - if (!signedData || !signedData->asArray()) { - return "SignedData must be an Array."; - } - - RpcHardwareInfo info; - provisionable->getHardwareInfo(&info); - if (version->asUint()->value() != info.versionNumber) { - return "CSR version (" + std::to_string(version->asUint()->value()) + - ") does not match the remotely provisioned component version (" + - std::to_string(info.versionNumber) + ")."; + if (!signedData) { + return "AuthenticatedRequest SignedData must be an Array."; } // DICE chain is [ pubkey, + DiceChainEntry ]. Its format is the same as BCC from RKP v1-2. - auto diceContents = validateBcc(diceCertChain->asArray()); + auto diceContents = validateBcc(diceCertChain); if (!diceContents) { - return diceContents.message() + "\n" + prettyPrint(diceCertChain.get()); + return diceContents.message() + "\n" + prettyPrint(diceCertChain); } if (diceContents->size() == 0U) { return "The DICE chain is empty. It must contain at least one entry."; @@ -970,33 +983,51 @@ ErrMsgOr> verifyCsr(const cppbor::Array& keysToSign, auto& udsPub = diceContents->back().pubKey; - auto error = validateUdsCerts(*udsCerts->asMap(), udsPub); + auto error = validateUdsCerts(*udsCerts, udsPub); if (!error.empty()) { return error; } - auto csrPayload = verifyAndParseCoseSign1(signedData->asArray(), udsPub, {} /* aad */); + auto signedPayload = verifyAndParseCoseSign1(signedData, udsPub, {} /* aad */); + if (!signedPayload) { + return signedPayload.message(); + } + + auto payload = parseAndValidateAuthenticatedRequestSignedPayload(*signedPayload, challenge); + if (!payload) { + return payload.message(); + } + + return payload; +} + +ErrMsgOr> verifyCsr(const cppbor::Array& keysToSign, + const std::vector& csr, + IRemotelyProvisionedComponent* provisionable, + const std::vector& challenge, + bool isFactory) { + RpcHardwareInfo info; + provisionable->getHardwareInfo(&info); + if (info.versionNumber != 3) { + return "Remotely provisioned component version (" + std::to_string(info.versionNumber) + + ") does not match expected version (3)."; + } + + auto csrPayload = parseAndValidateAuthenticatedRequest(csr, challenge); if (!csrPayload) { return csrPayload.message(); } - auto parsedCsrPayload = parseAndValidateCsrPayload(keysToSign, *csrPayload, provisionable, - challenge, isFactory); - if (!parsedCsrPayload) { - return parsedCsrPayload.message(); - } - - return *diceContents; + return parseAndValidateCsrPayload(keysToSign, *csrPayload, provisionable, isFactory); } -ErrMsgOr> verifyFactoryCsr(const cppbor::Array& keysToSign, - const std::vector& csr, - IRemotelyProvisionedComponent* provisionable, - const std::vector& challenge) { +ErrMsgOr> verifyFactoryCsr( + const cppbor::Array& keysToSign, const std::vector& csr, + IRemotelyProvisionedComponent* provisionable, const std::vector& challenge) { return verifyCsr(keysToSign, csr, provisionable, challenge, /*isFactory=*/true); } -ErrMsgOr> verifyProductionCsr( +ErrMsgOr> verifyProductionCsr( const cppbor::Array& keysToSign, const std::vector& csr, IRemotelyProvisionedComponent* provisionable, const std::vector& challenge) { return verifyCsr(keysToSign, csr, provisionable, challenge, /*isFactory=*/false);