From fb49ad2f3cb8e3ac7f2de81c7524ccaa8bb75e15 Mon Sep 17 00:00:00 2001 From: Andrew Scull Date: Wed, 9 Nov 2022 21:02:48 +0000 Subject: [PATCH] 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);