From 6194d6b68076df31a9872c3280b94fb30c874779 Mon Sep 17 00:00:00 2001 From: C Freeman Date: Mon, 4 Apr 2022 08:23:45 -0400 Subject: [PATCH] Validate CSR in separate commissioning step (#16913) * Vaidate CSR as a separate step * Pass all validataion parameters to GenerateNOCChain This will let OperationalCredentialsIssuer impls validate the CSR before generating the NOC chain. * Restyled by clang-format * Update src/controller/ExampleOperationalCredentialsIssuer.cpp Co-authored-by: Boris Zbarsky * Remove PAA from GenerateNOCChain command It's not available. * More const is better const No one's messing around with these things, may as well. * Pass PAI to noc chain generation. * Remove stale comment * Bring back namespasing. I accidentally removed it. * Once more. * And again. I hate myself. Co-authored-by: Restyled.io Co-authored-by: Boris Zbarsky --- src/controller/AutoCommissioner.cpp | 2 + src/controller/CHIPDeviceController.cpp | 55 +++++++++++++++---- src/controller/CHIPDeviceController.h | 19 ++++++- src/controller/CommissioningDelegate.cpp | 4 ++ src/controller/CommissioningDelegate.h | 1 + .../ExampleOperationalCredentialsIssuer.cpp | 11 +++- .../ExampleOperationalCredentialsIssuer.h | 4 +- .../OperationalCredentialsDelegate.h | 10 ++-- .../AndroidOperationalCredentialsIssuer.cpp | 7 ++- .../AndroidOperationalCredentialsIssuer.h | 4 +- src/controller/python/OpCredsBinding.cpp | 7 ++- .../CHIP/CHIPOperationalCredentialsDelegate.h | 6 +- .../CHIPOperationalCredentialsDelegate.mm | 6 +- 13 files changed, 100 insertions(+), 36 deletions(-) diff --git a/src/controller/AutoCommissioner.cpp b/src/controller/AutoCommissioner.cpp index 763d92939a8ad8..61b502c1b5cf03 100644 --- a/src/controller/AutoCommissioner.cpp +++ b/src/controller/AutoCommissioner.cpp @@ -135,6 +135,8 @@ CommissioningStage AutoCommissioner::GetNextCommissioningStageInternal(Commissio case CommissioningStage::kAttestationVerification: return CommissioningStage::kSendOpCertSigningRequest; case CommissioningStage::kSendOpCertSigningRequest: + return CommissioningStage::kValidateCSR; + case CommissioningStage::kValidateCSR: return CommissioningStage::kGenerateNOCChain; case CommissioningStage::kGenerateNOCChain: return CommissioningStage::kSendTrustedRootCert; diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 9dd4abbae67d81..0283ec7b0c7cde 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -969,6 +969,26 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device return CHIP_NO_ERROR; } +CHIP_ERROR DeviceCommissioner::ValidateCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, + const ByteSpan & AttestationSignature, const ByteSpan & dac, const ByteSpan & csrNonce) +{ + MATTER_TRACE_EVENT_SCOPE("ValidateCSR", "DeviceCommissioner"); + VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); + + DeviceAttestationVerifier * dacVerifier = GetDeviceAttestationVerifier(); + + P256PublicKey dacPubkey; + ReturnErrorOnFailure(ExtractPubkeyFromX509Cert(dac, dacPubkey)); + + // Retrieve attestation challenge + ByteSpan attestationChallenge = + proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(); + + // The operational CA should also verify this on its end during NOC generation, if end-to-end attestation is desired. + return dacVerifier->VerifyNodeOperationalCSRInformation(NOCSRElements, attestationChallenge, AttestationSignature, dacPubkey, + csrNonce); +} + CHIP_ERROR DeviceCommissioner::SendOperationalCertificateSigningRequestCommand(DeviceProxy * device, const ByteSpan & csrNonce) { MATTER_TRACE_EVENT_SCOPE("SendOperationalCertificateSigningRequestCommand", "DeviceCommissioner"); @@ -1038,15 +1058,14 @@ void DeviceCommissioner::OnDeviceNOCChainGeneration(void * context, CHIP_ERROR s } CHIP_ERROR DeviceCommissioner::ProcessCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, - const ByteSpan & AttestationSignature, ByteSpan dac, ByteSpan csrNonce) + const ByteSpan & AttestationSignature, const ByteSpan & dac, const ByteSpan & pai, + const ByteSpan & csrNonce) { MATTER_TRACE_EVENT_SCOPE("ProcessOpCSR", "DeviceCommissioner"); VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE); ChipLogProgress(Controller, "Getting certificate chain for the device from the issuer"); - DeviceAttestationVerifier * dacVerifier = GetDeviceAttestationVerifier(); - P256PublicKey dacPubkey; ReturnErrorOnFailure(ExtractPubkeyFromX509Cert(dac, dacPubkey)); @@ -1054,10 +1073,6 @@ CHIP_ERROR DeviceCommissioner::ProcessCSR(DeviceProxy * proxy, const ByteSpan & ByteSpan attestationChallenge = proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(); - // The operational CA should also verify this on its end during NOC generation, if end-to-end attestation is desired. - ReturnErrorOnFailure(dacVerifier->VerifyNodeOperationalCSRInformation(NOCSRElements, attestationChallenge, AttestationSignature, - dacPubkey, csrNonce)); - mOperationalCredentialsDelegate->SetNodeIdForNextNOCRequest(proxy->GetDeviceId()); if (mFabricInfo != nullptr) @@ -1065,8 +1080,8 @@ CHIP_ERROR DeviceCommissioner::ProcessCSR(DeviceProxy * proxy, const ByteSpan & mOperationalCredentialsDelegate->SetFabricIdForNextNOCRequest(mFabricInfo->GetFabricId()); } - return mOperationalCredentialsDelegate->GenerateNOCChain(NOCSRElements, AttestationSignature, dac, ByteSpan(), ByteSpan(), - &mDeviceNOCChainCallback); + return mOperationalCredentialsDelegate->GenerateNOCChain(NOCSRElements, csrNonce, AttestationSignature, attestationChallenge, + dac, pai, &mDeviceNOCChainCallback); } CHIP_ERROR DeviceCommissioner::SendOperationalCertificate(DeviceProxy * device, const ByteSpan & nocCertBuf, @@ -1776,15 +1791,33 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio } SendOperationalCertificateSigningRequestCommand(proxy, params.GetCSRNonce().Value()); break; - case CommissioningStage::kGenerateNOCChain: { + case CommissioningStage::kValidateCSR: { if (!params.GetNOCChainGenerationParameters().HasValue() || !params.GetDAC().HasValue() || !params.GetCSRNonce().HasValue()) + { + ChipLogError(Controller, "Unable to validate CSR"); + return CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); + } + // This is non-blocking, so send the callback immediately. + CHIP_ERROR err = ValidateCSR(proxy, params.GetNOCChainGenerationParameters().Value().nocsrElements, + params.GetNOCChainGenerationParameters().Value().signature, params.GetDAC().Value(), + params.GetCSRNonce().Value()); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Unable to validate CSR"); + } + CommissioningStageComplete(err); + } + break; + case CommissioningStage::kGenerateNOCChain: { + if (!params.GetNOCChainGenerationParameters().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() || + !params.GetCSRNonce().HasValue()) { ChipLogError(Controller, "Unable to generate NOC chain parameters"); return CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT); } CHIP_ERROR err = ProcessCSR(proxy, params.GetNOCChainGenerationParameters().Value().nocsrElements, params.GetNOCChainGenerationParameters().Value().signature, params.GetDAC().Value(), - params.GetCSRNonce().Value()); + params.GetPAI().Value(), params.GetCSRNonce().Value()); if (err != CHIP_NO_ERROR) { ChipLogError(Controller, "Unable to process Op CSR"); diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index a69758023edffb..be14ea4cfe30bc 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -680,10 +680,25 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, * @param[in] NOCSRElements CSR elements as per specifications section 11.22.5.6. NOCSR Elements. * @param[in] AttestationSignature Cryptographic signature generated for all the above fields. * @param[in] dac device attestation certificate + * @param[in] pai Product Attestation Intermediate certificate * @param[in] csrNonce certificate signing request nonce */ - CHIP_ERROR ProcessCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, const ByteSpan & AttestationSignature, ByteSpan dac, - ByteSpan csrNonce); + CHIP_ERROR ProcessCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, const ByteSpan & AttestationSignature, + const ByteSpan & dac, const ByteSpan & pai, const ByteSpan & csrNonce); + + /** + * @brief + * This function validates the CSR information from the device. + * (Reference: Specifications section 11.18.5.6. NOCSR Elements) + * + * @param[in] proxy device proxy + * @param[in] NOCSRElements CSR elements as per specifications section 11.22.5.6. NOCSR Elements. + * @param[in] AttestationSignature Cryptographic signature generated for all the above fields. + * @param[in] dac device attestation certificate + * @param[in] csrNonce certificate signing request nonce + */ + CHIP_ERROR ValidateCSR(DeviceProxy * proxy, const ByteSpan & NOCSRElements, const ByteSpan & AttestationSignature, + const ByteSpan & dac, const ByteSpan & csrNonce); /** * @brief diff --git a/src/controller/CommissioningDelegate.cpp b/src/controller/CommissioningDelegate.cpp index d02da1499df424..82834ce3854ae8 100644 --- a/src/controller/CommissioningDelegate.cpp +++ b/src/controller/CommissioningDelegate.cpp @@ -65,6 +65,10 @@ const char * StageToString(CommissioningStage stage) return "SendOpCertSigningRequest"; break; + case kValidateCSR: + return "ValidateCSR"; + break; + case kGenerateNOCChain: return "GenerateNOCChain"; break; diff --git a/src/controller/CommissioningDelegate.h b/src/controller/CommissioningDelegate.h index 6313f276bc14b1..9beccb6d21f9e1 100644 --- a/src/controller/CommissioningDelegate.h +++ b/src/controller/CommissioningDelegate.h @@ -39,6 +39,7 @@ enum CommissioningStage : uint8_t kSendAttestationRequest, kAttestationVerification, kSendOpCertSigningRequest, + kValidateCSR, kGenerateNOCChain, kSendTrustedRootCert, kSendNOC, diff --git a/src/controller/ExampleOperationalCredentialsIssuer.cpp b/src/controller/ExampleOperationalCredentialsIssuer.cpp index 5271c71cb58882..e2d0e731f0a6d6 100644 --- a/src/controller/ExampleOperationalCredentialsIssuer.cpp +++ b/src/controller/ExampleOperationalCredentialsIssuer.cpp @@ -176,12 +176,17 @@ CHIP_ERROR ExampleOperationalCredentialsIssuer::GenerateNOCChainAfterValidation( return NewNodeOperationalX509Cert(noc_request, pubkey, mIntermediateIssuer, noc); } -CHIP_ERROR ExampleOperationalCredentialsIssuer::GenerateNOCChain(const ByteSpan & csrElements, - const ByteSpan & attestationSignature, const ByteSpan & DAC, - const ByteSpan & PAI, const ByteSpan & PAA, +CHIP_ERROR ExampleOperationalCredentialsIssuer::GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, + const ByteSpan & attestationSignature, + const ByteSpan & attestationChallenge, const ByteSpan & DAC, + const ByteSpan & PAI, Callback::Callback * onCompletion) { VerifyOrReturnError(mInitialized, CHIP_ERROR_INCORRECT_STATE); + // At this point, Credential issuer may wish to validate the CSR information + (void) attestationChallenge; + (void) csrNonce; + NodeId assignedId; if (mNodeIdRequested) { diff --git a/src/controller/ExampleOperationalCredentialsIssuer.h b/src/controller/ExampleOperationalCredentialsIssuer.h index 47b6392caecc94..a85684cf6957e2 100644 --- a/src/controller/ExampleOperationalCredentialsIssuer.h +++ b/src/controller/ExampleOperationalCredentialsIssuer.h @@ -55,8 +55,8 @@ class DLL_EXPORT ExampleOperationalCredentialsIssuer : public OperationalCredent ExampleOperationalCredentialsIssuer(uint32_t index = 0) { mIndex = index; } ~ExampleOperationalCredentialsIssuer() override {} - CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & attestationSignature, const ByteSpan & DAC, - const ByteSpan & PAI, const ByteSpan & PAA, + CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, const ByteSpan & attestationSignature, + const ByteSpan & attestationChallenge, const ByteSpan & DAC, const ByteSpan & PAI, Callback::Callback * onCompletion) override; void SetNodeIdForNextNOCRequest(NodeId nodeId) override diff --git a/src/controller/OperationalCredentialsDelegate.h b/src/controller/OperationalCredentialsDelegate.h index ada16e5c48380e..e34b728b1ee9da 100644 --- a/src/controller/OperationalCredentialsDelegate.h +++ b/src/controller/OperationalCredentialsDelegate.h @@ -51,17 +51,19 @@ class DLL_EXPORT OperationalCredentialsDelegate * * The delegate will call `onCompletion` when the NOC certificate chain is ready. * - * @param[in] csrElements CSR elements as per specifications section 11.22.5.6. NOCSR Elements. + * @param[in] csrElements CSR elements as per specifications section 11.18.5.6. NOCSR Elements. + * @param[in] csrNonce CSR nonce as described in 6.4.6.1 * @param[in] attestationSignature Attestation signature as per specifications section 11.22.7.6. CSRResponse Command. + * @param[in] attestationChallenge Attestation challenge as per 11.18.5.7 * @param[in] DAC Device attestation certificate received from the device being commissioned * @param[in] PAI Product Attestation Intermediate certificate - * @param[in] PAA Product Attestation Authority certificate * @param[in] onCompletion Callback handler to provide generated NOC chain to the caller of GenerateNOCChain() * * @return CHIP_ERROR CHIP_NO_ERROR on success, or corresponding error code. */ - virtual CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & attestationSignature, const ByteSpan & DAC, - const ByteSpan & PAI, const ByteSpan & PAA, + virtual CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, + const ByteSpan & attestationSignature, const ByteSpan & attestationChallenge, + const ByteSpan & DAC, const ByteSpan & PAI, Callback::Callback * onCompletion) = 0; /** diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp index 191dea78a50181..08dbf659fa4e57 100644 --- a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp +++ b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp @@ -120,9 +120,10 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::GenerateNOCChainAfterValidation( return NewNodeOperationalX509Cert(noc_request, pubkey, mIssuer, noc); } -CHIP_ERROR AndroidOperationalCredentialsIssuer::GenerateNOCChain(const ByteSpan & csrElements, - const ByteSpan & attestationSignature, const ByteSpan & DAC, - const ByteSpan & PAI, const ByteSpan & PAA, +CHIP_ERROR AndroidOperationalCredentialsIssuer::GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, + const ByteSpan & attestationSignature, + const ByteSpan & attestationChallenge, const ByteSpan & DAC, + const ByteSpan & PAI, Callback::Callback * onCompletion) { jmethodID method; diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.h b/src/controller/java/AndroidOperationalCredentialsIssuer.h index b770f73833b38c..1fa5f6a6b16c24 100644 --- a/src/controller/java/AndroidOperationalCredentialsIssuer.h +++ b/src/controller/java/AndroidOperationalCredentialsIssuer.h @@ -47,8 +47,8 @@ class DLL_EXPORT AndroidOperationalCredentialsIssuer : public OperationalCredent public: virtual ~AndroidOperationalCredentialsIssuer() {} - CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & attestationSignature, const ByteSpan & DAC, - const ByteSpan & PAI, const ByteSpan & PAA, + CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, const ByteSpan & attestationSignature, + const ByteSpan & attestationChallenge, const ByteSpan & DAC, const ByteSpan & PAI, Callback::Callback * onCompletion) override; void SetNodeIdForNextNOCRequest(NodeId nodeId) override diff --git a/src/controller/python/OpCredsBinding.cpp b/src/controller/python/OpCredsBinding.cpp index 4a08a2a0402759..feb1ff099ac9f9 100644 --- a/src/controller/python/OpCredsBinding.cpp +++ b/src/controller/python/OpCredsBinding.cpp @@ -78,11 +78,12 @@ class OperationalCredentialsAdapter : public OperationalCredentialsDelegate } private: - CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & attestationSignature, const ByteSpan & DAC, - const ByteSpan & PAI, const ByteSpan & PAA, + CHIP_ERROR GenerateNOCChain(const ByteSpan & csrElements, const ByteSpan & csrNonce, const ByteSpan & attestationSignature, + const ByteSpan & attestationChallenge, const ByteSpan & DAC, const ByteSpan & PAI, Callback::Callback * onCompletion) override { - return mExampleOpCredsIssuer.GenerateNOCChain(csrElements, attestationSignature, DAC, PAI, PAA, onCompletion); + return mExampleOpCredsIssuer.GenerateNOCChain(csrElements, csrNonce, attestationSignature, attestationChallenge, DAC, PAI, + onCompletion); } void SetNodeIdForNextNOCRequest(NodeId nodeId) override { mExampleOpCredsIssuer.SetNodeIdForNextNOCRequest(nodeId); } diff --git a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h index 5846d0c5a2f24f..ae72033b6704d5 100644 --- a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h +++ b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.h @@ -46,9 +46,9 @@ class CHIPOperationalCredentialsDelegate : public chip::Controller::OperationalC */ CHIP_ERROR init(CHIPPersistentStorageDelegateBridge * storage, ChipP256KeypairPtr nocSigner, NSData * _Nullable ipk); - CHIP_ERROR GenerateNOCChain(const chip::ByteSpan & csrElements, const chip::ByteSpan & attestationSignature, - const chip::ByteSpan & DAC, const chip::ByteSpan & PAI, const chip::ByteSpan & PAA, - chip::Callback::Callback * onCompletion) override; + CHIP_ERROR GenerateNOCChain(const chip::ByteSpan & csrElements, const chip::ByteSpan & csrNonce, + const chip::ByteSpan & attestationSignature, const chip::ByteSpan & attestationChallenge, const chip::ByteSpan & DAC, + const chip::ByteSpan & PAI, chip::Callback::Callback * onCompletion) override; void SetNodeIdForNextNOCRequest(chip::NodeId nodeId) override { diff --git a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm index 04a98ef2ea9b85..e2f93ae9b5ab06 100644 --- a/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm +++ b/src/darwin/Framework/CHIP/CHIPOperationalCredentialsDelegate.mm @@ -351,9 +351,9 @@ static BOOL isRunningTests(void) return NewNodeOperationalX509Cert(noc_request, pubkey, *mIssuerKey, noc); } -CHIP_ERROR CHIPOperationalCredentialsDelegate::GenerateNOCChain(const chip::ByteSpan & csrElements, - const chip::ByteSpan & attestationSignature, const chip::ByteSpan & DAC, const chip::ByteSpan & PAI, const chip::ByteSpan & PAA, - chip::Callback::Callback * onCompletion) +CHIP_ERROR CHIPOperationalCredentialsDelegate::GenerateNOCChain(const chip::ByteSpan & csrElements, const chip::ByteSpan & csrNonce, + const chip::ByteSpan & attestationSignature, const chip::ByteSpan & attestationChallenge, const chip::ByteSpan & DAC, + const chip::ByteSpan & PAI, chip::Callback::Callback * onCompletion) { chip::NodeId assignedId; if (mNodeIdRequested) {