Stop using a fixed salt when opening commissioning windows. (project-…

…chip#16645) * Stop using a fixed salt when opening commissioning windows. Fixes project-chip#10586 * Address review comment. * Apply suggestions from code review to fix bug in salt size checking. Co-authored-by: Damian Królik <[email protected]> Co-authored-by: Damian Królik <[email protected]>
rochaferraz · Mar 31, 2022 · d94e712 · d94e712
1 parent fd8fc1a
commit d94e712
Show file tree

Hide file tree

Showing 6 changed files with 46 additions and 30 deletions.
diff --git a/examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.cpp b/examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.cpp
@@ -35,7 +35,7 @@ CHIP_ERROR OpenCommissioningWindowCommand::RunCommand()
     {
         SetupPayload ignored;
         return mWindowOpener->OpenCommissioningWindow(mNodeId, System::Clock::Seconds16(mTimeout), mIteration, mDiscriminator,
-                                                      NullOptional, &mOnOpenCommissioningWindowCallback, ignored,
+                                                      NullOptional, NullOptional, &mOnOpenCommissioningWindowCallback, ignored,
                                                       /* readVIDPIDAttributes */ true);
     }
 

diff --git a/src/controller/CommissioningWindowOpener.cpp b/src/controller/CommissioningWindowOpener.cpp
@@ -55,11 +55,19 @@ CHIP_ERROR CommissioningWindowOpener::OpenBasicCommissioningWindow(NodeId device
 
 CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, Seconds16 timeout, uint32_t iteration,
                                                               uint16_t discriminator, Optional<uint32_t> setupPIN,
+                                                              Optional<ByteSpan> salt,
                                                               Callback::Callback<OnOpenCommissioningWindow> * callback,
                                                               SetupPayload & payload, bool readVIDPIDAttributes)
 {
     VerifyOrReturnError(mNextStep == Step::kAcceptCommissioningStart, CHIP_ERROR_INCORRECT_STATE);
 
+    VerifyOrReturnError(kSpake2p_Min_PBKDF_Iterations <= iteration && iteration <= kSpake2p_Max_PBKDF_Iterations,
+                        CHIP_ERROR_INVALID_ARGUMENT);
+    VerifyOrReturnError(
+        !salt.HasValue() ||
+            (salt.Value().size() >= kSpake2p_Min_PBKDF_Salt_Length && salt.Value().size() <= kSpake2p_Max_PBKDF_Salt_Length),
+        CHIP_ERROR_INVALID_ARGUMENT);
+
     mSetupPayload = SetupPayload();
 
     if (setupPIN.HasValue())
@@ -72,6 +80,17 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S
         mCommissioningWindowOption = CommissioningWindowOption::kTokenWithRandomPIN;
     }
 
+    if (salt.HasValue())
+    {
+        memcpy(mPBKDFSaltBuffer, salt.Value().data(), salt.Value().size());
+        mPBKDFSalt = ByteSpan(mPBKDFSaltBuffer, salt.Value().size());
+    }
+    else
+    {
+        ReturnErrorOnFailure(DRBG_get_bytes(mPBKDFSaltBuffer, sizeof(mPBKDFSaltBuffer)));
+        mPBKDFSalt = ByteSpan(mPBKDFSaltBuffer);
+    }
+
     mSetupPayload.version               = 0;
     mSetupPayload.discriminator         = discriminator;
     mSetupPayload.rendezvousInformation = RendezvousInformationFlags(RendezvousInformationFlag::kOnNetwork);
@@ -80,11 +99,11 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindow(NodeId deviceId, S
     mBasicCommissioningWindowCallback = nullptr;
     mNodeId                           = deviceId;
     mCommissioningWindowTimeout       = timeout;
-    mCommissioningWindowIteration     = iteration;
+    mPBKDFIterations                  = iteration;
 
     bool randomSetupPIN = !setupPIN.HasValue();
-    ReturnErrorOnFailure(PASESession::GeneratePASEVerifier(mVerifier, mCommissioningWindowIteration, GetSPAKE2Salt(),
-                                                           randomSetupPIN, mSetupPayload.setUpPINCode));
+    ReturnErrorOnFailure(
+        PASESession::GeneratePASEVerifier(mVerifier, mPBKDFIterations, mPBKDFSalt, randomSetupPIN, mSetupPayload.setUpPINCode));
 
     payload = mSetupPayload;
 
@@ -119,8 +138,8 @@ CHIP_ERROR CommissioningWindowOpener::OpenCommissioningWindowInternal(Operationa
         request.commissioningTimeout = mCommissioningWindowTimeout.count();
         request.PAKEVerifier         = serializedVerifierSpan;
         request.discriminator        = mSetupPayload.discriminator;
-        request.iterations           = mCommissioningWindowIteration;
-        request.salt                 = GetSPAKE2Salt();
+        request.iterations           = mPBKDFIterations;
+        request.salt                 = mPBKDFSalt;
 
         ReturnErrorOnFailure(cluster.InvokeCommand(request, this, OnOpenCommissioningWindowSuccess,
                                                    OnOpenCommissioningWindowFailure, MakeOptional(kTimedInvokeTimeoutMs)));
@@ -277,15 +296,6 @@ void CommissioningWindowOpener::OnDeviceConnectionFailureCallback(void * context
     OnOpenCommissioningWindowFailure(context, error);
 }
 
-namespace {
-constexpr char kSpake2pKeyExchangeSalt[] = "SPAKE2P Key Salt";
-} // anonymous namespace
-
-ByteSpan CommissioningWindowOpener::GetSPAKE2Salt()
-{
-    return ByteSpan(Uint8::from_const_char(kSpake2pKeyExchangeSalt), sizeof(kSpake2pKeyExchangeSalt) - 1);
-}
-
 AutoCommissioningWindowOpener::AutoCommissioningWindowOpener(DeviceController * controller) :
     CommissioningWindowOpener(controller), mOnOpenCommissioningWindowCallback(OnOpenCommissioningWindowResponse, this),
     mOnOpenBasicCommissioningWindowCallback(OnOpenBasicCommissioningWindowResponse, this)
@@ -313,8 +323,8 @@ CHIP_ERROR AutoCommissioningWindowOpener::OpenBasicCommissioningWindow(DeviceCon
 
 CHIP_ERROR AutoCommissioningWindowOpener::OpenCommissioningWindow(DeviceController * controller, NodeId deviceId, Seconds16 timeout,
                                                                   uint32_t iteration, uint16_t discriminator,
-                                                                  Optional<uint32_t> setupPIN, SetupPayload & payload,
-                                                                  bool readVIDPIDAttributes)
+                                                                  Optional<uint32_t> setupPIN, Optional<ByteSpan> salt,
+                                                                  SetupPayload & payload, bool readVIDPIDAttributes)
 {
     // Not using Platform::New because we want to keep our constructor private.
     auto * opener = new AutoCommissioningWindowOpener(controller);
@@ -324,7 +334,7 @@ CHIP_ERROR AutoCommissioningWindowOpener::OpenCommissioningWindow(DeviceControll
     }
 
     CHIP_ERROR err = opener->CommissioningWindowOpener::OpenCommissioningWindow(
-        deviceId, timeout, iteration, discriminator, setupPIN, &opener->mOnOpenCommissioningWindowCallback, payload,
+        deviceId, timeout, iteration, discriminator, setupPIN, salt, &opener->mOnOpenCommissioningWindowCallback, payload,
         readVIDPIDAttributes);
     if (err != CHIP_NO_ERROR)
     {

diff --git a/src/controller/CommissioningWindowOpener.h b/src/controller/CommissioningWindowOpener.h
@@ -84,6 +84,11 @@ class CommissioningWindowOpener
      *                          PAKE passcode verifier to be used for this commissioning.
      * @param[in] discriminator The long discriminator for the DNS-SD advertisement.
      * @param[in] setupPIN      The setup PIN to use, or NullOptional to use a randomly-generated one.
+     * @param[in] salt          The salt to use, or NullOptional to use a
+     *                          randomly-generated one.  If provided, must be at
+     *                          least kSpake2p_Min_PBKDF_Salt_Length bytes and
+     *                          at most kSpake2p_Max_PBKDF_Salt_Length bytes in
+     *                          length.
      * @param[in] callback      The function to be called on success or failure of opening of commissioning window.
      * @param[out] payload      The setup payload, not including the VID/PID bits,
      *                          even if those were asked for, that is generated
@@ -98,7 +103,7 @@ class CommissioningWindowOpener
      *                                 callback.
      */
     CHIP_ERROR OpenCommissioningWindow(NodeId deviceId, System::Clock::Seconds16 timeout, uint32_t iteration,
-                                       uint16_t discriminator, Optional<uint32_t> setupPIN,
+                                       uint16_t discriminator, Optional<uint32_t> setupPIN, Optional<ByteSpan> salt,
                                        Callback::Callback<OnOpenCommissioningWindow> * callback, SetupPayload & payload,
                                        bool readVIDPIDAttributes = false);
 
@@ -124,10 +129,6 @@ class CommissioningWindowOpener
     static void OnDeviceConnectedCallback(void * context, OperationalDeviceProxy * device);
     static void OnDeviceConnectionFailureCallback(void * context, PeerId peerId, CHIP_ERROR error);
 
-    // TODO: Salt should be provided as an input or it should be randomly generated when
-    // the PIN is randomly generated.
-    static ByteSpan GetSPAKE2Salt();
-
     DeviceController * const mController = nullptr;
     Step mNextStep                       = Step::kAcceptCommissioningStart;
 
@@ -136,9 +137,12 @@ class CommissioningWindowOpener
     SetupPayload mSetupPayload;
     NodeId mNodeId                                       = kUndefinedNodeId;
     System::Clock::Seconds16 mCommissioningWindowTimeout = System::Clock::kZero;
-    uint32_t mCommissioningWindowIteration               = 0;
     CommissioningWindowOption mCommissioningWindowOption = CommissioningWindowOption::kOriginalSetupCode;
     Spake2pVerifier mVerifier; // Used for non-basic commissioning.
+    // Parameters needed for non-basic commissioning.
+    uint32_t mPBKDFIterations = 0;
+    uint8_t mPBKDFSaltBuffer[kSpake2p_Max_PBKDF_Salt_Length];
+    ByteSpan mPBKDFSalt;
 
     Callback::Callback<OnDeviceConnected> mDeviceConnected;
     Callback::Callback<OnDeviceConnectionFailure> mDeviceConnectionFailure;
@@ -160,7 +164,7 @@ class AutoCommissioningWindowOpener : private CommissioningWindowOpener
     // callback.
     static CHIP_ERROR OpenCommissioningWindow(DeviceController * controller, NodeId deviceId, System::Clock::Seconds16 timeout,
                                               uint32_t iteration, uint16_t discriminator, Optional<uint32_t> setupPIN,
-                                              SetupPayload & payload, bool readVIDPIDAttributes = false);
+                                              Optional<ByteSpan> salt, SetupPayload & payload, bool readVIDPIDAttributes = false);
 
 private:
     AutoCommissioningWindowOpener(DeviceController * controller);

diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp
@@ -624,9 +624,9 @@ JNI_METHOD(jboolean, openPairingWindowWithPIN)
     AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle);
 
     chip::SetupPayload setupPayload;
-    err = AutoCommissioningWindowOpener::OpenCommissioningWindow(wrapper->Controller(), chipDevice->GetDeviceId(),
-                                                                 System::Clock::Seconds16(duration), iteration, discriminator,
-                                                                 MakeOptional(static_cast<uint32_t>(setupPinCode)), setupPayload);
+    err = AutoCommissioningWindowOpener::OpenCommissioningWindow(
+        wrapper->Controller(), chipDevice->GetDeviceId(), System::Clock::Seconds16(duration), iteration, discriminator,
+        MakeOptional(static_cast<uint32_t>(setupPinCode)), NullOptional, setupPayload);
 
     if (err != CHIP_NO_ERROR)
     {

diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp
@@ -426,7 +426,8 @@ ChipError::StorageType pychip_DeviceController_OpenCommissioningWindow(chip::Con
     {
         SetupPayload payload;
         return Controller::AutoCommissioningWindowOpener::OpenCommissioningWindow(
-                   devCtrl, nodeid, System::Clock::Seconds16(timeout), iteration, discriminator, NullOptional, payload)
+                   devCtrl, nodeid, System::Clock::Seconds16(timeout), iteration, discriminator, NullOptional, NullOptional,
+                   payload)
             .AsInteger();
     }
 

diff --git a/src/darwin/Framework/CHIP/CHIPDeviceController.mm b/src/darwin/Framework/CHIP/CHIPDeviceController.mm
@@ -527,7 +527,8 @@ - (NSString *)openPairingWindowWithPIN:(uint64_t)deviceID
     chip::SetupPayload setupPayload;
     err = chip::Controller::AutoCommissioningWindowOpener::OpenCommissioningWindow(self.cppCommissioner, deviceID,
         chip::System::Clock::Seconds16(static_cast<uint16_t>(duration)), chip::Crypto::kSpake2p_Min_PBKDF_Iterations,
-        static_cast<uint16_t>(discriminator), chip::MakeOptional(static_cast<uint32_t>(setupPIN)), setupPayload);
+        static_cast<uint16_t>(discriminator), chip::MakeOptional(static_cast<uint32_t>(setupPIN)), chip::NullOptional,
+        setupPayload);
 
     if (err != CHIP_NO_ERROR) {
         CHIP_LOG_ERROR("Error(%s): Open Pairing Window failed", chip::ErrorStr(err));