Skip to content

Commit

Permalink
Stop using a fixed salt when opening commissioning windows. (#16645)
Browse files Browse the repository at this point in the history 
* Stop using a fixed salt when opening commissioning windows.

Fixes #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]>
  • Loading branch information
@bzbarsky-apple @Damian-Nordic
bzbarsky-apple and Damian-Nordic authored Mar 28, 2022
1 parent 1676f1b commit 467c696
Show file tree
Hide file tree
Showing 6 changed files with 46 additions and 30 deletions.
2 changes: 1 addition & 1 deletion examples/chip-tool/commands/pairing/OpenCommissioningWindowCommand.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
44 changes: 27 additions & 17 deletions src/controller/CommissioningWindowOpener.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand All @@ -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);
Expand All @@ -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;

Expand Down Expand Up @@ -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)));
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
Expand All @@ -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)
{
Expand Down
18 changes: 11 additions & 7 deletions src/controller/CommissioningWindowOpener.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

Expand All @@ -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;

Expand All @@ -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;
Expand All @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions src/controller/java/CHIPDeviceController-JNI.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
3 changes: 2 additions & 1 deletion src/controller/python/ChipDeviceController-ScriptBinding.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
3 changes: 2 additions & 1 deletion src/darwin/Framework/CHIP/CHIPDeviceController.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down

0 comments on commit 467c696

Please sign in to comment.