From a11e4b30598583a2e3860b7d07d0a9299458c944 Mon Sep 17 00:00:00 2001 From: Vijay Selvaraj Date: Tue, 2 Jan 2024 20:57:57 -0500 Subject: [PATCH 1/5] Added an interface for revocation checks to DeviceAttestationVerifier --- examples/chip-tool/BUILD.gn | 1 + .../chip-tool/commands/common/CHIPCommand.h | 5 + .../common/CredentialIssuerCommands.h | 3 +- .../example/ExampleCredentialIssuerCommands.h | 5 +- src/controller/python/BUILD.gn | 1 + src/credentials/BUILD.gn | 15 ++ .../DefaultDeviceAttestationVerifier.cpp | 57 +++++- .../DefaultDeviceAttestationVerifier.h | 10 +- .../DeviceAttestationVerifier.cpp | 6 + .../DeviceAttestationVerifier.h | 22 ++ .../JSONFileRevocationSet.cpp | 189 ++++++++++++++++++ .../JSONFileRevocationSet.h | 58 ++++++ 12 files changed, 365 insertions(+), 7 deletions(-) create mode 100644 src/credentials/attestation_verifier/JSONFileRevocationSet.cpp create mode 100644 src/credentials/attestation_verifier/JSONFileRevocationSet.h diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn index 10d03c02ee295d..2726586ab4a14d 100644 --- a/examples/chip-tool/BUILD.gn +++ b/examples/chip-tool/BUILD.gn @@ -109,6 +109,7 @@ static_library("chip-tool-utils") { "${chip_root}/src/app/tests/suites/commands/interaction_model", "${chip_root}/src/controller/data_model", "${chip_root}/src/credentials:file_attestation_trust_store", + "${chip_root}/src/credentials:json_file_revocation_set", "${chip_root}/src/lib", "${chip_root}/src/lib/core:types", "${chip_root}/src/lib/support/jsontlv", diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index 55817b5213e589..9e5f719236042a 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -69,6 +69,9 @@ class CHIPCommand : public Command CHIPCommand(const char * commandName, CredentialIssuerCommands * credIssuerCmds, const char * helpText = nullptr) : Command(commandName, helpText), mCredIssuerCmds(credIssuerCmds) { + AddArgument( + "revocation-set-path", &mRevocationSetPath, + "Path to file holding a list of Revoked Certificates. Can be absolute or relative to the current working directory."); AddArgument("paa-trust-store-path", &mPaaTrustStorePath, "Path to directory holding PAA certificate information. Can be absolute or relative to the current working " "directory."); @@ -218,6 +221,7 @@ class CHIPCommand : public Command chip::Optional mCommissionerNodeId; chip::Optional mCommissionerVendorId; chip::Optional mBleAdapterId; + chip::Optional mRevocationSetPath; chip::Optional mPaaTrustStorePath; chip::Optional mCDTrustStorePath; chip::Optional mUseMaxSizedCerts; @@ -226,6 +230,7 @@ class CHIPCommand : public Command // Cached trust store so commands other than the original startup command // can spin up commissioners as needed. static const chip::Credentials::AttestationTrustStore * sTrustStore; + static const chip::Credentials::RevocationSet * sRevocationSet; static void RunQueuedCommand(intptr_t commandArg); typedef decltype(RunQueuedCommand) MatterWorkCallback; diff --git a/examples/chip-tool/commands/common/CredentialIssuerCommands.h b/examples/chip-tool/commands/common/CredentialIssuerCommands.h index afb36773529e1e..fc67c12acc96c1 100644 --- a/examples/chip-tool/commands/common/CredentialIssuerCommands.h +++ b/examples/chip-tool/commands/common/CredentialIssuerCommands.h @@ -59,7 +59,8 @@ class CredentialIssuerCommands * @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error code. */ virtual CHIP_ERROR SetupDeviceAttestation(chip::Controller::SetupParams & setupParams, - const chip::Credentials::AttestationTrustStore * trustStore) = 0; + const chip::Credentials::AttestationTrustStore * trustStore, + const chip::Credentials::RevocationSet * revocationSet) = 0; /** * @brief Add a list of additional non-default CD verifying keys (by certificate) diff --git a/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h b/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h index a23b45eae10627..07936e52820412 100644 --- a/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h +++ b/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h @@ -34,11 +34,12 @@ class ExampleCredentialIssuerCommands : public CredentialIssuerCommands return mOpCredsIssuer.Initialize(storage); } CHIP_ERROR SetupDeviceAttestation(chip::Controller::SetupParams & setupParams, - const chip::Credentials::AttestationTrustStore * trustStore) override + const chip::Credentials::AttestationTrustStore * trustStore, + const chip::Credentials::RevocationSet * revocationSet) override { chip::Credentials::SetDeviceAttestationCredentialsProvider(chip::Credentials::Examples::GetExampleDACProvider()); - mDacVerifier = chip::Credentials::GetDefaultDACVerifier(trustStore); + mDacVerifier = chip::Credentials::GetDefaultDACVerifier(trustStore, revocationSet); setupParams.deviceAttestationVerifier = mDacVerifier; mDacVerifier->EnableCdTestKeySupport(mAllowTestCdSigningKey); diff --git a/src/controller/python/BUILD.gn b/src/controller/python/BUILD.gn index 5fc2212098fea8..b9bb947950b98c 100644 --- a/src/controller/python/BUILD.gn +++ b/src/controller/python/BUILD.gn @@ -136,6 +136,7 @@ shared_library("ChipDeviceCtrl") { public_deps += [ "${chip_root}/src/controller/data_model", "${chip_root}/src/credentials:file_attestation_trust_store", + "${chip_root}/src/credentials:json_file_revocation_set", "${chip_root}/src/lib/support:testing", "${chip_root}/src/tracing/json", "${chip_root}/src/tracing/perfetto", diff --git a/src/credentials/BUILD.gn b/src/credentials/BUILD.gn index df7afc0c1025ba..b5ffe87511aa67 100644 --- a/src/credentials/BUILD.gn +++ b/src/credentials/BUILD.gn @@ -185,3 +185,18 @@ static_library("file_attestation_trust_store") { "${nlassert_root}:nlassert", ] } + +static_library("json_file_revocation_set") { + output_name = "libJSONFileRevocationSet" + + sources = [ + "attestation_verifier/JSONFileRevocationSet.cpp", + "attestation_verifier/JSONFileRevocationSet.h", + ] + + public_deps = [ + ":credentials", + "${chip_root}/third_party/jsoncpp", + "${nlassert_root}:nlassert", + ] +} diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp index 0d7f67ff82b0f6..2f83de8f6e1648 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp @@ -458,6 +458,50 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, attestationError = attestationError); } + { + uint8_t issuerBuf[kMaxCertificateDistinguishedNameLength] = { 0 }; + MutableByteSpan paaIssuer(issuerBuf); + MutableByteSpan paiIssuer(issuerBuf); + MutableByteSpan dacIssuer(issuerBuf); + uint8_t akidBuf[kAuthorityKeyIdentifierLength]; + MutableByteSpan akid(akidBuf); + uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength]; + MutableByteSpan serialNumber(serialNumberBuf); + + VerifyOrExit(ExtractIssuerFromX509Cert(paaDerBuffer, paaIssuer) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kPaaFormatInvalid); + VerifyOrExit(ExtractAKIDFromX509Cert(paaDerBuffer, akid) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kPaaFormatInvalid); + VerifyOrExit(ExtractSerialNumberFromX509Cert(paaDerBuffer, serialNumber) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kPaaFormatInvalid); + + attestationError = IsCertificateRevoked(true, paaVidPid, paaIssuer, akid, serialNumber); + VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, + attestationError = AttestationVerificationResult::kPaaRevoked); + + VerifyOrExit(ExtractIssuerFromX509Cert(info.paiDerBuffer, paiIssuer) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kPaiFormatInvalid); + VerifyOrExit(ExtractAKIDFromX509Cert(info.paiDerBuffer, akid) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kPaiFormatInvalid); + VerifyOrExit(ExtractSerialNumberFromX509Cert(info.paiDerBuffer, serialNumber) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kPaiFormatInvalid); + + attestationError = IsCertificateRevoked(false, paiVidPid, paiIssuer, akid, serialNumber); + VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, + attestationError = AttestationVerificationResult::kPaiRevoked); + + VerifyOrExit(ExtractIssuerFromX509Cert(info.dacDerBuffer, dacIssuer) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kDacFormatInvalid); + VerifyOrExit(ExtractAKIDFromX509Cert(info.dacDerBuffer, akid) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kDacFormatInvalid); + VerifyOrExit(ExtractSerialNumberFromX509Cert(info.dacDerBuffer, serialNumber) == CHIP_NO_ERROR, + attestationError = AttestationVerificationResult::kDacFormatInvalid); + + attestationError = IsCertificateRevoked(false, dacVidPid, dacIssuer, akid, serialNumber); + VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, + attestationError = AttestationVerificationResult::kDacRevoked); + } + exit: onCompletion->mCall(onCompletion->mContext, info, attestationError); } @@ -607,6 +651,15 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa return CHIP_NO_ERROR; } +AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, AttestationCertVidPid vidPidUnderTest, + ByteSpan issuer, ByteSpan authorityKeyId, + ByteSpan serialNumber) +{ + VerifyOrReturnError(mRevocationSet != nullptr, AttestationVerificationResult::kNotImplemented); + + return mRevocationSet->IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, authorityKeyId, serialNumber); +} + bool CsaCdKeysTrustStore::IsCdTestKey(const ByteSpan & kid) const { return kid.data_equal(ByteSpan{ gTestCdPubkeyKid }); @@ -683,9 +736,9 @@ const AttestationTrustStore * GetTestAttestationTrustStore() return &gTestAttestationTrustStore.get(); } -DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore) +DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet) { - static DefaultDACVerifier defaultDACVerifier{ paaRootStore }; + static DefaultDACVerifier defaultDACVerifier{ paaRootStore, revocationSet }; return &defaultDACVerifier; } diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h index bdf7f705dbd0f1..cead19a46349fc 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h @@ -57,7 +57,9 @@ class CsaCdKeysTrustStore : public WellKnownKeysTrustStore class DefaultDACVerifier : public DeviceAttestationVerifier { public: - DefaultDACVerifier(const AttestationTrustStore * paaRootStore) : mAttestationTrustStore(paaRootStore) {} + DefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet) : + mAttestationTrustStore(paaRootStore), mRevocationSet(revocationSet) + {} void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info, Callback::Callback * onCompletion) override; @@ -74,6 +76,9 @@ class DefaultDACVerifier : public DeviceAttestationVerifier const ByteSpan & attestationSignatureBuffer, const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override; + AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, ByteSpan issuer, + ByteSpan authorityKeyId, ByteSpan serialNumber) override; + CsaCdKeysTrustStore * GetCertificationDeclarationTrustStore() override { return &mCdKeysTrustStore; } protected: @@ -81,6 +86,7 @@ class DefaultDACVerifier : public DeviceAttestationVerifier CsaCdKeysTrustStore mCdKeysTrustStore; const AttestationTrustStore * mAttestationTrustStore; + const RevocationSet * mRevocationSet; }; /** @@ -109,7 +115,7 @@ const AttestationTrustStore * GetTestAttestationTrustStore(); * process lifetime. In particular, after the first call it's not * possible to change which AttestationTrustStore is used by this verifier. */ -DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore); +DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet); } // namespace Credentials } // namespace chip diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp index cc5d9d3030de80..e45628d7151976 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp @@ -69,6 +69,12 @@ class UnimplementedDACVerifier : public DeviceAttestationVerifier (void) csrNonce; return CHIP_ERROR_NOT_IMPLEMENTED; } + + AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, ByteSpan issuer, + ByteSpan authorityKeyId, ByteSpan serialNumber) override + { + return AttestationVerificationResult::kNotImplemented; + } }; // Default to avoid nullptr on getter and cleanly handle new products/clients before diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h index 9465d934207d1d..54fc959f4e1d82 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h @@ -259,6 +259,25 @@ class ArrayAttestationTrustStore : public AttestationTrustStore const size_t mNumCerts; }; +/** + * @brief Helper utility to model a basic Revocation Set usable for device attestation verifiers. + * + */ +class RevocationSet +{ +public: + RevocationSet() = default; + virtual ~RevocationSet() = default; + + // Not copyable + RevocationSet(const RevocationSet &) = delete; + RevocationSet & operator=(const RevocationSet &) = delete; + + virtual AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, + ByteSpan issuer, ByteSpan authorityKeyId, + ByteSpan serialNumber) const = 0; +}; + class DeviceAttestationVerifier { public: @@ -386,6 +405,9 @@ class DeviceAttestationVerifier const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) = 0; + virtual AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, + ByteSpan issuer, ByteSpan authorityKeyId, ByteSpan serialNumber) = 0; + /** * @brief Get the trust store used for the attestation verifier. * diff --git a/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp b/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp new file mode 100644 index 00000000000000..c9e6a29f992a80 --- /dev/null +++ b/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp @@ -0,0 +1,189 @@ +/* + * + * Copyright (c) 2022 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "JSONFileRevocationSet.h" + +#include +#include +#include + +#include +#include +#include + +namespace chip { +namespace Credentials { + +JSONFileRevocationSet::JSONFileRevocationSet(const char * revocationSetPath) +{ + VerifyOrReturn(revocationSetPath != nullptr); + + if (revocationSetPath != nullptr) + { + LoadJSONRevocationSet(revocationSetPath, mRevocationSet); + VerifyOrReturn(revocationSetCount()); + } + + mIsInitialized = true; +} + +void LoadJSONRevocationSet(const char * revocationSetPath, Json::Value & revocationSet) +{ + VerifyOrReturn(revocationSetPath != nullptr); + + std::ifstream ifs(revocationSetPath); + Json::Reader reader; + reader.parse(ifs, revocationSet); +} + +AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, + ByteSpan issuer, ByteSpan authorityKeyId, + ByteSpan serialNumber) const +{ + for (size_t revocation_set_idx = 0; revocation_set_idx < mRevocationSet.size(); ++revocation_set_idx) + { + Json::Value type = mRevocationSet[static_cast(revocation_set_idx)]["type"]; + + // 1. + if (type.asString().compare("revocation_set") == 0) + { + Json::Value jsonCrlIssuerSubjectKeyId = mRevocationSet[static_cast(revocation_set_idx)]["issuer_subject_key_id"]; + Json::Value jsonCrlIssuerName = mRevocationSet[static_cast(revocation_set_idx)]["issuer_name"]; + Json::Value jsonCrlRevokedSerialNumbers = + mRevocationSet[static_cast(revocation_set_idx)]["revoked_serial_numbers"]; + + uint8_t crlIssuerSubjectKeyIdBuf[Crypto::kAuthorityKeyIdentifierLength] = { 0 }; + ByteSpan crlIssuerSubjectKeyId(crlIssuerSubjectKeyIdBuf); + + VerifyOrReturnError(Encoding::HexToBytes(jsonCrlIssuerSubjectKeyId.asString().c_str(), + jsonCrlIssuerSubjectKeyId.asString().size(), crlIssuerSubjectKeyIdBuf, + sizeof(crlIssuerSubjectKeyIdBuf)) == Crypto::kAuthorityKeyIdentifierLength, + AttestationVerificationResult::kInvalidArgument); + + size_t crlSignerCertificateMaxLength = BASE64_MAX_DECODED_LEN(jsonCrlIssuerName.asString().size()); + + Platform::ScopedMemoryBuffer crlSignerCertificate; + + ReturnErrorCodeIf(!crlSignerCertificate.Alloc(crlSignerCertificateMaxLength), AttestationVerificationResult::kNoMemory); + + // 2. + size_t crlSignerCertificateLength = + Base64Decode(jsonCrlIssuerName.asString().c_str(), jsonCrlIssuerName.asString().size(), crlSignerCertificate.Get()); + VerifyOrReturnError(crlSignerCertificateLength > 0 && crlSignerCertificateLength != UINT16_MAX, + AttestationVerificationResult::kInternalError); + + // 3. + if (isPaa) + { + Crypto::AttestationCertVidPid vid; + + Crypto::ExtractVIDPIDFromAttributeString( + Crypto::DNAttrType::kCommonName, ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vid, vid); + + if (vid.mVendorId.HasValue()) + { + if (vid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value()) + { + // VID does not match. Stop further processing and continue to next entry. + continue; + } + } + } + // 4. + else + { + Crypto::AttestationCertVidPid vidPid; + + Crypto::ExtractVIDPIDFromAttributeString(Crypto::DNAttrType::kCommonName, + ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vidPid, + vidPid); + + if (vidPid.mVendorId.HasValue() && vidPid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value()) + { + // VID does not match. Stop further processing and continue to next entry. + continue; + } + + if (vidPid.mProductId.HasValue()) + { + if (vidPid.mProductId.Value() != vidPidUnderTest.mProductId.Value()) + { + // PID does not match. Stop further processing and continue to next entry. + continue; + } + } + } + + // 7.a Perform CRLFile validation + if (authorityKeyId.data_equal(crlIssuerSubjectKeyId) == false) + { + continue; + } + + // TODO: 7.b + + // TODO: 8. + + // 9. && 10. + for (int serial_number_idx = 0; serial_number_idx < static_cast(jsonCrlRevokedSerialNumbers.size()); + ++serial_number_idx) + { + size_t revokedSerialNumberMaxLength = + BASE64_MAX_DECODED_LEN(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size()); + + Platform::ScopedMemoryBuffer revokedSerialNumber; + + ReturnErrorCodeIf(!revokedSerialNumber.Alloc(revokedSerialNumberMaxLength), + AttestationVerificationResult::kNoMemory); + + size_t revokedSerialNumberLength = + Base64Decode(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().c_str(), + jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size(), revokedSerialNumber.Get()); + VerifyOrReturnError(revokedSerialNumberLength > 0 && revokedSerialNumberLength != UINT16_MAX, + AttestationVerificationResult::kInternalError); + + uint8_t crlRevokedSerialNumberBuf[Crypto::kMaxCertificateSerialNumberLength] = { 0 }; + ByteSpan crlSerialNumber(crlRevokedSerialNumberBuf); + + VerifyOrReturnError(Encoding::HexToBytes(reinterpret_cast(revokedSerialNumber.Get()), + revokedSerialNumberLength, crlRevokedSerialNumberBuf, + sizeof(crlRevokedSerialNumberBuf)) == sizeof(crlRevokedSerialNumberBuf), + AttestationVerificationResult::kInvalidArgument); + + if (serialNumber.data_equal(crlSerialNumber)) + { + return AttestationVerificationResult::kPaaRevoked; + } + } + } + } + + return AttestationVerificationResult::kSuccess; +} + +JSONFileRevocationSet::~JSONFileRevocationSet() +{ + Cleanup(); +} + +void JSONFileRevocationSet::Cleanup() +{ + mRevocationSet.clear(); + mIsInitialized = false; +} + +} // namespace Credentials +} // namespace chip diff --git a/src/credentials/attestation_verifier/JSONFileRevocationSet.h b/src/credentials/attestation_verifier/JSONFileRevocationSet.h new file mode 100644 index 00000000000000..5ebdd8b33d49df --- /dev/null +++ b/src/credentials/attestation_verifier/JSONFileRevocationSet.h @@ -0,0 +1,58 @@ +/* + * + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#pragma once + +#include + +#include + +namespace chip { +namespace Credentials { + +/** + * @brief Load a Revocation Set from a given path. + * + * Returns an empty vector if no files are found or unrecoverable errors arise. + * + * @param revocationSetPath - path from where to search for a revocation set. + * @return a vector of certificate DER data + */ +void LoadJSONRevocationSet(const char * revocationSetPath, Json::Value & revocationSet); + +class JSONFileRevocationSet : public RevocationSet +{ +public: + JSONFileRevocationSet(const char * revocationSetPath = nullptr); + ~JSONFileRevocationSet(); + + AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, ByteSpan issuer, + ByteSpan authorityKeyId, ByteSpan serialNumber) const override; + + bool IsInitialized() const { return mIsInitialized; } + size_t revocationSetCount() const { return mRevocationSet.size(); }; + +protected: + Json::Value mRevocationSet; + +private: + bool mIsInitialized = false; + + void Cleanup(); +}; + +} // namespace Credentials +} // namespace chip From 4db45bfdd9f25da0a1bd3c256307be78ec75d8b1 Mon Sep 17 00:00:00 2001 From: Vijay Selvaraj Date: Tue, 5 Mar 2024 13:59:22 -0500 Subject: [PATCH 2/5] Addressed review comments --- scripts/tools/check_includes_config.py | 1 + .../DefaultDeviceAttestationVerifier.cpp | 58 ++++++-------- .../DefaultDeviceAttestationVerifier.h | 6 +- .../JSONFileRevocationSet.cpp | 78 ++++++++----------- 4 files changed, 61 insertions(+), 82 deletions(-) diff --git a/scripts/tools/check_includes_config.py b/scripts/tools/check_includes_config.py index 26689e4a2f85b4..42fdaeef9b6637 100644 --- a/scripts/tools/check_includes_config.py +++ b/scripts/tools/check_includes_config.py @@ -134,6 +134,7 @@ 'src/credentials/attestation_verifier/FileAttestationTrustStore.h': {'vector'}, 'src/credentials/attestation_verifier/FileAttestationTrustStore.cpp': {'string'}, + 'src/credentials/attestation_verifier/JSONFileRevocationSet.cpp': {'fstream'}, 'src/setup_payload/AdditionalDataPayload.h': {'string'}, 'src/setup_payload/AdditionalDataPayloadParser.cpp': {'vector'}, diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp index 2f83de8f6e1648..7960e241b99ec1 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp @@ -459,45 +459,15 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer } { - uint8_t issuerBuf[kMaxCertificateDistinguishedNameLength] = { 0 }; - MutableByteSpan paaIssuer(issuerBuf); - MutableByteSpan paiIssuer(issuerBuf); - MutableByteSpan dacIssuer(issuerBuf); - uint8_t akidBuf[kAuthorityKeyIdentifierLength]; - MutableByteSpan akid(akidBuf); - uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength]; - MutableByteSpan serialNumber(serialNumberBuf); - - VerifyOrExit(ExtractIssuerFromX509Cert(paaDerBuffer, paaIssuer) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kPaaFormatInvalid); - VerifyOrExit(ExtractAKIDFromX509Cert(paaDerBuffer, akid) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kPaaFormatInvalid); - VerifyOrExit(ExtractSerialNumberFromX509Cert(paaDerBuffer, serialNumber) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kPaaFormatInvalid); - - attestationError = IsCertificateRevoked(true, paaVidPid, paaIssuer, akid, serialNumber); + attestationError = IsCertificateRevoked(true, paaVidPid, paaDerBuffer); VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, attestationError = AttestationVerificationResult::kPaaRevoked); - VerifyOrExit(ExtractIssuerFromX509Cert(info.paiDerBuffer, paiIssuer) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kPaiFormatInvalid); - VerifyOrExit(ExtractAKIDFromX509Cert(info.paiDerBuffer, akid) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kPaiFormatInvalid); - VerifyOrExit(ExtractSerialNumberFromX509Cert(info.paiDerBuffer, serialNumber) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kPaiFormatInvalid); - - attestationError = IsCertificateRevoked(false, paiVidPid, paiIssuer, akid, serialNumber); + attestationError = IsCertificateRevoked(false, paiVidPid, info.paiDerBuffer); VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, attestationError = AttestationVerificationResult::kPaiRevoked); - VerifyOrExit(ExtractIssuerFromX509Cert(info.dacDerBuffer, dacIssuer) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kDacFormatInvalid); - VerifyOrExit(ExtractAKIDFromX509Cert(info.dacDerBuffer, akid) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kDacFormatInvalid); - VerifyOrExit(ExtractSerialNumberFromX509Cert(info.dacDerBuffer, serialNumber) == CHIP_NO_ERROR, - attestationError = AttestationVerificationResult::kDacFormatInvalid); - - attestationError = IsCertificateRevoked(false, dacVidPid, dacIssuer, akid, serialNumber); + attestationError = IsCertificateRevoked(false, dacVidPid, info.dacDerBuffer); VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, attestationError = AttestationVerificationResult::kDacRevoked); } @@ -651,11 +621,31 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa return CHIP_NO_ERROR; } +AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, + ByteSpan certificate) +{ + uint8_t issuerBuf[kMaxCertificateDistinguishedNameLength] = { 0 }; + MutableByteSpan issuer(issuerBuf); + uint8_t akidBuf[kAuthorityKeyIdentifierLength]; + MutableByteSpan akid(akidBuf); + uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength]; + MutableByteSpan serialNumber(serialNumberBuf); + + VerifyOrReturnError(ExtractIssuerFromX509Cert(certificate, issuer) == CHIP_NO_ERROR, + AttestationVerificationResult::kPaaFormatInvalid); + VerifyOrReturnError(ExtractAKIDFromX509Cert(certificate, akid) == CHIP_NO_ERROR, + AttestationVerificationResult::kPaaFormatInvalid); + VerifyOrReturnError(ExtractSerialNumberFromX509Cert(certificate, serialNumber) == CHIP_NO_ERROR, + AttestationVerificationResult::kPaaFormatInvalid); + + return IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, akid, serialNumber); +} + AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, AttestationCertVidPid vidPidUnderTest, ByteSpan issuer, ByteSpan authorityKeyId, ByteSpan serialNumber) { - VerifyOrReturnError(mRevocationSet != nullptr, AttestationVerificationResult::kNotImplemented); + VerifyOrReturnError(mRevocationSet != nullptr, AttestationVerificationResult::kSuccess); return mRevocationSet->IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, authorityKeyId, serialNumber); } diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h index cead19a46349fc..99a9a6ed94430f 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h @@ -87,6 +87,9 @@ class DefaultDACVerifier : public DeviceAttestationVerifier CsaCdKeysTrustStore mCdKeysTrustStore; const AttestationTrustStore * mAttestationTrustStore; const RevocationSet * mRevocationSet; + + AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, + ByteSpan certificate); }; /** @@ -115,7 +118,8 @@ const AttestationTrustStore * GetTestAttestationTrustStore(); * process lifetime. In particular, after the first call it's not * possible to change which AttestationTrustStore is used by this verifier. */ -DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet); +DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, + const RevocationSet * revocationSet = nullptr); } // namespace Credentials } // namespace chip diff --git a/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp b/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp index c9e6a29f992a80..e2fe70ce7b1c48 100644 --- a/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp +++ b/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp @@ -16,13 +16,13 @@ */ #include "JSONFileRevocationSet.h" +#include #include #include #include #include #include -#include namespace chip { namespace Credentials { @@ -53,17 +53,16 @@ AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool i ByteSpan issuer, ByteSpan authorityKeyId, ByteSpan serialNumber) const { - for (size_t revocation_set_idx = 0; revocation_set_idx < mRevocationSet.size(); ++revocation_set_idx) + for (int revocation_set_idx = 0; revocation_set_idx < static_cast(mRevocationSet.size()); ++revocation_set_idx) { - Json::Value type = mRevocationSet[static_cast(revocation_set_idx)]["type"]; + Json::Value revocationSetEntry = mRevocationSet[revocation_set_idx]; // 1. - if (type.asString().compare("revocation_set") == 0) + if (revocationSetEntry["type"].asString().compare("revocation_set") == 0) { - Json::Value jsonCrlIssuerSubjectKeyId = mRevocationSet[static_cast(revocation_set_idx)]["issuer_subject_key_id"]; - Json::Value jsonCrlIssuerName = mRevocationSet[static_cast(revocation_set_idx)]["issuer_name"]; - Json::Value jsonCrlRevokedSerialNumbers = - mRevocationSet[static_cast(revocation_set_idx)]["revoked_serial_numbers"]; + Json::Value jsonCrlIssuerSubjectKeyId = revocationSetEntry["issuer_subject_key_id"]; + Json::Value jsonCrlIssuerName = revocationSetEntry["issuer_name"]; + Json::Value jsonCrlRevokedSerialNumbers = revocationSetEntry["revoked_serial_numbers"]; uint8_t crlIssuerSubjectKeyIdBuf[Crypto::kAuthorityKeyIdentifierLength] = { 0 }; ByteSpan crlIssuerSubjectKeyId(crlIssuerSubjectKeyIdBuf); @@ -81,62 +80,46 @@ AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool i // 2. size_t crlSignerCertificateLength = - Base64Decode(jsonCrlIssuerName.asString().c_str(), jsonCrlIssuerName.asString().size(), crlSignerCertificate.Get()); + Base64Decode(jsonCrlIssuerName.asString().c_str(), static_cast(jsonCrlIssuerName.asString().size()), + crlSignerCertificate.Get()); VerifyOrReturnError(crlSignerCertificateLength > 0 && crlSignerCertificateLength != UINT16_MAX, AttestationVerificationResult::kInternalError); - // 3. - if (isPaa) - { - Crypto::AttestationCertVidPid vid; + // 3. && 4. + Crypto::AttestationCertVidPid vidPid; - Crypto::ExtractVIDPIDFromAttributeString( - Crypto::DNAttrType::kCommonName, ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vid, vid); + Crypto::ExtractVIDPIDFromAttributeString( + Crypto::DNAttrType::kCommonName, ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vidPid, vidPid); - if (vid.mVendorId.HasValue()) + if (vidPid.mVendorId.HasValue() && vidPid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value()) + { + // VID does not match. Stop further processing and continue to next entry. + continue; + } + + if (isPaa) + { + if (vidPid.mProductId.HasValue()) { - if (vid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value()) - { - // VID does not match. Stop further processing and continue to next entry. - continue; - } + // PAA must not contain PID entry. Format wrong. Continuing to next entry. + continue; } } - // 4. else { - Crypto::AttestationCertVidPid vidPid; - - Crypto::ExtractVIDPIDFromAttributeString(Crypto::DNAttrType::kCommonName, - ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vidPid, - vidPid); - - if (vidPid.mVendorId.HasValue() && vidPid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value()) + if (vidPid.mProductId.HasValue() && vidPid.mProductId.Value() != vidPidUnderTest.mProductId.Value()) { - // VID does not match. Stop further processing and continue to next entry. + // PID does not match. Stop further processing and continue to next entry. continue; } - - if (vidPid.mProductId.HasValue()) - { - if (vidPid.mProductId.Value() != vidPidUnderTest.mProductId.Value()) - { - // PID does not match. Stop further processing and continue to next entry. - continue; - } - } } - // 7.a Perform CRLFile validation + // 7. Perform CRLFile validation if (authorityKeyId.data_equal(crlIssuerSubjectKeyId) == false) { continue; } - // TODO: 7.b - - // TODO: 8. - // 9. && 10. for (int serial_number_idx = 0; serial_number_idx < static_cast(jsonCrlRevokedSerialNumbers.size()); ++serial_number_idx) @@ -151,15 +134,16 @@ AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool i size_t revokedSerialNumberLength = Base64Decode(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().c_str(), - jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size(), revokedSerialNumber.Get()); + static_cast(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size()), + revokedSerialNumber.Get()); VerifyOrReturnError(revokedSerialNumberLength > 0 && revokedSerialNumberLength != UINT16_MAX, AttestationVerificationResult::kInternalError); uint8_t crlRevokedSerialNumberBuf[Crypto::kMaxCertificateSerialNumberLength] = { 0 }; ByteSpan crlSerialNumber(crlRevokedSerialNumberBuf); - VerifyOrReturnError(Encoding::HexToBytes(reinterpret_cast(revokedSerialNumber.Get()), - revokedSerialNumberLength, crlRevokedSerialNumberBuf, + VerifyOrReturnError(Encoding::HexToBytes(Uint8::to_char(revokedSerialNumber.Get()), revokedSerialNumberLength, + crlRevokedSerialNumberBuf, sizeof(crlRevokedSerialNumberBuf)) == sizeof(crlRevokedSerialNumberBuf), AttestationVerificationResult::kInvalidArgument); From 11838c6616a08cc527771748a884bef809ff0920 Mon Sep 17 00:00:00 2001 From: Vijay Selvaraj Date: Fri, 15 Mar 2024 12:44:42 -0400 Subject: [PATCH 3/5] Removed the changes to add revocation set support to chip-tool and it will be submitted as a followup PR --- examples/chip-tool/BUILD.gn | 1 - .../chip-tool/commands/common/CHIPCommand.h | 5 - .../common/CredentialIssuerCommands.h | 3 +- .../example/ExampleCredentialIssuerCommands.h | 5 +- scripts/tools/check_includes_config.py | 1 - src/controller/AutoCommissioner.cpp | 2 + src/controller/CHIPDeviceController.cpp | 98 +++++++++- src/controller/CHIPDeviceController.h | 13 ++ src/controller/CommissioningDelegate.h | 1 + src/controller/python/BUILD.gn | 1 - src/credentials/BUILD.gn | 15 -- .../DefaultDeviceAttestationVerifier.cpp | 47 +---- .../DefaultDeviceAttestationVerifier.h | 15 +- .../DeviceAttestationVerifier.cpp | 7 +- .../DeviceAttestationVerifier.h | 23 +-- .../JSONFileRevocationSet.cpp | 173 ------------------ .../JSONFileRevocationSet.h | 58 ------ 17 files changed, 132 insertions(+), 336 deletions(-) delete mode 100644 src/credentials/attestation_verifier/JSONFileRevocationSet.cpp delete mode 100644 src/credentials/attestation_verifier/JSONFileRevocationSet.h diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn index 2726586ab4a14d..10d03c02ee295d 100644 --- a/examples/chip-tool/BUILD.gn +++ b/examples/chip-tool/BUILD.gn @@ -109,7 +109,6 @@ static_library("chip-tool-utils") { "${chip_root}/src/app/tests/suites/commands/interaction_model", "${chip_root}/src/controller/data_model", "${chip_root}/src/credentials:file_attestation_trust_store", - "${chip_root}/src/credentials:json_file_revocation_set", "${chip_root}/src/lib", "${chip_root}/src/lib/core:types", "${chip_root}/src/lib/support/jsontlv", diff --git a/examples/chip-tool/commands/common/CHIPCommand.h b/examples/chip-tool/commands/common/CHIPCommand.h index 9e5f719236042a..55817b5213e589 100644 --- a/examples/chip-tool/commands/common/CHIPCommand.h +++ b/examples/chip-tool/commands/common/CHIPCommand.h @@ -69,9 +69,6 @@ class CHIPCommand : public Command CHIPCommand(const char * commandName, CredentialIssuerCommands * credIssuerCmds, const char * helpText = nullptr) : Command(commandName, helpText), mCredIssuerCmds(credIssuerCmds) { - AddArgument( - "revocation-set-path", &mRevocationSetPath, - "Path to file holding a list of Revoked Certificates. Can be absolute or relative to the current working directory."); AddArgument("paa-trust-store-path", &mPaaTrustStorePath, "Path to directory holding PAA certificate information. Can be absolute or relative to the current working " "directory."); @@ -221,7 +218,6 @@ class CHIPCommand : public Command chip::Optional mCommissionerNodeId; chip::Optional mCommissionerVendorId; chip::Optional mBleAdapterId; - chip::Optional mRevocationSetPath; chip::Optional mPaaTrustStorePath; chip::Optional mCDTrustStorePath; chip::Optional mUseMaxSizedCerts; @@ -230,7 +226,6 @@ class CHIPCommand : public Command // Cached trust store so commands other than the original startup command // can spin up commissioners as needed. static const chip::Credentials::AttestationTrustStore * sTrustStore; - static const chip::Credentials::RevocationSet * sRevocationSet; static void RunQueuedCommand(intptr_t commandArg); typedef decltype(RunQueuedCommand) MatterWorkCallback; diff --git a/examples/chip-tool/commands/common/CredentialIssuerCommands.h b/examples/chip-tool/commands/common/CredentialIssuerCommands.h index fc67c12acc96c1..afb36773529e1e 100644 --- a/examples/chip-tool/commands/common/CredentialIssuerCommands.h +++ b/examples/chip-tool/commands/common/CredentialIssuerCommands.h @@ -59,8 +59,7 @@ class CredentialIssuerCommands * @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error code. */ virtual CHIP_ERROR SetupDeviceAttestation(chip::Controller::SetupParams & setupParams, - const chip::Credentials::AttestationTrustStore * trustStore, - const chip::Credentials::RevocationSet * revocationSet) = 0; + const chip::Credentials::AttestationTrustStore * trustStore) = 0; /** * @brief Add a list of additional non-default CD verifying keys (by certificate) diff --git a/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h b/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h index 07936e52820412..a23b45eae10627 100644 --- a/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h +++ b/examples/chip-tool/commands/example/ExampleCredentialIssuerCommands.h @@ -34,12 +34,11 @@ class ExampleCredentialIssuerCommands : public CredentialIssuerCommands return mOpCredsIssuer.Initialize(storage); } CHIP_ERROR SetupDeviceAttestation(chip::Controller::SetupParams & setupParams, - const chip::Credentials::AttestationTrustStore * trustStore, - const chip::Credentials::RevocationSet * revocationSet) override + const chip::Credentials::AttestationTrustStore * trustStore) override { chip::Credentials::SetDeviceAttestationCredentialsProvider(chip::Credentials::Examples::GetExampleDACProvider()); - mDacVerifier = chip::Credentials::GetDefaultDACVerifier(trustStore, revocationSet); + mDacVerifier = chip::Credentials::GetDefaultDACVerifier(trustStore); setupParams.deviceAttestationVerifier = mDacVerifier; mDacVerifier->EnableCdTestKeySupport(mAllowTestCdSigningKey); diff --git a/scripts/tools/check_includes_config.py b/scripts/tools/check_includes_config.py index 42fdaeef9b6637..26689e4a2f85b4 100644 --- a/scripts/tools/check_includes_config.py +++ b/scripts/tools/check_includes_config.py @@ -134,7 +134,6 @@ 'src/credentials/attestation_verifier/FileAttestationTrustStore.h': {'vector'}, 'src/credentials/attestation_verifier/FileAttestationTrustStore.cpp': {'string'}, - 'src/credentials/attestation_verifier/JSONFileRevocationSet.cpp': {'fstream'}, 'src/setup_payload/AdditionalDataPayload.h': {'string'}, 'src/setup_payload/AdditionalDataPayloadParser.cpp': {'vector'}, diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 557cdf43a6ad9c..7f6e4bcc0ac228 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -388,6 +388,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio case CommissioningStage::kSendAttestationRequest: return CommissioningStage::kAttestationVerification; case CommissioningStage::kAttestationVerification: + return CommissioningStage::kAttestationRevocationCheck; + case CommissioningStage::kAttestationRevocationCheck: return CommissioningStage::kSendOpCertSigningRequest; case CommissioningStage::kSendOpCertSigningRequest: return CommissioningStage::kValidateCSR; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 637e5debffae72..d20a54087def98 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -392,6 +392,7 @@ DeviceCommissioner::DeviceCommissioner() : mOnDeviceConnectionRetryCallback(OnDeviceConnectionRetryFn, this), #endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES mDeviceAttestationInformationVerificationCallback(OnDeviceAttestationInformationVerification, this), + mDACChainRevocationStatusVerificationCallback(OnDACChainRevocationStatusVerification, this), mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), mSetUpCodePairer(this) {} @@ -877,7 +878,8 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de return CHIP_ERROR_INCORRECT_STATE; } - if (mCommissioningStage != CommissioningStage::kAttestationVerification) + if (mCommissioningStage != CommissioningStage::kAttestationVerification && + mCommissioningStage != CommissioningStage::kAttestationRevocationCheck) { ChipLogError(Controller, "Commissioning is not attestation verification phase"); return CHIP_ERROR_INCORRECT_STATE; @@ -1146,6 +1148,61 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification( } } else + { + { + ChipLogProgress(Controller, "Successfully validated 'Attestation Information' command received from the device."); + commissioner->CommissioningStageComplete(CHIP_NO_ERROR); + } + } +} + +void DeviceCommissioner::OnDACChainRevocationStatusVerification( + void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, AttestationVerificationResult result) +{ + MATTER_TRACE_SCOPE("OnDACChainRevocationStatusVerification", "DeviceCommissioner"); + DeviceCommissioner * commissioner = reinterpret_cast(context); + + if (!commissioner->mDeviceBeingCommissioned) + { + ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device"); + return; + } + + auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters(); + Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate(); + + if (result != AttestationVerificationResult::kSuccess) + { + CommissioningDelegate::CommissioningReport report; + report.Set(result); + if (result == AttestationVerificationResult::kNotImplemented) + { + ChipLogError(Controller, + "Failed in verifying 'DAC Chain Revocation Status' command received from the device due to default " + "DeviceAttestationVerifier Class not being overridden by a real implementation."); + commissioner->CommissioningStageComplete(CHIP_ERROR_NOT_IMPLEMENTED, report); + return; + } + + ChipLogError(Controller, + "Failed in verifying 'DAC Chain Revocation Status' command received from the device: err %hu. Look at " + "AttestationVerificationResult enum to understand the errors", + static_cast(result)); + // Go look at AttestationVerificationResult enum in src/credentials/attestation_verifier/DeviceAttestationVerifier.h to + // understand the errors. + + // If a device attestation status delegate is installed, delegate handling of failure to the client and let them decide on + // whether to proceed further or not. + if (deviceAttestationDelegate) + { + commissioner->ExtendArmFailSafeForDeviceAttestation(info, result); + } + else + { + commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report); + } + } + else { if (deviceAttestationDelegate && deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation()) { @@ -1153,7 +1210,7 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification( } else { - ChipLogProgress(Controller, "Successfully validated 'Attestation Information' command received from the device."); + ChipLogProgress(Controller, "Successfully validated 'DAC Chain Revocation Status' command received from the device."); commissioner->CommissioningStageComplete(CHIP_NO_ERROR); } } @@ -1305,6 +1362,18 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device return CHIP_NO_ERROR; } +CHIP_ERROR +DeviceCommissioner::ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info) +{ + MATTER_TRACE_SCOPE("ValidateDACChainRevocationStatus", "DeviceCommissioner"); + VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE); + + mDeviceAttestationVerifier->ValidateDACChainRevocationStatus(info, &mDACChainRevocationStatusVerificationCallback); + + return CHIP_NO_ERROR; +} + CHIP_ERROR DeviceCommissioner::ValidateCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, const ByteSpan & AttestationSignature, const ByteSpan & dac, const ByteSpan & csrNonce) { @@ -2925,6 +2994,31 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } } break; + case CommissioningStage::kAttestationRevocationCheck: { + ChipLogProgress(Controller, "Verifying device's DAC chain revocation status"); + if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() || + !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || + !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) + { + ChipLogError(Controller, "Missing attestation certificates"); + CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); + return; + } + + DeviceAttestationVerifier::AttestationInfo info( + params.GetAttestationElements().Value(), + proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(), + params.GetAttestationSignature().Value(), params.GetPAI().Value(), params.GetDAC().Value(), + params.GetAttestationNonce().Value(), params.GetRemoteVendorId().Value(), params.GetRemoteProductId().Value()); + + if (ValidateDACChainRevocationStatus(info) != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Error validating device's DAC chain revocation status"); + CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); + return; + } + } + break; case CommissioningStage::kSendOpCertSigningRequest: { if (!params.GetCSRNonce().HasValue()) { diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index a635edb958a632..b425195e8a421f 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -604,6 +604,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, */ CHIP_ERROR ValidateAttestationInfo(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); + /** + * @brief + * This function validates the revocation status of the DAC Chain sent by the device. + * + * @param[in] info Structure contatining all the required information for validating the device attestation. + */ + CHIP_ERROR ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); + /** * @brief * Sends CommissioningStepComplete report to the commissioning delegate. Function will fill in current step. @@ -884,6 +892,9 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, static void OnDeviceAttestationInformationVerification(void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, Credentials::AttestationVerificationResult result); + static void OnDACChainRevocationStatusVerification(void * context, + const Credentials::DeviceAttestationVerifier::AttestationInfo & info, + Credentials::AttestationVerificationResult result); static void OnDeviceNOCChainGeneration(void * context, CHIP_ERROR status, const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac, Optional ipk, @@ -1017,6 +1028,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, chip::Callback::Callback mDeviceAttestationInformationVerificationCallback; + chip::Callback::Callback + mDACChainRevocationStatusVerificationCallback; chip::Callback::Callback mDeviceNOCChainCallback; SetUpCodePairer mSetUpCodePairer; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 2a78663541dd49..e7a6479941aee1 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -46,6 +46,7 @@ enum CommissioningStage : uint8_t kSendDACCertificateRequest, ///< Send DAC CertificateChainRequest (0x3E:2) command to the device kSendAttestationRequest, ///< Send AttestationRequest (0x3E:0) command to the device kAttestationVerification, ///< Verify AttestationResponse (0x3E:1) validity + kAttestationRevocationCheck, ///< Verify Revocation Status of device's DAC chain kSendOpCertSigningRequest, ///< Send CSRRequest (0x3E:4) command to the device kValidateCSR, ///< Verify CSRResponse (0x3E:5) validity kGenerateNOCChain, ///< TLV encode Node Operational Credentials (NOC) chain certs diff --git a/src/controller/python/BUILD.gn b/src/controller/python/BUILD.gn index b9bb947950b98c..5fc2212098fea8 100644 --- a/src/controller/python/BUILD.gn +++ b/src/controller/python/BUILD.gn @@ -136,7 +136,6 @@ shared_library("ChipDeviceCtrl") { public_deps += [ "${chip_root}/src/controller/data_model", "${chip_root}/src/credentials:file_attestation_trust_store", - "${chip_root}/src/credentials:json_file_revocation_set", "${chip_root}/src/lib/support:testing", "${chip_root}/src/tracing/json", "${chip_root}/src/tracing/perfetto", diff --git a/src/credentials/BUILD.gn b/src/credentials/BUILD.gn index b5ffe87511aa67..df7afc0c1025ba 100644 --- a/src/credentials/BUILD.gn +++ b/src/credentials/BUILD.gn @@ -185,18 +185,3 @@ static_library("file_attestation_trust_store") { "${nlassert_root}:nlassert", ] } - -static_library("json_file_revocation_set") { - output_name = "libJSONFileRevocationSet" - - sources = [ - "attestation_verifier/JSONFileRevocationSet.cpp", - "attestation_verifier/JSONFileRevocationSet.h", - ] - - public_deps = [ - ":credentials", - "${chip_root}/third_party/jsoncpp", - "${nlassert_root}:nlassert", - ] -} diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp index 7960e241b99ec1..54ea76f1553e94 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp @@ -458,20 +458,6 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, attestationError = attestationError); } - { - attestationError = IsCertificateRevoked(true, paaVidPid, paaDerBuffer); - VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, - attestationError = AttestationVerificationResult::kPaaRevoked); - - attestationError = IsCertificateRevoked(false, paiVidPid, info.paiDerBuffer); - VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, - attestationError = AttestationVerificationResult::kPaiRevoked); - - attestationError = IsCertificateRevoked(false, dacVidPid, info.dacDerBuffer); - VerifyOrExit(attestationError == AttestationVerificationResult::kSuccess, - attestationError = AttestationVerificationResult::kDacRevoked); - } - exit: onCompletion->mCall(onCompletion->mContext, info, attestationError); } @@ -621,33 +607,14 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa return CHIP_NO_ERROR; } -AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, - ByteSpan certificate) +void DefaultDACVerifier::ValidateDACChainRevocationStatus(const AttestationInfo & info, + Callback::Callback * onCompletion) { - uint8_t issuerBuf[kMaxCertificateDistinguishedNameLength] = { 0 }; - MutableByteSpan issuer(issuerBuf); - uint8_t akidBuf[kAuthorityKeyIdentifierLength]; - MutableByteSpan akid(akidBuf); - uint8_t serialNumberBuf[kMaxCertificateSerialNumberLength]; - MutableByteSpan serialNumber(serialNumberBuf); - - VerifyOrReturnError(ExtractIssuerFromX509Cert(certificate, issuer) == CHIP_NO_ERROR, - AttestationVerificationResult::kPaaFormatInvalid); - VerifyOrReturnError(ExtractAKIDFromX509Cert(certificate, akid) == CHIP_NO_ERROR, - AttestationVerificationResult::kPaaFormatInvalid); - VerifyOrReturnError(ExtractSerialNumberFromX509Cert(certificate, serialNumber) == CHIP_NO_ERROR, - AttestationVerificationResult::kPaaFormatInvalid); - - return IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, akid, serialNumber); -} + AttestationVerificationResult attestationError = AttestationVerificationResult::kSuccess; -AttestationVerificationResult DefaultDACVerifier::IsCertificateRevoked(bool isPaa, AttestationCertVidPid vidPidUnderTest, - ByteSpan issuer, ByteSpan authorityKeyId, - ByteSpan serialNumber) -{ - VerifyOrReturnError(mRevocationSet != nullptr, AttestationVerificationResult::kSuccess); + // TODO - return mRevocationSet->IsCertificateRevoked(isPaa, vidPidUnderTest, issuer, authorityKeyId, serialNumber); + onCompletion->mCall(onCompletion->mContext, info, attestationError); } bool CsaCdKeysTrustStore::IsCdTestKey(const ByteSpan & kid) const @@ -726,9 +693,9 @@ const AttestationTrustStore * GetTestAttestationTrustStore() return &gTestAttestationTrustStore.get(); } -DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet) +DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore) { - static DefaultDACVerifier defaultDACVerifier{ paaRootStore, revocationSet }; + static DefaultDACVerifier defaultDACVerifier{ paaRootStore }; return &defaultDACVerifier; } diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h index 99a9a6ed94430f..1f9015531a9b71 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h @@ -57,9 +57,7 @@ class CsaCdKeysTrustStore : public WellKnownKeysTrustStore class DefaultDACVerifier : public DeviceAttestationVerifier { public: - DefaultDACVerifier(const AttestationTrustStore * paaRootStore, const RevocationSet * revocationSet) : - mAttestationTrustStore(paaRootStore), mRevocationSet(revocationSet) - {} + DefaultDACVerifier(const AttestationTrustStore * paaRootStore) : mAttestationTrustStore(paaRootStore) {} void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info, Callback::Callback * onCompletion) override; @@ -76,8 +74,8 @@ class DefaultDACVerifier : public DeviceAttestationVerifier const ByteSpan & attestationSignatureBuffer, const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override; - AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, ByteSpan issuer, - ByteSpan authorityKeyId, ByteSpan serialNumber) override; + void ValidateDACChainRevocationStatus(const AttestationInfo & info, + Callback::Callback * onCompletion) override; CsaCdKeysTrustStore * GetCertificationDeclarationTrustStore() override { return &mCdKeysTrustStore; } @@ -86,10 +84,6 @@ class DefaultDACVerifier : public DeviceAttestationVerifier CsaCdKeysTrustStore mCdKeysTrustStore; const AttestationTrustStore * mAttestationTrustStore; - const RevocationSet * mRevocationSet; - - AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, - ByteSpan certificate); }; /** @@ -118,8 +112,7 @@ const AttestationTrustStore * GetTestAttestationTrustStore(); * process lifetime. In particular, after the first call it's not * possible to change which AttestationTrustStore is used by this verifier. */ -DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore, - const RevocationSet * revocationSet = nullptr); +DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore); } // namespace Credentials } // namespace chip diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp index e45628d7151976..c489109157dad5 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp @@ -70,10 +70,11 @@ class UnimplementedDACVerifier : public DeviceAttestationVerifier return CHIP_ERROR_NOT_IMPLEMENTED; } - AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, ByteSpan issuer, - ByteSpan authorityKeyId, ByteSpan serialNumber) override + void ValidateDACChainRevocationStatus(const AttestationInfo & info, + Callback::Callback * onCompletion) override { - return AttestationVerificationResult::kNotImplemented; + (void) info; + (void) onCompletion; } }; diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h index 54fc959f4e1d82..ec62b9b9d150c6 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h @@ -259,25 +259,6 @@ class ArrayAttestationTrustStore : public AttestationTrustStore const size_t mNumCerts; }; -/** - * @brief Helper utility to model a basic Revocation Set usable for device attestation verifiers. - * - */ -class RevocationSet -{ -public: - RevocationSet() = default; - virtual ~RevocationSet() = default; - - // Not copyable - RevocationSet(const RevocationSet &) = delete; - RevocationSet & operator=(const RevocationSet &) = delete; - - virtual AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, - ByteSpan issuer, ByteSpan authorityKeyId, - ByteSpan serialNumber) const = 0; -}; - class DeviceAttestationVerifier { public: @@ -405,8 +386,8 @@ class DeviceAttestationVerifier const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) = 0; - virtual AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, - ByteSpan issuer, ByteSpan authorityKeyId, ByteSpan serialNumber) = 0; + virtual void ValidateDACChainRevocationStatus(const AttestationInfo & info, + Callback::Callback * onCompletion) = 0; /** * @brief Get the trust store used for the attestation verifier. diff --git a/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp b/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp deleted file mode 100644 index e2fe70ce7b1c48..00000000000000 --- a/src/credentials/attestation_verifier/JSONFileRevocationSet.cpp +++ /dev/null @@ -1,173 +0,0 @@ -/* - * - * Copyright (c) 2022 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -#include "JSONFileRevocationSet.h" - -#include -#include -#include -#include - -#include -#include - -namespace chip { -namespace Credentials { - -JSONFileRevocationSet::JSONFileRevocationSet(const char * revocationSetPath) -{ - VerifyOrReturn(revocationSetPath != nullptr); - - if (revocationSetPath != nullptr) - { - LoadJSONRevocationSet(revocationSetPath, mRevocationSet); - VerifyOrReturn(revocationSetCount()); - } - - mIsInitialized = true; -} - -void LoadJSONRevocationSet(const char * revocationSetPath, Json::Value & revocationSet) -{ - VerifyOrReturn(revocationSetPath != nullptr); - - std::ifstream ifs(revocationSetPath); - Json::Reader reader; - reader.parse(ifs, revocationSet); -} - -AttestationVerificationResult JSONFileRevocationSet::IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, - ByteSpan issuer, ByteSpan authorityKeyId, - ByteSpan serialNumber) const -{ - for (int revocation_set_idx = 0; revocation_set_idx < static_cast(mRevocationSet.size()); ++revocation_set_idx) - { - Json::Value revocationSetEntry = mRevocationSet[revocation_set_idx]; - - // 1. - if (revocationSetEntry["type"].asString().compare("revocation_set") == 0) - { - Json::Value jsonCrlIssuerSubjectKeyId = revocationSetEntry["issuer_subject_key_id"]; - Json::Value jsonCrlIssuerName = revocationSetEntry["issuer_name"]; - Json::Value jsonCrlRevokedSerialNumbers = revocationSetEntry["revoked_serial_numbers"]; - - uint8_t crlIssuerSubjectKeyIdBuf[Crypto::kAuthorityKeyIdentifierLength] = { 0 }; - ByteSpan crlIssuerSubjectKeyId(crlIssuerSubjectKeyIdBuf); - - VerifyOrReturnError(Encoding::HexToBytes(jsonCrlIssuerSubjectKeyId.asString().c_str(), - jsonCrlIssuerSubjectKeyId.asString().size(), crlIssuerSubjectKeyIdBuf, - sizeof(crlIssuerSubjectKeyIdBuf)) == Crypto::kAuthorityKeyIdentifierLength, - AttestationVerificationResult::kInvalidArgument); - - size_t crlSignerCertificateMaxLength = BASE64_MAX_DECODED_LEN(jsonCrlIssuerName.asString().size()); - - Platform::ScopedMemoryBuffer crlSignerCertificate; - - ReturnErrorCodeIf(!crlSignerCertificate.Alloc(crlSignerCertificateMaxLength), AttestationVerificationResult::kNoMemory); - - // 2. - size_t crlSignerCertificateLength = - Base64Decode(jsonCrlIssuerName.asString().c_str(), static_cast(jsonCrlIssuerName.asString().size()), - crlSignerCertificate.Get()); - VerifyOrReturnError(crlSignerCertificateLength > 0 && crlSignerCertificateLength != UINT16_MAX, - AttestationVerificationResult::kInternalError); - - // 3. && 4. - Crypto::AttestationCertVidPid vidPid; - - Crypto::ExtractVIDPIDFromAttributeString( - Crypto::DNAttrType::kCommonName, ByteSpan(crlSignerCertificate.Get(), crlSignerCertificateLength), vidPid, vidPid); - - if (vidPid.mVendorId.HasValue() && vidPid.mVendorId.Value() != vidPidUnderTest.mVendorId.Value()) - { - // VID does not match. Stop further processing and continue to next entry. - continue; - } - - if (isPaa) - { - if (vidPid.mProductId.HasValue()) - { - // PAA must not contain PID entry. Format wrong. Continuing to next entry. - continue; - } - } - else - { - if (vidPid.mProductId.HasValue() && vidPid.mProductId.Value() != vidPidUnderTest.mProductId.Value()) - { - // PID does not match. Stop further processing and continue to next entry. - continue; - } - } - - // 7. Perform CRLFile validation - if (authorityKeyId.data_equal(crlIssuerSubjectKeyId) == false) - { - continue; - } - - // 9. && 10. - for (int serial_number_idx = 0; serial_number_idx < static_cast(jsonCrlRevokedSerialNumbers.size()); - ++serial_number_idx) - { - size_t revokedSerialNumberMaxLength = - BASE64_MAX_DECODED_LEN(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size()); - - Platform::ScopedMemoryBuffer revokedSerialNumber; - - ReturnErrorCodeIf(!revokedSerialNumber.Alloc(revokedSerialNumberMaxLength), - AttestationVerificationResult::kNoMemory); - - size_t revokedSerialNumberLength = - Base64Decode(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().c_str(), - static_cast(jsonCrlRevokedSerialNumbers[serial_number_idx].asString().size()), - revokedSerialNumber.Get()); - VerifyOrReturnError(revokedSerialNumberLength > 0 && revokedSerialNumberLength != UINT16_MAX, - AttestationVerificationResult::kInternalError); - - uint8_t crlRevokedSerialNumberBuf[Crypto::kMaxCertificateSerialNumberLength] = { 0 }; - ByteSpan crlSerialNumber(crlRevokedSerialNumberBuf); - - VerifyOrReturnError(Encoding::HexToBytes(Uint8::to_char(revokedSerialNumber.Get()), revokedSerialNumberLength, - crlRevokedSerialNumberBuf, - sizeof(crlRevokedSerialNumberBuf)) == sizeof(crlRevokedSerialNumberBuf), - AttestationVerificationResult::kInvalidArgument); - - if (serialNumber.data_equal(crlSerialNumber)) - { - return AttestationVerificationResult::kPaaRevoked; - } - } - } - } - - return AttestationVerificationResult::kSuccess; -} - -JSONFileRevocationSet::~JSONFileRevocationSet() -{ - Cleanup(); -} - -void JSONFileRevocationSet::Cleanup() -{ - mRevocationSet.clear(); - mIsInitialized = false; -} - -} // namespace Credentials -} // namespace chip diff --git a/src/credentials/attestation_verifier/JSONFileRevocationSet.h b/src/credentials/attestation_verifier/JSONFileRevocationSet.h deleted file mode 100644 index 5ebdd8b33d49df..00000000000000 --- a/src/credentials/attestation_verifier/JSONFileRevocationSet.h +++ /dev/null @@ -1,58 +0,0 @@ -/* - * - * Copyright (c) 2023 Project CHIP Authors - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -#pragma once - -#include - -#include - -namespace chip { -namespace Credentials { - -/** - * @brief Load a Revocation Set from a given path. - * - * Returns an empty vector if no files are found or unrecoverable errors arise. - * - * @param revocationSetPath - path from where to search for a revocation set. - * @return a vector of certificate DER data - */ -void LoadJSONRevocationSet(const char * revocationSetPath, Json::Value & revocationSet); - -class JSONFileRevocationSet : public RevocationSet -{ -public: - JSONFileRevocationSet(const char * revocationSetPath = nullptr); - ~JSONFileRevocationSet(); - - AttestationVerificationResult IsCertificateRevoked(bool isPaa, Crypto::AttestationCertVidPid vidPidUnderTest, ByteSpan issuer, - ByteSpan authorityKeyId, ByteSpan serialNumber) const override; - - bool IsInitialized() const { return mIsInitialized; } - size_t revocationSetCount() const { return mRevocationSet.size(); }; - -protected: - Json::Value mRevocationSet; - -private: - bool mIsInitialized = false; - - void Cleanup(); -}; - -} // namespace Credentials -} // namespace chip From f638fe245df51a8b73d7fddd9734bb07efc05936 Mon Sep 17 00:00:00 2001 From: Vijay Selvaraj Date: Tue, 23 Apr 2024 21:12:12 -0400 Subject: [PATCH 4/5] Implemented PR #31222 review comments --- docs/ERROR_CODES.md | 1 + src/controller/AutoCommissioner.cpp | 4 +- src/controller/AutoCommissioner.h | 4 +- src/controller/CHIPDeviceController.cpp | 127 ++++++++---------- src/controller/CHIPDeviceController.h | 16 ++- src/controller/CommissioningDelegate.h | 2 + .../DefaultDeviceAttestationVerifier.cpp | 6 +- .../DefaultDeviceAttestationVerifier.h | 4 +- .../DeviceAttestationVerifier.cpp | 5 +- .../DeviceAttestationVerifier.h | 11 +- src/lib/core/CHIPError.cpp | 3 + src/lib/core/CHIPError.h | 9 +- src/lib/core/tests/TestCHIPErrorStr.cpp | 1 + 13 files changed, 102 insertions(+), 91 deletions(-) diff --git a/docs/ERROR_CODES.md b/docs/ERROR_CODES.md index 122d188f8e0f70..bcef33e0c2affd 100644 --- a/docs/ERROR_CODES.md +++ b/docs/ERROR_CODES.md @@ -47,6 +47,7 @@ This file was **AUTOMATICALLY** generated by | 29 | 0x1D | `CHIP_ERROR_INVALID_IPK` | | 30 | 0x1E | `CHIP_ERROR_INVALID_STRING_LENGTH` | | 31 | 0x1F | `CHIP_ERROR_INVALID_LIST_LENGTH` | +| 32 | 0x20 | `CHIP_ERROR_FAILED_DEVICE_ATTESTATION` | | 33 | 0x21 | `CHIP_ERROR_END_OF_TLV` | | 34 | 0x22 | `CHIP_ERROR_TLV_UNDERRUN` | | 35 | 0x23 | `CHIP_ERROR_INVALID_TLV_ELEMENT` | diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 7f6e4bcc0ac228..5ddb0be88feedf 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -388,7 +388,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio case CommissioningStage::kSendAttestationRequest: return CommissioningStage::kAttestationVerification; case CommissioningStage::kAttestationVerification: - return CommissioningStage::kAttestationRevocationCheck; + return mBypassDeviceAttestation ? CommissioningStage::kSendOpCertSigningRequest + : CommissioningStage::kAttestationRevocationCheck; case CommissioningStage::kAttestationRevocationCheck: return CommissioningStage::kSendOpCertSigningRequest; case CommissioningStage::kSendOpCertSigningRequest: @@ -577,6 +578,7 @@ CHIP_ERROR AutoCommissioner::StartCommissioning(DeviceCommissioner * commissione return CHIP_ERROR_INVALID_ARGUMENT; } mStopCommissioning = false; + mBypassDeviceAttestation = false; mCommissioner = commissioner; mCommissioneeDeviceProxy = proxy; mNeedsNetworkSetup = diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index e311b72d190b86..a14f84e3a65782 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -37,6 +37,7 @@ class AutoCommissioner : public CommissioningDelegate CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) override; void StopCommissioning() { mStopCommissioning = true; }; + void BypassDeviceAttestation() override { mBypassDeviceAttestation = true; } CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override; @@ -92,7 +93,8 @@ class AutoCommissioner : public CommissioningDelegate mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)); }; - bool mStopCommissioning = false; + bool mStopCommissioning = false; + bool mBypassDeviceAttestation = false; DeviceCommissioner * mCommissioner = nullptr; CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index d20a54087def98..edc0a1c5a0c59a 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -392,8 +392,8 @@ DeviceCommissioner::DeviceCommissioner() : mOnDeviceConnectionRetryCallback(OnDeviceConnectionRetryFn, this), #endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES mDeviceAttestationInformationVerificationCallback(OnDeviceAttestationInformationVerification, this), - mDACChainRevocationStatusVerificationCallback(OnDACChainRevocationStatusVerification, this), - mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), mSetUpCodePairer(this) + mDACChainRevocationStatusCallback(OnDACChainRevocationStatus, this), mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), + mSetUpCodePairer(this) {} CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params) @@ -888,6 +888,8 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de ChipLogProgress(Controller, "Continuing commissioning after attestation failure for device ID 0x" ChipLogFormatX64, ChipLogValueX64(commissioneeDevice->GetDeviceId())); + mCommissioningDelegate->BypassDeviceAttestation(); + if (attestationResult != AttestationVerificationResult::kSuccess) { ChipLogError(Controller, "Client selected error: %u for failed 'Attestation Information' for device", @@ -1107,6 +1109,22 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification( MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner"); DeviceCommissioner * commissioner = reinterpret_cast(context); + if (commissioner->attestationInformationVerificationResult == AttestationVerificationResult::kNotImplemented) + { + commissioner->attestationInformationVerificationResult = result; + } + + if (commissioner->attestationInformationVerificationResult == AttestationVerificationResult::kSuccess && + commissioner->dacChainRevocationStatusResult == AttestationVerificationResult::kNotImplemented) + { + // Check for revoked DAC Chain before calling delegate. Enter next stage. + return commissioner->CommissioningStageComplete(CHIP_NO_ERROR); + } + + result = commissioner->attestationInformationVerificationResult != AttestationVerificationResult::kSuccess + ? commissioner->attestationInformationVerificationResult + : commissioner->dacChainRevocationStatusResult; + if (!commissioner->mDeviceBeingCommissioned) { ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device"); @@ -1156,64 +1174,21 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification( } } -void DeviceCommissioner::OnDACChainRevocationStatusVerification( - void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, AttestationVerificationResult result) +void DeviceCommissioner::OnDACChainRevocationStatus(void * context, + const Credentials::DeviceAttestationVerifier::AttestationInfo & info, + AttestationVerificationResult result) { - MATTER_TRACE_SCOPE("OnDACChainRevocationStatusVerification", "DeviceCommissioner"); + MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner"); DeviceCommissioner * commissioner = reinterpret_cast(context); - if (!commissioner->mDeviceBeingCommissioned) + if (commissioner->dacChainRevocationStatusResult == AttestationVerificationResult::kNotImplemented) { - ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device"); - return; + commissioner->dacChainRevocationStatusResult = result; } - auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters(); - Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate(); - - if (result != AttestationVerificationResult::kSuccess) - { - CommissioningDelegate::CommissioningReport report; - report.Set(result); - if (result == AttestationVerificationResult::kNotImplemented) - { - ChipLogError(Controller, - "Failed in verifying 'DAC Chain Revocation Status' command received from the device due to default " - "DeviceAttestationVerifier Class not being overridden by a real implementation."); - commissioner->CommissioningStageComplete(CHIP_ERROR_NOT_IMPLEMENTED, report); - return; - } - - ChipLogError(Controller, - "Failed in verifying 'DAC Chain Revocation Status' command received from the device: err %hu. Look at " - "AttestationVerificationResult enum to understand the errors", - static_cast(result)); - // Go look at AttestationVerificationResult enum in src/credentials/attestation_verifier/DeviceAttestationVerifier.h to - // understand the errors. + VerifyOrReturn(commissioner->attestationInformationVerificationResult != AttestationVerificationResult::kNotImplemented); - // If a device attestation status delegate is installed, delegate handling of failure to the client and let them decide on - // whether to proceed further or not. - if (deviceAttestationDelegate) - { - commissioner->ExtendArmFailSafeForDeviceAttestation(info, result); - } - else - { - commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report); - } - } - else - { - if (deviceAttestationDelegate && deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation()) - { - commissioner->ExtendArmFailSafeForDeviceAttestation(info, result); - } - else - { - ChipLogProgress(Controller, "Successfully validated 'DAC Chain Revocation Status' command received from the device."); - commissioner->CommissioningStageComplete(CHIP_NO_ERROR); - } - } + OnDeviceAttestationInformationVerification(context, info, commissioner->dacChainRevocationStatusResult); } void DeviceCommissioner::OnArmFailSafeExtendedForDeviceAttestation( @@ -1363,13 +1338,13 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device } CHIP_ERROR -DeviceCommissioner::ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info) +DeviceCommissioner::CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info) { - MATTER_TRACE_SCOPE("ValidateDACChainRevocationStatus", "DeviceCommissioner"); + MATTER_TRACE_SCOPE("CheckForRevokedDACChain", "DeviceCommissioner"); VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE); - mDeviceAttestationVerifier->ValidateDACChainRevocationStatus(info, &mDACChainRevocationStatusVerificationCallback); + mDeviceAttestationVerifier->CheckForRevokedDACChain(info, &mDACChainRevocationStatusCallback); return CHIP_NO_ERROR; } @@ -2971,14 +2946,11 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } case CommissioningStage::kAttestationVerification: { ChipLogProgress(Controller, "Verifying attestation"); - if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() || - !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || - !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) - { - ChipLogError(Controller, "Missing attestation information"); - CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); - return; - } + VerifyOrReturn(IsAttestationInformationMissing(params) == false); + + // Reset results before verifying + attestationInformationVerificationResult = AttestationVerificationResult::kNotImplemented; + dacChainRevocationStatusResult = AttestationVerificationResult::kNotImplemented; DeviceAttestationVerifier::AttestationInfo info( params.GetAttestationElements().Value(), @@ -2996,14 +2968,7 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio break; case CommissioningStage::kAttestationRevocationCheck: { ChipLogProgress(Controller, "Verifying device's DAC chain revocation status"); - if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() || - !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || - !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) - { - ChipLogError(Controller, "Missing attestation certificates"); - CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); - return; - } + VerifyOrReturn(IsAttestationInformationMissing(params) == false); DeviceAttestationVerifier::AttestationInfo info( params.GetAttestationElements().Value(), @@ -3011,10 +2976,10 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio params.GetAttestationSignature().Value(), params.GetPAI().Value(), params.GetDAC().Value(), params.GetAttestationNonce().Value(), params.GetRemoteVendorId().Value(), params.GetRemoteProductId().Value()); - if (ValidateDACChainRevocationStatus(info) != CHIP_NO_ERROR) + if (CheckForRevokedDACChain(info) != CHIP_NO_ERROR) { ChipLogError(Controller, "Error validating device's DAC chain revocation status"); - CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); + CommissioningStageComplete(CHIP_ERROR_FAILED_DEVICE_ATTESTATION); return; } } @@ -3359,6 +3324,20 @@ void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, } } +bool DeviceCommissioner::IsAttestationInformationMissing(CommissioningParameters & params) +{ + if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() || + !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || + !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) + { + ChipLogError(Controller, "Missing attestation information"); + CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); + return true; + } + + return false; +} + CHIP_ERROR DeviceController::GetCompressedFabricIdBytes(MutableByteSpan & outBytes) const { const auto * fabricInfo = GetFabricInfo(); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index b425195e8a421f..90b0e2a02ece68 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -610,7 +610,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, * * @param[in] info Structure contatining all the required information for validating the device attestation. */ - CHIP_ERROR ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); + CHIP_ERROR CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); /** * @brief @@ -792,6 +792,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, ObjectPool mCommissioneeDevicePool; + Credentials::AttestationVerificationResult attestationInformationVerificationResult = + Credentials::AttestationVerificationResult::kNotImplemented; + Credentials::AttestationVerificationResult dacChainRevocationStatusResult = + Credentials::AttestationVerificationResult::kNotImplemented; + #if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable UserDirectedCommissioningServer * mUdcServer = nullptr; // mUdcTransportMgr is for insecure communication (ex. user directed commissioning) @@ -892,9 +897,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, static void OnDeviceAttestationInformationVerification(void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, Credentials::AttestationVerificationResult result); - static void OnDACChainRevocationStatusVerification(void * context, - const Credentials::DeviceAttestationVerifier::AttestationInfo & info, - Credentials::AttestationVerificationResult result); + static void OnDACChainRevocationStatus(void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, + Credentials::AttestationVerificationResult result); static void OnDeviceNOCChainGeneration(void * context, CHIP_ERROR status, const ByteSpan & noc, const ByteSpan & icac, const ByteSpan & rcac, Optional ipk, @@ -1020,6 +1024,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, // extend it). void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step); + bool IsAttestationInformationMissing(CommissioningParameters & params); + chip::Callback::Callback mOnDeviceConnectedCallback; chip::Callback::Callback mOnDeviceConnectionFailureCallback; #if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES @@ -1029,7 +1035,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, chip::Callback::Callback mDeviceAttestationInformationVerificationCallback; chip::Callback::Callback - mDACChainRevocationStatusVerificationCallback; + mDACChainRevocationStatusCallback; chip::Callback::Callback mDeviceNOCChainCallback; SetUpCodePairer mSetUpCodePairer; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index e7a6479941aee1..ec3fe4aabfd6f7 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -761,6 +761,7 @@ class CommissioningDelegate * kSendDACCertificateRequest: RequestedCertificate * kSendAttestationRequest: AttestationResponse * kAttestationVerification: AttestationErrorInfo if there is an error + * kAttestationRevocationCheck: AttestationErrorInfo if there is an error * kSendOpCertSigningRequest: CSRResponse * kGenerateNOCChain: NocChain * kSendTrustedRootCert: None @@ -786,6 +787,7 @@ class CommissioningDelegate virtual void SetOperationalCredentialsDelegate(OperationalCredentialsDelegate * operationalCredentialsDelegate) = 0; virtual CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) = 0; virtual CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningReport report) = 0; + virtual void BypassDeviceAttestation() = 0; }; } // namespace Controller diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp index 54ea76f1553e94..a36feda965563a 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp @@ -607,12 +607,12 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa return CHIP_NO_ERROR; } -void DefaultDACVerifier::ValidateDACChainRevocationStatus(const AttestationInfo & info, - Callback::Callback * onCompletion) +void DefaultDACVerifier::CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) { AttestationVerificationResult attestationError = AttestationVerificationResult::kSuccess; - // TODO + // TODO(#xyz): Implement default version of CheckForRevokedDACChain onCompletion->mCall(onCompletion->mContext, info, attestationError); } diff --git a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h index 1f9015531a9b71..346d098a58a337 100644 --- a/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h @@ -74,8 +74,8 @@ class DefaultDACVerifier : public DeviceAttestationVerifier const ByteSpan & attestationSignatureBuffer, const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override; - void ValidateDACChainRevocationStatus(const AttestationInfo & info, - Callback::Callback * onCompletion) override; + void CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) override; CsaCdKeysTrustStore * GetCertificationDeclarationTrustStore() override { return &mCdKeysTrustStore; } diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp index c489109157dad5..5bdbb9a8d82792 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp @@ -70,11 +70,12 @@ class UnimplementedDACVerifier : public DeviceAttestationVerifier return CHIP_ERROR_NOT_IMPLEMENTED; } - void ValidateDACChainRevocationStatus(const AttestationInfo & info, - Callback::Callback * onCompletion) override + void CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) override { (void) info; (void) onCompletion; + VerifyOrDie(false); } }; diff --git a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h index ec62b9b9d150c6..f45ceae06c23fe 100644 --- a/src/credentials/attestation_verifier/DeviceAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DeviceAttestationVerifier.h @@ -386,8 +386,15 @@ class DeviceAttestationVerifier const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) = 0; - virtual void ValidateDACChainRevocationStatus(const AttestationInfo & info, - Callback::Callback * onCompletion) = 0; + /** + * @brief Verify whether or not the given DAC chain is revoked. + * + * @param[in] info All of the information required to check for revoked DAC chain. + * @param[in] onCompletion Callback handler to provide Attestation Information Verification result to the caller of + * CheckForRevokedDACChain() + */ + virtual void CheckForRevokedDACChain(const AttestationInfo & info, + Callback::Callback * onCompletion) = 0; /** * @brief Get the trust store used for the attestation verifier. diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp index 68a44d8c39a7ea..2b2461ca79d09c 100644 --- a/src/lib/core/CHIPError.cpp +++ b/src/lib/core/CHIPError.cpp @@ -146,6 +146,9 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err) case CHIP_ERROR_INVALID_LIST_LENGTH.AsInteger(): desc = "Invalid list length"; break; + case CHIP_ERROR_FAILED_DEVICE_ATTESTATION.AsInteger(): + desc = "Failed Device Attestation"; + break; case CHIP_END_OF_TLV.AsInteger(): desc = "End of TLV"; break; diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 38b76bca539c2a..9a9e6729d37e9e 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -707,7 +707,14 @@ using CHIP_ERROR = ::chip::ChipError; */ #define CHIP_ERROR_INVALID_LIST_LENGTH CHIP_CORE_ERROR(0x1f) -// AVAILABLE: 0x20 +/** + * @def CHIP_ERROR_FAILED_DEVICE_ATTESTATION + * + * @brief + * Device Attestation failed. + * + */ +#define CHIP_ERROR_FAILED_DEVICE_ATTESTATION CHIP_CORE_ERROR(0x20) /** * @def CHIP_END_OF_TLV diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp index 8f21a2e9ec3397..3620d4e255a481 100644 --- a/src/lib/core/tests/TestCHIPErrorStr.cpp +++ b/src/lib/core/tests/TestCHIPErrorStr.cpp @@ -71,6 +71,7 @@ static const CHIP_ERROR kTestElements[] = CHIP_ERROR_UNINITIALIZED, CHIP_ERROR_INVALID_STRING_LENGTH, CHIP_ERROR_INVALID_LIST_LENGTH, + CHIP_ERROR_FAILED_DEVICE_ATTESTATION, CHIP_END_OF_TLV, CHIP_ERROR_TLV_UNDERRUN, CHIP_ERROR_INVALID_TLV_ELEMENT, From f79903e11f0b886d9e85b9231aa8f3ed628f7fed Mon Sep 17 00:00:00 2001 From: Vijay Selvaraj Date: Tue, 7 May 2024 11:15:55 -0400 Subject: [PATCH 5/5] Fixed issues in revocation check and code cleanup --- src/controller/AutoCommissioner.cpp | 7 ++-- src/controller/AutoCommissioner.h | 4 +-- src/controller/CHIPDeviceController.cpp | 33 ++++++++++++------- src/controller/CHIPDeviceController.h | 18 +++++----- src/controller/CommissioningDelegate.h | 1 - src/controller/python/OpCredsBinding.cpp | 2 ++ .../commissioning_failure_test.py | 4 +-- src/python_testing/TC_CGEN_2_4.py | 6 ++-- 8 files changed, 42 insertions(+), 33 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 41ece6b2429dbc..5b37988a491260 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -389,8 +389,7 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio case CommissioningStage::kSendAttestationRequest: return CommissioningStage::kAttestationVerification; case CommissioningStage::kAttestationVerification: - return mBypassDeviceAttestation ? CommissioningStage::kSendOpCertSigningRequest - : CommissioningStage::kAttestationRevocationCheck; + return CommissioningStage::kAttestationRevocationCheck; case CommissioningStage::kAttestationRevocationCheck: return CommissioningStage::kSendOpCertSigningRequest; case CommissioningStage::kSendOpCertSigningRequest: @@ -582,7 +581,6 @@ CHIP_ERROR AutoCommissioner::StartCommissioning(DeviceCommissioner * commissione return CHIP_ERROR_INVALID_ARGUMENT; } mStopCommissioning = false; - mBypassDeviceAttestation = false; mCommissioner = commissioner; mCommissioneeDeviceProxy = proxy; mNeedsNetworkSetup = @@ -697,7 +695,8 @@ CHIP_ERROR AutoCommissioner::CommissioningStepFinished(CHIP_ERROR err, Commissio "Failed device attestation. Device vendor and/or product ID do not match the IDs expected. " "Verify DAC certificate chain and certification declaration to ensure spec rules followed."); } - else if (report.stageCompleted == CommissioningStage::kAttestationVerification) + + if (report.stageCompleted == CommissioningStage::kAttestationVerification) { ChipLogError(Controller, "Failed verifying attestation information. Now checking DAC chain revoked status."); // don't error out until we check for DAC chain revocation status diff --git a/src/controller/AutoCommissioner.h b/src/controller/AutoCommissioner.h index 653c924fac172c..3399e2776946de 100644 --- a/src/controller/AutoCommissioner.h +++ b/src/controller/AutoCommissioner.h @@ -38,7 +38,6 @@ class AutoCommissioner : public CommissioningDelegate CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) override; void StopCommissioning() { mStopCommissioning = true; }; - void BypassDeviceAttestation() override { mBypassDeviceAttestation = true; } CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningDelegate::CommissioningReport report) override; @@ -95,8 +94,7 @@ class AutoCommissioner : public CommissioningDelegate mDeviceCommissioningInfo.network.thread.endpoint != kInvalidEndpointId)); }; - bool mStopCommissioning = false; - bool mBypassDeviceAttestation = false; + bool mStopCommissioning = false; DeviceCommissioner * mCommissioner = nullptr; CommissioneeDeviceProxy * mCommissioneeDeviceProxy = nullptr; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index f5dc91edcacad1..910c0b21040c96 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -947,8 +947,7 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de return CHIP_ERROR_INCORRECT_STATE; } - if (mCommissioningStage != CommissioningStage::kAttestationVerification && - mCommissioningStage != CommissioningStage::kAttestationRevocationCheck) + if (mCommissioningStage != CommissioningStage::kAttestationRevocationCheck) { ChipLogError(Controller, "Commissioning is not attestation verification phase"); return CHIP_ERROR_INCORRECT_STATE; @@ -957,8 +956,6 @@ DeviceCommissioner::ContinueCommissioningAfterDeviceAttestation(DeviceProxy * de ChipLogProgress(Controller, "Continuing commissioning after attestation failure for device ID 0x" ChipLogFormatX64, ChipLogValueX64(commissioneeDevice->GetDeviceId())); - mCommissioningDelegate->BypassDeviceAttestation(); - if (attestationResult != AttestationVerificationResult::kSuccess) { ChipLogError(Controller, "Client selected error: %u for failed 'Attestation Information' for device", @@ -1198,8 +1195,14 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification( auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters(); Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate(); - result = - params.GetCompletionStatus().attestationResult.HasValue() ? params.GetCompletionStatus().attestationResult.Value() : result; + if (params.GetCompletionStatus().attestationResult.HasValue()) + { + auto previousResult = params.GetCompletionStatus().attestationResult.Value(); + if (previousResult != AttestationVerificationResult::kSuccess) + { + result = previousResult; + } + } if (result != AttestationVerificationResult::kSuccess) { @@ -3066,7 +3069,12 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } case CommissioningStage::kAttestationVerification: { ChipLogProgress(Controller, "Verifying attestation"); - VerifyOrReturn(IsAttestationInformationMissing(params) == false); + if (IsAttestationInformationMissing(params)) + { + ChipLogError(Controller, "Missing attestation information"); + CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); + return; + } DeviceAttestationVerifier::AttestationInfo info( params.GetAttestationElements().Value(), @@ -3084,7 +3092,12 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio break; case CommissioningStage::kAttestationRevocationCheck: { ChipLogProgress(Controller, "Verifying device's DAC chain revocation status"); - VerifyOrReturn(IsAttestationInformationMissing(params) == false); + if (IsAttestationInformationMissing(params)) + { + ChipLogError(Controller, "Missing attestation information"); + CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); + return; + } DeviceAttestationVerifier::AttestationInfo info( params.GetAttestationElements().Value(), @@ -3464,14 +3477,12 @@ void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, } } -bool DeviceCommissioner::IsAttestationInformationMissing(CommissioningParameters & params) +bool DeviceCommissioner::IsAttestationInformationMissing(const CommissioningParameters & params) { if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() || !params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || !params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue()) { - ChipLogError(Controller, "Missing attestation information"); - CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); return true; } diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 873894267ec25c..8bef525e9cd773 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -640,14 +640,6 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, */ CHIP_ERROR ValidateAttestationInfo(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); - /** - * @brief - * This function validates the revocation status of the DAC Chain sent by the device. - * - * @param[in] info Structure contatining all the required information for validating the device attestation. - */ - CHIP_ERROR CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); - /** * @brief * Sends CommissioningStepComplete report to the commissioning delegate. Function will fill in current step. @@ -1008,6 +1000,14 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, */ CHIP_ERROR ProcessCertificateChain(const ByteSpan & certificate); + /** + * @brief + * This function validates the revocation status of the DAC Chain sent by the device. + * + * @param[in] info Structure contatining all the required information for validating the device attestation. + */ + CHIP_ERROR CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info); + void HandleAttestationResult(CHIP_ERROR err); CommissioneeDeviceProxy * FindCommissioneeDevice(NodeId id); @@ -1061,7 +1061,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, // extend it). void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step); - bool IsAttestationInformationMissing(CommissioningParameters & params); + bool IsAttestationInformationMissing(const CommissioningParameters & params); chip::Callback::Callback mOnDeviceConnectedCallback; chip::Callback::Callback mOnDeviceConnectionFailureCallback; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 721a15130a69e6..e79ad1d67c4bcf 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -812,7 +812,6 @@ class CommissioningDelegate virtual void SetOperationalCredentialsDelegate(OperationalCredentialsDelegate * operationalCredentialsDelegate) = 0; virtual CHIP_ERROR StartCommissioning(DeviceCommissioner * commissioner, CommissioneeDeviceProxy * proxy) = 0; virtual CHIP_ERROR CommissioningStepFinished(CHIP_ERROR err, CommissioningReport report) = 0; - virtual void BypassDeviceAttestation() = 0; }; } // namespace Controller diff --git a/src/controller/python/OpCredsBinding.cpp b/src/controller/python/OpCredsBinding.cpp index 66a6c841843cde..5fd4205d4c7ce7 100644 --- a/src/controller/python/OpCredsBinding.cpp +++ b/src/controller/python/OpCredsBinding.cpp @@ -331,6 +331,8 @@ class TestCommissioner : public chip::Controller::AutoCommissioner return mNeedsDST && mParams.GetDSTOffsets().HasValue(); case chip::Controller::CommissioningStage::kError: case chip::Controller::CommissioningStage::kSecurePairing: + // "not valid" because attestation verification always fails after entering revocation check step + case chip::Controller::CommissioningStage::kAttestationVerification: return false; default: return true; diff --git a/src/controller/python/test/test_scripts/commissioning_failure_test.py b/src/controller/python/test/test_scripts/commissioning_failure_test.py index 4681dd108d758a..eca170601c7f29 100755 --- a/src/controller/python/test/test_scripts/commissioning_failure_test.py +++ b/src/controller/python/test/test_scripts/commissioning_failure_test.py @@ -96,7 +96,7 @@ def main(): # TODO: Start at stage 2 once handling for arming failsafe on pase is done. if options.report: - for testFailureStage in range(3, 20): + for testFailureStage in range(3, 21): FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1, setuppin=20202021, nodeid=1), @@ -105,7 +105,7 @@ def main(): "Commissioning failure tests failed for simulated report failure on stage {}".format(testFailureStage)) else: - for testFailureStage in range(3, 20): + for testFailureStage in range(3, 21): FailIfNot(test.TestPaseOnly(ip=options.deviceAddress1, setuppin=20202021, nodeid=1), diff --git a/src/python_testing/TC_CGEN_2_4.py b/src/python_testing/TC_CGEN_2_4.py index 23ab28ef09fad9..7bb2e6d21d3d73 100644 --- a/src/python_testing/TC_CGEN_2_4.py +++ b/src/python_testing/TC_CGEN_2_4.py @@ -34,9 +34,9 @@ kSendPAICertificateRequest = 10 kSendDACCertificateRequest = 11 kSendAttestationRequest = 12 -kSendOpCertSigningRequest = 14 -kSendTrustedRootCert = 17 -kSendNOC = 18 +kSendOpCertSigningRequest = 15 +kSendTrustedRootCert = 18 +kSendNOC = 19 class TC_CGEN_2_4(MatterBaseTest):