From 5263a9471d2d2433941c35ce131a42d2241c18d9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 28 Mar 2022 04:38:26 -0400 Subject: [PATCH] Stop using a fixed salt when opening commissioning windows. (#16645) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Stop using a fixed salt when opening commissioning windows. Fixes https://github.com/project-chip/connectedhomeip/issues/10586 * Address review comment. * Apply suggestions from code review to fix bug in salt size checking. Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com> Co-authored-by: Damian Królik <66667989+Damian-Nordic@users.noreply.github.com> --- .../OpenCommissioningWindowCommand.cpp | 2 +- src/controller/CommissioningWindowOpener.cpp | 44 ++++++++++++------- src/controller/CommissioningWindowOpener.h | 18 +++++--- .../java/CHIPDeviceController-JNI.cpp | 6 +-- .../ChipDeviceController-ScriptBinding.cpp | 3 +- .../Framework/CHIP/CHIPDeviceController.mm | 3 +- 6 files changed, 46 insertions(+), 30 deletions(-) diff --git a/examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.cpp b/examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.cpp index bd39eb855b715b..6624443b137f9c 100644 --- 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 index 977c44ffc74245..9ff1c486ee6a8f 100644 --- 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 setupPIN, + Optional salt, Callback::Callback * 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 setupPIN, SetupPayload & payload, - bool readVIDPIDAttributes) + Optional setupPIN, Optional 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 index d3bc789d35c06f..30be3d90051557 100644 --- 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 setupPIN, + uint16_t discriminator, Optional setupPIN, Optional salt, Callback::Callback * 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 mDeviceConnected; Callback::Callback 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 setupPIN, - SetupPayload & payload, bool readVIDPIDAttributes = false); + Optional 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 index b4338f067e5b66..7c34725b446a22 100644 --- 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(setupPinCode)), setupPayload); + err = AutoCommissioningWindowOpener::OpenCommissioningWindow( + wrapper->Controller(), chipDevice->GetDeviceId(), System::Clock::Seconds16(duration), iteration, discriminator, + MakeOptional(static_cast(setupPinCode)), NullOptional, setupPayload); if (err != CHIP_NO_ERROR) { diff --git a/src/controller/python/ChipDeviceController-ScriptBinding.cpp b/src/controller/python/ChipDeviceController-ScriptBinding.cpp index 482a062e924f34..919809b4479b21 100644 --- 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 index cd15f0bf622c11..738d5b79839e55 100644 --- 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(duration)), chip::Crypto::kSpake2p_Min_PBKDF_Iterations, - static_cast(discriminator), chip::MakeOptional(static_cast(setupPIN)), setupPayload); + static_cast(discriminator), chip::MakeOptional(static_cast(setupPIN)), chip::NullOptional, + setupPayload); if (err != CHIP_NO_ERROR) { CHIP_LOG_ERROR("Error(%s): Open Pairing Window failed", chip::ErrorStr(err));