From 2a884cc0c1a8f98da4e2d39612d3a2c7d22be047 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 3 Aug 2021 11:07:11 -0700 Subject: [PATCH] address review comments --- .../administrator-commissioning-server.cpp | 4 ++-- src/controller/CHIPDevice.cpp | 10 ++++++---- src/protocols/secure_channel/PASESession.cpp | 14 +++++++------- src/protocols/secure_channel/PASESession.h | 6 +++++- .../secure_channel/RendezvousParameters.h | 2 +- 5 files changed, 21 insertions(+), 15 deletions(-) diff --git a/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp b/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp index 64b684712e4433..86d7961c4e71ba 100644 --- a/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp +++ b/src/app/clusters/administrator-commissioning-server/administrator-commissioning-server.cpp @@ -53,8 +53,8 @@ bool emberAfAdministratorCommissioningClusterOpenCommissioningWindowCallback(chi VerifyOrExit(commissioningTimeout <= kMaxCommissionioningTimeoutSeconds, status = EMBER_ZCL_STATUS_FAILURE); VerifyOrExit(discriminator <= kMaxDiscriminatorValue, status = EMBER_ZCL_STATUS_FAILURE); - memcpy(&verifier[0][0], &verifierData[0], kSpake2p_WS_Length); - memcpy(&verifier[1][0], &verifierData[kSpake2p_WS_Length], kSpake2p_WS_Length); + memcpy(verifier.mW0, &verifierData[0], kSpake2p_WS_Length); + memcpy(verifier.mL, &verifierData[kSpake2p_WS_Length], kSpake2p_WS_Length); VerifyOrExit(OpenPairingWindowUsingVerifier(commissioningTimeout, discriminator, verifier, iterations, salt, passcodeID) == CHIP_NO_ERROR, diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index a4b6f0c0e0e28a..4f99d69fe8a9f2 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -358,8 +358,10 @@ void Device::OnOpenPairingWindowFailureResponse(void * context, uint8_t status) CHIP_ERROR Device::OpenPairingWindow(uint16_t timeout, PairingWindowOption option, SetupPayload & setupPayload) { + constexpr EndpointId kAdministratorCommissioningClusterEndpoint = 0; + chip::Controller::AdministratorCommissioningCluster cluster; - cluster.Associate(this, 0); + cluster.Associate(this, kAdministratorCommissioningClusterEndpoint); Callback::Cancelable * successCallback = mOpenPairingSuccessCallback.Cancel(); Callback::Cancelable * failureCallback = mOpenPairingFailureCallback.Cancel(); @@ -372,9 +374,9 @@ CHIP_ERROR Device::OpenPairingWindow(uint16_t timeout, PairingWindowOption optio ReturnErrorOnFailure( PASESession::GeneratePASEVerifier(verifier, kPBKDFMinimumIterations, salt, randomSetupPIN, setupPayload.setUpPINCode)); - ReturnErrorOnFailure( - cluster.OpenCommissioningWindow(successCallback, failureCallback, timeout, ByteSpan(&verifier[0][0], sizeof(verifier)), - setupPayload.discriminator, kPBKDFMinimumIterations, salt, mPAKEVerifierID++)); + ReturnErrorOnFailure(cluster.OpenCommissioningWindow( + successCallback, failureCallback, timeout, ByteSpan(reinterpret_cast(&verifier), sizeof(verifier)), + setupPayload.discriminator, kPBKDFMinimumIterations, salt, mPAKEVerifierID++)); } else { diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index a86b93b0efed7c..9a99ec44d368b3 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -80,7 +80,7 @@ void PASESession::Clear() // This function zeroes out and resets the memory used by the object. // It's done so that no security related information will be leaked. memset(&mPoint[0], 0, sizeof(mPoint)); - memset(&mPASEVerifier[0][0], 0, sizeof(mPASEVerifier)); + memset(&mPASEVerifier, 0, sizeof(mPASEVerifier)); memset(&mKe[0], 0, sizeof(mKe)); mNextExpectedMsg = Protocols::SecureChannel::MsgType::PASE_Spake2pError; @@ -209,7 +209,7 @@ CHIP_ERROR PASESession::ComputePASEVerifier(uint32_t setUpPINCode, uint32_t pbkd Encoding::LittleEndian::Put32(littleEndianSetupPINCode, setUpPINCode); return mPBKDF.pbkdf2_sha256(littleEndianSetupPINCode, sizeof(littleEndianSetupPINCode), salt.data(), salt.size(), - pbkdf2IterCount, sizeof(PASEVerifier), &verifier[0][0]); + pbkdf2IterCount, sizeof(PASEVerifier), reinterpret_cast(&verifier)); } CHIP_ERROR PASESession::GeneratePASEVerifier(PASEVerifier & verifier, uint32_t pbkdf2IterCount, const ByteSpan & salt, @@ -292,7 +292,7 @@ CHIP_ERROR PASESession::WaitForPairing(const PASEVerifier & verifier, uint32_t p { ReturnErrorOnFailure(WaitForPairing(0, pbkdf2IterCount, salt, myKeyId, delegate)); - memmove(&mPASEVerifier, verifier, sizeof(verifier)); + memmove(&mPASEVerifier, &verifier, sizeof(verifier)); mComputeVerifier = false; mPasscodeID = passcodeID; @@ -427,7 +427,7 @@ CHIP_ERROR PASESession::SendPBKDFParamResponse() // Update commissioning hash with the pbkdf2 param response that's being sent. ReturnErrorOnFailure(mCommissioningHash.AddData(ByteSpan{ resp->Start(), resp->DataLength() })); ReturnErrorOnFailure(SetupSpake2p(mIterationCount, ByteSpan(mSalt, mSaltLength))); - ReturnErrorOnFailure(mSpake2p.ComputeL(mPoint, &sizeof_point, &mPASEVerifier[1][0], kSpake2p_WS_Length)); + ReturnErrorOnFailure(mSpake2p.ComputeL(mPoint, &sizeof_point, mPASEVerifier.mL, kSpake2p_WS_Length)); mNextExpectedMsg = Protocols::SecureChannel::MsgType::PASE_Spake2p1; @@ -492,8 +492,8 @@ CHIP_ERROR PASESession::SendMsg1() uint8_t X[kMAX_Point_Length]; size_t X_len = sizeof(X); - ReturnErrorOnFailure(mSpake2p.BeginProver(nullptr, 0, nullptr, 0, &mPASEVerifier[0][0], kSpake2p_WS_Length, - &mPASEVerifier[1][0], kSpake2p_WS_Length)); + ReturnErrorOnFailure( + mSpake2p.BeginProver(nullptr, 0, nullptr, 0, mPASEVerifier.mW0, kSpake2p_WS_Length, mPASEVerifier.mL, kSpake2p_WS_Length)); ReturnErrorOnFailure(mSpake2p.ComputeRoundOne(NULL, 0, X, &X_len)); @@ -535,7 +535,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const System::PacketBufferHandle VerifyOrExit(buf != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(buf_len == sizeof(encryptionKeyId) + kMAX_Point_Length, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); - err = mSpake2p.BeginVerifier(nullptr, 0, nullptr, 0, &mPASEVerifier[0][0], kSpake2p_WS_Length, mPoint, sizeof(mPoint)); + err = mSpake2p.BeginVerifier(nullptr, 0, nullptr, 0, mPASEVerifier.mW0, kSpake2p_WS_Length, mPoint, sizeof(mPoint)); SuccessOrExit(err); encryptionKeyId = chip::Encoding::LittleEndian::Read16(buf); diff --git a/src/protocols/secure_channel/PASESession.h b/src/protocols/secure_channel/PASESession.h index 08eecc75dde08b..b9de64824b8205 100644 --- a/src/protocols/secure_channel/PASESession.h +++ b/src/protocols/secure_channel/PASESession.h @@ -74,7 +74,11 @@ struct PASESessionSerializable uint16_t mPeerKeyId; }; -typedef uint8_t PASEVerifier[2][kSpake2p_WS_Length]; +struct PASEVerifier +{ + uint8_t mW0[kSpake2p_WS_Length]; + uint8_t mL[kSpake2p_WS_Length]; +}; class DLL_EXPORT PASESession : public Messaging::ExchangeDelegate, public PairingSession { diff --git a/src/protocols/secure_channel/RendezvousParameters.h b/src/protocols/secure_channel/RendezvousParameters.h index fda1299c2be37c..0dab80e7515ea4 100644 --- a/src/protocols/secure_channel/RendezvousParameters.h +++ b/src/protocols/secure_channel/RendezvousParameters.h @@ -106,7 +106,7 @@ class RendezvousParameters const PASEVerifier & GetPASEVerifier() const { return mPASEVerifier; } RendezvousParameters & SetPASEVerifier(PASEVerifier & verifier) { - memmove(mPASEVerifier, verifier, sizeof(verifier)); + memmove(&mPASEVerifier, &verifier, sizeof(verifier)); mHasPASEVerifier = true; return *this; }