address review comments

project-chip · Jul 29, 2021 · c284965 · c284965
1 parent 24612f1
commit c284965
Show file tree

Hide file tree

Showing 4 changed files with 47 additions and 51 deletions.
diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp
@@ -427,8 +427,6 @@ void Device::Reset()
     SetActive(false);
     mCASESession.Clear();
 
-    ReleaseCSR();
-
     mState          = ConnectionState::NotConnected;
     mSessionManager = nullptr;
     mStatusDelegate = nullptr;

diff --git a/src/controller/CHIPDevice.h b/src/controller/CHIPDevice.h
@@ -382,27 +382,15 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
 
     ByteSpan GetCSRNonce() const { return ByteSpan(mCSRNonce, sizeof(mCSRNonce)); }
 
-    CHIP_ERROR SetCSR(ByteSpan csr)
+    CHIP_ERROR SetNOC(ByteSpan noc)
     {
-        ReleaseCSR();
-        mCSR = static_cast<uint8_t *>(chip::Platform::MemoryAlloc(csr.size()));
-        ReturnErrorCodeIf(mCSR == nullptr, CHIP_ERROR_NO_MEMORY);
-        memcpy(mCSR, csr.data(), csr.size());
-        mCSRLength = csr.size();
+        VerifyOrReturnError(noc.size() <= sizeof(mNOC), CHIP_ERROR_INVALID_ARGUMENT);
+        memcpy(mNOC, noc.data(), noc.size());
+        mNOCLength = noc.size();
         return CHIP_NO_ERROR;
     }
 
-    ByteSpan GetCSR() const { return ByteSpan(mCSR, mCSRLength); }
-
-    void ReleaseCSR()
-    {
-        if (mCSR != nullptr)
-        {
-            chip::Platform::MemoryFree(mCSR);
-        }
-        mCSR       = nullptr;
-        mCSRLength = 0;
-    }
+    ByteSpan GetNOC() const { return ByteSpan(mNOC, mNOCLength); }
 
     /*
      * This function can be called to establish a secure session with the device.
@@ -512,8 +500,8 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
 
     uint8_t mCSRNonce[kOpCSRNonceLength];
 
-    uint8_t * mCSR    = nullptr;
-    size_t mCSRLength = 0;
+    uint8_t mNOC[Credentials::kMaxDERCertLength];
+    size_t mNOCLength = 0;
 
     SessionIDAllocator * mIDAllocator = nullptr;
 

diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
@@ -1258,20 +1258,13 @@ void DeviceCommissioner::OnOperationalCertificateSigningRequest(void * context,
     commissioner->mOpCSRResponseCallback.Cancel();
     commissioner->mOnCSRFailureCallback.Cancel();
 
-    Device * device = &commissioner->mActiveDevices[commissioner->mDeviceBeingPaired];
-
-    // Verify that Nonce matches with what we sent, and we have sent the root certificate
-    if (!CSRNonce.data_equal(device->GetCSRNonce()) || commissioner->SendTrustedRootCertificate(device) != CHIP_NO_ERROR)
+    if (commissioner->ProcessOpCSR(CSR, CSRNonce, VendorReserved1, VendorReserved2, VendorReserved3, Signature) != CHIP_NO_ERROR)
     {
         // Handle error, and notify session failure to the commissioner application.
         ChipLogError(Controller, "Failed to process the certificate signing request");
         // TODO: Map error status to correct error code
         commissioner->OnSessionEstablishmentError(CHIP_ERROR_INTERNAL);
     }
-    else
-    {
-        device->SetCSR(CSR);
-    }
 }
 
 void DeviceCommissioner::OnDeviceNOCGenerated(void * context, const ByteSpan & noc)
@@ -1280,28 +1273,16 @@ void DeviceCommissioner::OnDeviceNOCGenerated(void * context, const ByteSpan & n
 
     DeviceCommissioner * commissioner = static_cast<DeviceCommissioner *>(context);
 
-    // The operational certificate array can contain upto 2 certificates (NOC, and ICAC)
-    // The memory is allocated to account for both these certificates.
-    uint32_t chipCertAllocatedLen = kMaxCHIPCertLength * 2;
-    chip::Platform::ScopedMemoryBuffer<uint8_t> chipCert;
-
     Device * device = nullptr;
     VerifyOrExit(commissioner->mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(commissioner->mDeviceBeingPaired < kNumMaxActiveDevices, err = CHIP_ERROR_INCORRECT_STATE);
 
     device = &commissioner->mActiveDevices[commissioner->mDeviceBeingPaired];
+    err    = device->SetNOC(noc);
+    SuccessOrExit(err);
 
-    VerifyOrExit(chipCert.Alloc(chipCertAllocatedLen), err = CHIP_ERROR_NO_MEMORY);
-
-    {
-        MutableByteSpan chipCertSpan(chipCert.Get(), chipCertAllocatedLen);
-        err = commissioner->GenerateOperationalCertificates(noc, chipCertSpan);
-        SuccessOrExit(err);
-
-        ChipLogProgress(Controller, "Sending operational certificate to the device");
-        err = commissioner->SendOperationalCertificate(device, chipCertSpan);
-        SuccessOrExit(err);
-    }
+    err = commissioner->SendTrustedRootCertificate(device);
+    SuccessOrExit(err);
 
 exit:
     if (err != CHIP_NO_ERROR)
@@ -1311,9 +1292,19 @@ void DeviceCommissioner::OnDeviceNOCGenerated(void * context, const ByteSpan & n
     }
 }
 
-CHIP_ERROR DeviceCommissioner::ProcessOpCSR(Device * device, const ByteSpan & CSR)
+CHIP_ERROR DeviceCommissioner::ProcessOpCSR(const ByteSpan & CSR, const ByteSpan & CSRNonce, const ByteSpan & VendorReserved1,
+                                            const ByteSpan & VendorReserved2, const ByteSpan & VendorReserved3,
+                                            const ByteSpan & Signature)
 {
     VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
+    VerifyOrReturnError(mDeviceBeingPaired < kNumMaxActiveDevices, CHIP_ERROR_INCORRECT_STATE);
+
+    Device * device = &mActiveDevices[mDeviceBeingPaired];
+
+    // Verify that Nonce matches with what we sent
+    const ByteSpan nonce = device->GetCSRNonce();
+    VerifyOrReturnError(CSRNonce.size() == nonce.size(), CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(memcmp(CSRNonce.data(), nonce.data(), CSRNonce.size()) == 0, CHIP_ERROR_INVALID_ARGUMENT);
 
     chip::Platform::ScopedMemoryBuffer<uint8_t> chipOpCert;
     ReturnErrorCodeIf(!chipOpCert.Alloc(kMaxCHIPCertLength * 2), CHIP_ERROR_NO_MEMORY);
@@ -1448,6 +1439,11 @@ void DeviceCommissioner::OnRootCertSuccessResponse(void * context)
     CHIP_ERROR err  = CHIP_NO_ERROR;
     Device * device = nullptr;
 
+    // The operational certificate array can contain upto 2 certificates (NOC, and ICAC)
+    // The memory is allocated to account for both these certificates.
+    uint32_t chipCertAllocatedLen = kMaxCHIPCertLength * 2;
+    chip::Platform::ScopedMemoryBuffer<uint8_t> chipCert;
+
     VerifyOrExit(commissioner->mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);
 
     commissioner->mRootCertResponseCallback.Cancel();
@@ -1457,8 +1453,17 @@ void DeviceCommissioner::OnRootCertSuccessResponse(void * context)
 
     device = &commissioner->mActiveDevices[commissioner->mDeviceBeingPaired];
 
-    err = commissioner->ProcessOpCSR(device, device->GetCSR());
-    SuccessOrExit(err);
+    VerifyOrExit(chipCert.Alloc(chipCertAllocatedLen), err = CHIP_ERROR_NO_MEMORY);
+
+    {
+        MutableByteSpan chipCertSpan(chipCert.Get(), chipCertAllocatedLen);
+        err = commissioner->GenerateOperationalCertificates(device->GetNOC(), chipCertSpan);
+        SuccessOrExit(err);
+
+        ChipLogProgress(Controller, "Sending operational certificate to the device");
+        err = commissioner->SendOperationalCertificate(device, chipCertSpan);
+        SuccessOrExit(err);
+    }
 
 exit:
     if (err != CHIP_NO_ERROR)

diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
@@ -677,10 +677,15 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
      *   This function processes the CSR sent by the device.
      *   (Reference: Specifications section 11.22.5.8. OpCSR Elements)
      *
-     * @param[in] device The object that contains device's current state.
-     * @param[in] CSR    The Certificate Signing Request.
+     * @param[in] CSR             The Certificate Signing Request.
+     * @param[in] CSRNonce        The Nonce sent by us when we requested the CSR.
+     * @param[in] VendorReserved1 vendor-specific information that may aid in device commissioning.
+     * @param[in] VendorReserved2 vendor-specific information that may aid in device commissioning.
+     * @param[in] VendorReserved3 vendor-specific information that may aid in device commissioning.
+     * @param[in] Signature       Cryptographic signature generated for all the above fields.
      */
-    CHIP_ERROR ProcessOpCSR(Device * device, const ByteSpan & CSR);
+    CHIP_ERROR ProcessOpCSR(const ByteSpan & CSR, const ByteSpan & CSRNonce, const ByteSpan & VendorReserved1,
+                            const ByteSpan & VendorReserved2, const ByteSpan & VendorReserved3, const ByteSpan & Signature);
 
     // Cluster callbacks for advancing commissioning flows
     Callback::Callback<BasicSuccessCallback> mSuccess;