From a5c46bdb394d1d22632bd9ba4e8fb998cb747ff4 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Mon, 17 May 2021 15:55:59 -0700 Subject: [PATCH 1/5] Resurrect "Cleanup PASE code, error handling and error logging (#6852)"" This reverts commit 6bfdd05783f4fb60148fa469d29f2bcf16b7f50b. --- src/lib/support/logging/CHIPLogging.cpp | 2 + src/lib/support/logging/Constants.h | 2 + src/protocols/secure_channel/PASESession.cpp | 171 ++++++++++--------- src/protocols/secure_channel/PASESession.h | 5 +- 4 files changed, 96 insertions(+), 84 deletions(-) diff --git a/src/lib/support/logging/CHIPLogging.cpp b/src/lib/support/logging/CHIPLogging.cpp index d70009e210b10f..1a51e17909e456 100644 --- a/src/lib/support/logging/CHIPLogging.cpp +++ b/src/lib/support/logging/CHIPLogging.cpp @@ -91,6 +91,8 @@ static const char ModuleNames[] = "-\0\0" // None "SPL" // SetupPayload "SVR" // AppServer "DIS" // Discovery + "PAS" // PASE + "CAS" // CASE ; #define ModuleNamesCount ((sizeof(ModuleNames) - 1) / chip::Logging::kMaxModuleNameLen) diff --git a/src/lib/support/logging/Constants.h b/src/lib/support/logging/Constants.h index c605e2fd93214b..5b7f452b97ca67 100644 --- a/src/lib/support/logging/Constants.h +++ b/src/lib/support/logging/Constants.h @@ -54,6 +54,8 @@ enum LogModule kLogModule_SetupPayload, kLogModule_AppServer, kLogModule_Discovery, + kLogModule_PASE, + kLogModule_CASE, kLogModule_Max }; diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 8fb5ce8e8a0816..e0174b40d9398e 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -40,6 +40,7 @@ #include #include #include +#include #include #include @@ -184,7 +185,7 @@ CHIP_ERROR PASESession::Init(uint16_t myKeyId, uint32_t setupCode, SessionEstabl mDelegate = delegate; - ChipLogDetail(Ble, "Assigned local session key ID %d", myKeyId); + ChipLogDetail(PASE, "Assigned local session key ID %d", myKeyId); mConnectionState.SetLocalKeyID(myKeyId); mSetupPINCode = setupCode; mComputeVerifier = true; @@ -245,12 +246,10 @@ CHIP_ERROR PASESession::SetupSpake2p(uint32_t pbkdf2IterCount, const uint8_t * s CHIP_ERROR PASESession::WaitForPairing(uint32_t mySetUpPINCode, uint32_t pbkdf2IterCount, const uint8_t * salt, size_t saltLen, uint16_t myKeyId, SessionEstablishmentDelegate * delegate) { - CHIP_ERROR err = CHIP_NO_ERROR; - - VerifyOrExit(salt != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(saltLen > 0, err = CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(salt == nullptr, CHIP_ERROR_INVALID_ARGUMENT); + ReturnErrorCodeIf(saltLen == 0, CHIP_ERROR_INVALID_ARGUMENT); - err = Init(myKeyId, mySetUpPINCode, delegate); + CHIP_ERROR err = Init(myKeyId, mySetUpPINCode, delegate); SuccessOrExit(err); VerifyOrExit(CanCastTo(saltLen), err = CHIP_ERROR_INVALID_ARGUMENT); @@ -272,7 +271,7 @@ CHIP_ERROR PASESession::WaitForPairing(uint32_t mySetUpPINCode, uint32_t pbkdf2I mNextExpectedMsg = Protocols::SecureChannel::MsgType::PBKDFParamRequest; mPairingComplete = false; - ChipLogDetail(Ble, "Waiting for PBKDF param request"); + ChipLogDetail(PASE, "Waiting for PBKDF param request"); exit: if (err != CHIP_NO_ERROR) @@ -284,19 +283,14 @@ CHIP_ERROR PASESession::WaitForPairing(uint32_t mySetUpPINCode, uint32_t pbkdf2I CHIP_ERROR PASESession::WaitForPairing(const PASEVerifier & verifier, uint16_t myKeyId, SessionEstablishmentDelegate * delegate) { - CHIP_ERROR err = WaitForPairing(0, kSpake2p_Iteration_Count, reinterpret_cast(kSpake2pKeyExchangeSalt), - strlen(kSpake2pKeyExchangeSalt), myKeyId, delegate); - SuccessOrExit(err); + ReturnErrorOnFailure(WaitForPairing(0, kSpake2p_Iteration_Count, + reinterpret_cast(kSpake2pKeyExchangeSalt), + strlen(kSpake2pKeyExchangeSalt), myKeyId, delegate)); memmove(&mPASEVerifier, verifier, sizeof(verifier)); mComputeVerifier = false; -exit: - if (err != CHIP_NO_ERROR) - { - Clear(); - } - return err; + return CHIP_NO_ERROR; } CHIP_ERROR PASESession::Pair(const Transport::PeerAddress peerAddress, uint32_t peerSetUpPINCode, uint16_t myKeyId, @@ -324,12 +318,13 @@ CHIP_ERROR PASESession::Pair(const Transport::PeerAddress peerAddress, uint32_t void PASESession::OnResponseTimeout(ExchangeContext * ec) { - VerifyOrReturn(ec != nullptr, ChipLogError(Ble, "PASESession::OnResponseTimeout was called by null exchange")); + VerifyOrReturn(ec != nullptr, ChipLogError(PASE, "PASESession::OnResponseTimeout was called by null exchange")); VerifyOrReturn(mExchangeCtxt == nullptr || mExchangeCtxt == ec, - ChipLogError(Ble, "PASESession::OnResponseTimeout exchange doesn't match")); - ChipLogError(Ble, "PASESession timed out while waiting for a response from the peer. Expected message type was %d", + ChipLogError(PASE, "PASESession::OnResponseTimeout exchange doesn't match")); + ChipLogError(PASE, "PASESession timed out while waiting for a response from the peer. Expected message type was %d", mNextExpectedMsg); mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); + Clear(); } CHIP_ERROR PASESession::DeriveSecureSession(SecureSession & session, SecureSession::SessionRole role) @@ -341,35 +336,24 @@ CHIP_ERROR PASESession::DeriveSecureSession(SecureSession & session, SecureSessi CHIP_ERROR PASESession::SendPBKDFParamRequest() { - CHIP_ERROR err = CHIP_NO_ERROR; - System::PacketBufferHandle req = System::PacketBufferHandle::New(kPBKDFParamRandomNumberSize); - VerifyOrExit(!req.IsNull(), err = CHIP_SYSTEM_ERROR_NO_MEMORY); + VerifyOrReturnError(!req.IsNull(), CHIP_SYSTEM_ERROR_NO_MEMORY); - err = DRBG_get_bytes(req->Start(), kPBKDFParamRandomNumberSize); - SuccessOrExit(err); + ReturnErrorOnFailure(DRBG_get_bytes(req->Start(), kPBKDFParamRandomNumberSize)); req->SetDataLength(kPBKDFParamRandomNumberSize); // Update commissioning hash with the pbkdf2 param request that's being sent. - err = mCommissioningHash.AddData(req->Start(), req->DataLength()); - SuccessOrExit(err); + ReturnErrorOnFailure(mCommissioningHash.AddData(req->Start(), req->DataLength())); mNextExpectedMsg = Protocols::SecureChannel::MsgType::PBKDFParamResponse; - err = mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PBKDFParamRequest, std::move(req), - SendFlags(SendMessageFlags::kExpectResponse)); - SuccessOrExit(err); - - ChipLogDetail(Ble, "Sent PBKDF param request"); + ReturnErrorOnFailure(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PBKDFParamRequest, std::move(req), + SendFlags(SendMessageFlags::kExpectResponse))); -exit: + ChipLogDetail(PASE, "Sent PBKDF param request"); - if (err != CHIP_NO_ERROR) - { - Clear(); - } - return err; + return CHIP_NO_ERROR; } CHIP_ERROR PASESession::HandlePBKDFParamRequest(const System::PacketBufferHandle & msg) @@ -383,7 +367,7 @@ CHIP_ERROR PASESession::HandlePBKDFParamRequest(const System::PacketBufferHandle VerifyOrExit(req != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(reqlen == kPBKDFParamRandomNumberSize, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); - ChipLogDetail(Ble, "Received PBKDF param request"); + ChipLogDetail(PASE, "Received PBKDF param request"); // Update commissioning hash with the received pbkdf2 param request err = mCommissioningHash.AddData(req, reqlen); @@ -439,7 +423,7 @@ CHIP_ERROR PASESession::SendPBKDFParamResponse() ReturnErrorOnFailure(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PBKDFParamResponse, std::move(resp), SendFlags(SendMessageFlags::kExpectResponse))); - ChipLogDetail(Ble, "Sent PBKDF param response"); + ChipLogDetail(PASE, "Sent PBKDF param response"); return CHIP_NO_ERROR; } @@ -457,7 +441,7 @@ CHIP_ERROR PASESession::HandlePBKDFParamResponse(const System::PacketBufferHandl static_assert(CHAR_BIT == 8, "Assuming that sizeof returns octets"); size_t fixed_resplen = kPBKDFParamRandomNumberSize + sizeof(uint64_t) + sizeof(uint32_t); - ChipLogDetail(Ble, "Received PBKDF param response"); + ChipLogDetail(PASE, "Received PBKDF param response"); VerifyOrExit(resp != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(resplen >= fixed_resplen, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); @@ -514,7 +498,7 @@ CHIP_ERROR PASESession::SendMsg1() // Call delegate to send the Msg1 to peer ReturnErrorOnFailure(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PASE_Spake2p1, bbuf.Finalize(), SendFlags(SendMessageFlags::kExpectResponse))); - ChipLogDetail(Ble, "Sent spake2p msg1"); + ChipLogDetail(PASE, "Sent spake2p msg1"); return CHIP_NO_ERROR; } @@ -536,7 +520,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const System::PacketBufferHandle uint16_t encryptionKeyId = 0; - ChipLogDetail(Ble, "Received spake2p msg1"); + ChipLogDetail(PASE, "Received spake2p msg1"); VerifyOrExit(buf != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(buf_len == sizeof(encryptionKeyId) + kMAX_Point_Length, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); @@ -551,7 +535,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const System::PacketBufferHandle err = mSpake2p.ComputeRoundOne(msg->Start(), msg->DataLength(), Y, &Y_len); SuccessOrExit(err); - ChipLogDetail(Ble, "Peer assigned session key ID %d", encryptionKeyId); + ChipLogDetail(PASE, "Peer assigned session key ID %d", encryptionKeyId); mConnectionState.SetPeerKeyID(encryptionKeyId); err = mSpake2p.ComputeRoundTwo(msg->Start(), msg->DataLength(), verifier, &verifier_len); @@ -578,7 +562,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const System::PacketBufferHandle SuccessOrExit(err); } - ChipLogDetail(Ble, "Sent spake2p msg2"); + ChipLogDetail(PASE, "Sent spake2p msg2"); exit: @@ -606,7 +590,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle uint16_t encryptionKeyId = 0; - ChipLogDetail(Ble, "Received spake2p msg2"); + ChipLogDetail(PASE, "Received spake2p msg2"); VerifyOrExit(buf != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(buf_len == sizeof(encryptionKeyId) + kMAX_Point_Length + kMAX_Hash_Length, @@ -617,7 +601,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle buf = msg->Start(); buf_len = msg->DataLength(); - ChipLogDetail(Ble, "Peer assigned session key ID %d", encryptionKeyId); + ChipLogDetail(PASE, "Peer assigned session key ID %d", encryptionKeyId); mConnectionState.SetPeerKeyID(encryptionKeyId); err = mSpake2p.ComputeRoundTwo(buf, kMAX_Point_Length, verifier, &verifier_len_raw); @@ -637,7 +621,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle SuccessOrExit(err); } - ChipLogDetail(Ble, "Sent spake2p msg3"); + ChipLogDetail(PASE, "Sent spake2p msg3"); { const uint8_t * hash = &buf[kMAX_Point_Length]; @@ -654,6 +638,13 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle mPairingComplete = true; + // Close the exchange, as no additional messages are expected from the peer + if (mExchangeCtxt != nullptr) + { + mExchangeCtxt->Close(); + mExchangeCtxt = nullptr; + } + // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -672,7 +663,7 @@ CHIP_ERROR PASESession::HandleMsg3(const System::PacketBufferHandle & msg) const uint8_t * hash = msg->Start(); Spake2pErrorType spake2pErr = Spake2pErrorType::kUnexpected; - ChipLogDetail(Ble, "Received spake2p msg3"); + ChipLogDetail(PASE, "Received spake2p msg3"); // We will set NextExpectedMsg to PASE_Spake2pError in all cases // However, when we are using IP rendezvous, we might set it to PASE_Spake2p1. @@ -693,6 +684,13 @@ CHIP_ERROR PASESession::HandleMsg3(const System::PacketBufferHandle & msg) mPairingComplete = true; + // Close the exchange, as no additional messages are expected from the peer + if (mExchangeCtxt != nullptr) + { + mExchangeCtxt->Close(); + mExchangeCtxt = nullptr; + } + // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -707,64 +705,69 @@ CHIP_ERROR PASESession::HandleMsg3(const System::PacketBufferHandle & msg) void PASESession::SendErrorMsg(Spake2pErrorType errorCode) { - CHIP_ERROR err = CHIP_NO_ERROR; - System::PacketBufferHandle msg; uint16_t msglen = sizeof(Spake2pErrorMsg); Spake2pErrorMsg * pMsg = nullptr; msg = System::PacketBufferHandle::New(msglen); - VerifyOrExit(!msg.IsNull(), err = CHIP_SYSTEM_ERROR_NO_MEMORY); + VerifyOrReturn(!msg.IsNull(), ChipLogError(PASE, "Failed to allocate error message")); pMsg = reinterpret_cast(msg->Start()); pMsg->error = errorCode; msg->SetDataLength(msglen); - err = mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PASE_Spake2pError, std::move(msg)); - SuccessOrExit(err); - -exit: - Clear(); + VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PASE_Spake2pError, std::move(msg)), + ChipLogError(PASE, "Failed to send error message")); } -void PASESession::HandleErrorMsg(const System::PacketBufferHandle & msg) +CHIP_ERROR PASESession::HandleErrorMsg(const System::PacketBufferHandle & msg) { - // Request message processing - const uint8_t * buf = msg->Start(); - size_t buflen = msg->DataLength(); - Spake2pErrorMsg * pMsg = nullptr; - - VerifyOrExit(buf != nullptr, ChipLogError(Ble, "Null error msg received during pairing")); - VerifyOrExit(buflen == sizeof(Spake2pErrorMsg), ChipLogError(Ble, "Error msg with incorrect length received during pairing")); + ReturnErrorCodeIf(msg->Start() == nullptr || msg->DataLength() != sizeof(Spake2pErrorMsg), CHIP_ERROR_MESSAGE_INCOMPLETE); - pMsg = reinterpret_cast(msg->Start()); - ChipLogError(Ble, "Received error (%d) during pairing process", pMsg->error); + Spake2pErrorMsg * pMsg = reinterpret_cast(msg->Start()); + ChipLogError(PASE, "Received error during pairing process. %s", ErrorStr(pMsg->error)); - mDelegate->OnSessionEstablishmentError(pMsg->error); - -exit: - Clear(); + return pMsg->error; } -void PASESession::OnMessageReceived(ExchangeContext * ec, const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, - System::PacketBufferHandle && msg) +CHIP_ERROR PASESession::ValidateReceivedMessage(ExchangeContext * exchange, const PacketHeader & packetHeader, + const PayloadHeader & payloadHeader, System::PacketBufferHandle && msg) { - CHIP_ERROR err = CHIP_NO_ERROR; + VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(ec != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(mExchangeCtxt == nullptr || mExchangeCtxt == ec, err = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(!msg.IsNull(), err = CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrExit(payloadHeader.HasMessageType(mNextExpectedMsg) || - payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::PASE_Spake2pError), - err = CHIP_ERROR_INVALID_MESSAGE_TYPE); - - if (mExchangeCtxt == nullptr) + // mExchangeCtxt can be nullptr if this is the first message (PBKDFParamRequest) received by PASESession + // via UnsolicitedMessageHandler. The exchange context is allocated by exchange manager and provided + // to the handler (PASESession object). + if (mExchangeCtxt != nullptr) + { + if (mExchangeCtxt != exchange) + { + // Close the incoming exchange explicitly, as the cleanup code only closes mExchangeCtxt + exchange->Close(); + ReturnErrorOnFailure(CHIP_ERROR_INVALID_ARGUMENT); + } + } + else { - mExchangeCtxt = ec; + mExchangeCtxt = exchange; mExchangeCtxt->SetResponseTimeout(kSpake2p_Response_Timeout); } + VerifyOrReturnError(!msg.IsNull(), CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(payloadHeader.HasMessageType(mNextExpectedMsg) || + payloadHeader.HasMessageType(Protocols::SecureChannel::MsgType::PASE_Spake2pError), + CHIP_ERROR_INVALID_MESSAGE_TYPE); + + return CHIP_NO_ERROR; +} + +void PASESession::OnMessageReceived(ExchangeContext * exchange, const PacketHeader & packetHeader, + const PayloadHeader & payloadHeader, System::PacketBufferHandle && msg) +{ + CHIP_ERROR err = ValidateReceivedMessage(exchange, packetHeader, payloadHeader, msg); + SuccessOrExit(err); + mConnectionState.SetPeerAddress(mMessageDispatch.GetPeerAddress()); switch (static_cast(payloadHeader.GetMessageType())) @@ -790,7 +793,7 @@ void PASESession::OnMessageReceived(ExchangeContext * ec, const PacketHeader & p break; case Protocols::SecureChannel::MsgType::PASE_Spake2pError: - HandleErrorMsg(msg); + err = HandleErrorMsg(msg); break; default: @@ -803,6 +806,8 @@ void PASESession::OnMessageReceived(ExchangeContext * ec, const PacketHeader & p // Call delegate to indicate pairing failure if (err != CHIP_NO_ERROR) { + Clear(); + ChipLogError(PASE, "Failed during PASE session setup. %s", ErrorStr(err)); mDelegate->OnSessionEstablishmentError(err); } } diff --git a/src/protocols/secure_channel/PASESession.h b/src/protocols/secure_channel/PASESession.h index bc814f0424b183..4cc8abe90773ee 100644 --- a/src/protocols/secure_channel/PASESession.h +++ b/src/protocols/secure_channel/PASESession.h @@ -243,6 +243,9 @@ class DLL_EXPORT PASESession : public Messaging::ExchangeDelegateBase, public Pa CHIP_ERROR Init(uint16_t myKeyId, uint32_t setupCode, SessionEstablishmentDelegate * delegate); + CHIP_ERROR ValidateReceivedMessage(Messaging::ExchangeContext * exchange, const PacketHeader & packetHeader, + const PayloadHeader & payloadHeader, System::PacketBufferHandle & msg); + static CHIP_ERROR ComputePASEVerifier(uint32_t mySetUpPINCode, uint32_t pbkdf2IterCount, const uint8_t * salt, size_t saltLen, PASEVerifier & verifier); @@ -261,7 +264,7 @@ class DLL_EXPORT PASESession : public Messaging::ExchangeDelegateBase, public Pa CHIP_ERROR HandleMsg3(const System::PacketBufferHandle & msg); void SendErrorMsg(Spake2pErrorType errorCode); - void HandleErrorMsg(const System::PacketBufferHandle & msg); + CHIP_ERROR HandleErrorMsg(const System::PacketBufferHandle & msg); SessionEstablishmentDelegate * mDelegate = nullptr; From 0cb8cc839e9c3679151a1f8b6db98167d22d98a9 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 18 May 2021 14:10:49 -0700 Subject: [PATCH 2/5] cleanup code --- src/protocols/secure_channel/PASESession.cpp | 18 +++++++----------- src/protocols/secure_channel/PASESession.h | 4 +++- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index e0174b40d9398e..28ba609791a616 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -99,7 +99,11 @@ void PASESession::Clear() mPairingComplete = false; mComputeVerifier = true; mConnectionState.Reset(); + CloseExchange(); +} +void PASESession::CloseExchange() +{ if (mExchangeCtxt != nullptr) { mExchangeCtxt->Close(); @@ -639,11 +643,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle mPairingComplete = true; // Close the exchange, as no additional messages are expected from the peer - if (mExchangeCtxt != nullptr) - { - mExchangeCtxt->Close(); - mExchangeCtxt = nullptr; - } + CloseExchange(); // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -685,11 +685,7 @@ CHIP_ERROR PASESession::HandleMsg3(const System::PacketBufferHandle & msg) mPairingComplete = true; // Close the exchange, as no additional messages are expected from the peer - if (mExchangeCtxt != nullptr) - { - mExchangeCtxt->Close(); - mExchangeCtxt = nullptr; - } + CloseExchange(); // Call delegate to indicate pairing completion mDelegate->OnSessionEstablished(); @@ -765,7 +761,7 @@ CHIP_ERROR PASESession::ValidateReceivedMessage(ExchangeContext * exchange, cons void PASESession::OnMessageReceived(ExchangeContext * exchange, const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, System::PacketBufferHandle && msg) { - CHIP_ERROR err = ValidateReceivedMessage(exchange, packetHeader, payloadHeader, msg); + CHIP_ERROR err = ValidateReceivedMessage(exchange, packetHeader, payloadHeader, std::move(msg)); SuccessOrExit(err); mConnectionState.SetPeerAddress(mMessageDispatch.GetPeerAddress()); diff --git a/src/protocols/secure_channel/PASESession.h b/src/protocols/secure_channel/PASESession.h index 4cc8abe90773ee..cf86218aedf1b2 100644 --- a/src/protocols/secure_channel/PASESession.h +++ b/src/protocols/secure_channel/PASESession.h @@ -244,7 +244,7 @@ class DLL_EXPORT PASESession : public Messaging::ExchangeDelegateBase, public Pa CHIP_ERROR Init(uint16_t myKeyId, uint32_t setupCode, SessionEstablishmentDelegate * delegate); CHIP_ERROR ValidateReceivedMessage(Messaging::ExchangeContext * exchange, const PacketHeader & packetHeader, - const PayloadHeader & payloadHeader, System::PacketBufferHandle & msg); + const PayloadHeader & payloadHeader, System::PacketBufferHandle && msg); static CHIP_ERROR ComputePASEVerifier(uint32_t mySetUpPINCode, uint32_t pbkdf2IterCount, const uint8_t * salt, size_t saltLen, PASEVerifier & verifier); @@ -266,6 +266,8 @@ class DLL_EXPORT PASESession : public Messaging::ExchangeDelegateBase, public Pa void SendErrorMsg(Spake2pErrorType errorCode); CHIP_ERROR HandleErrorMsg(const System::PacketBufferHandle & msg); + void CloseExchange(); + SessionEstablishmentDelegate * mDelegate = nullptr; Protocols::SecureChannel::MsgType mNextExpectedMsg = Protocols::SecureChannel::MsgType::PASE_Spake2pError; From 97db28bf7d28c3ccd3cdfdf28f91416b69d30868 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 25 May 2021 08:41:20 -0700 Subject: [PATCH 3/5] Address review comments --- src/lib/support/logging/CHIPLogging.cpp | 2 - src/lib/support/logging/Constants.h | 2 - src/protocols/secure_channel/PASESession.cpp | 46 +++++++++++--------- 3 files changed, 25 insertions(+), 25 deletions(-) diff --git a/src/lib/support/logging/CHIPLogging.cpp b/src/lib/support/logging/CHIPLogging.cpp index 1a51e17909e456..d70009e210b10f 100644 --- a/src/lib/support/logging/CHIPLogging.cpp +++ b/src/lib/support/logging/CHIPLogging.cpp @@ -91,8 +91,6 @@ static const char ModuleNames[] = "-\0\0" // None "SPL" // SetupPayload "SVR" // AppServer "DIS" // Discovery - "PAS" // PASE - "CAS" // CASE ; #define ModuleNamesCount ((sizeof(ModuleNames) - 1) / chip::Logging::kMaxModuleNameLen) diff --git a/src/lib/support/logging/Constants.h b/src/lib/support/logging/Constants.h index 5b7f452b97ca67..c605e2fd93214b 100644 --- a/src/lib/support/logging/Constants.h +++ b/src/lib/support/logging/Constants.h @@ -54,8 +54,6 @@ enum LogModule kLogModule_SetupPayload, kLogModule_AppServer, kLogModule_Discovery, - kLogModule_PASE, - kLogModule_CASE, kLogModule_Max }; diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 28ba609791a616..0b1bba57a1e984 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -189,7 +189,7 @@ CHIP_ERROR PASESession::Init(uint16_t myKeyId, uint32_t setupCode, SessionEstabl mDelegate = delegate; - ChipLogDetail(PASE, "Assigned local session key ID %d", myKeyId); + ChipLogDetail(SecureChannel, "Assigned local session key ID %d", myKeyId); mConnectionState.SetLocalKeyID(myKeyId); mSetupPINCode = setupCode; mComputeVerifier = true; @@ -275,7 +275,7 @@ CHIP_ERROR PASESession::WaitForPairing(uint32_t mySetUpPINCode, uint32_t pbkdf2I mNextExpectedMsg = Protocols::SecureChannel::MsgType::PBKDFParamRequest; mPairingComplete = false; - ChipLogDetail(PASE, "Waiting for PBKDF param request"); + ChipLogDetail(SecureChannel, "Waiting for PBKDF param request"); exit: if (err != CHIP_NO_ERROR) @@ -322,10 +322,10 @@ CHIP_ERROR PASESession::Pair(const Transport::PeerAddress peerAddress, uint32_t void PASESession::OnResponseTimeout(ExchangeContext * ec) { - VerifyOrReturn(ec != nullptr, ChipLogError(PASE, "PASESession::OnResponseTimeout was called by null exchange")); + VerifyOrReturn(ec != nullptr, ChipLogError(SecureChannel, "PASESession::OnResponseTimeout was called by null exchange")); VerifyOrReturn(mExchangeCtxt == nullptr || mExchangeCtxt == ec, - ChipLogError(PASE, "PASESession::OnResponseTimeout exchange doesn't match")); - ChipLogError(PASE, "PASESession timed out while waiting for a response from the peer. Expected message type was %d", + ChipLogError(SecureChannel, "PASESession::OnResponseTimeout exchange doesn't match")); + ChipLogError(SecureChannel, "PASESession timed out while waiting for a response from the peer. Expected message type was %d", mNextExpectedMsg); mDelegate->OnSessionEstablishmentError(CHIP_ERROR_TIMEOUT); Clear(); @@ -355,7 +355,7 @@ CHIP_ERROR PASESession::SendPBKDFParamRequest() ReturnErrorOnFailure(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PBKDFParamRequest, std::move(req), SendFlags(SendMessageFlags::kExpectResponse))); - ChipLogDetail(PASE, "Sent PBKDF param request"); + ChipLogDetail(SecureChannel, "Sent PBKDF param request"); return CHIP_NO_ERROR; } @@ -371,7 +371,7 @@ CHIP_ERROR PASESession::HandlePBKDFParamRequest(const System::PacketBufferHandle VerifyOrExit(req != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(reqlen == kPBKDFParamRandomNumberSize, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); - ChipLogDetail(PASE, "Received PBKDF param request"); + ChipLogDetail(SecureChannel, "Received PBKDF param request"); // Update commissioning hash with the received pbkdf2 param request err = mCommissioningHash.AddData(req, reqlen); @@ -427,7 +427,7 @@ CHIP_ERROR PASESession::SendPBKDFParamResponse() ReturnErrorOnFailure(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PBKDFParamResponse, std::move(resp), SendFlags(SendMessageFlags::kExpectResponse))); - ChipLogDetail(PASE, "Sent PBKDF param response"); + ChipLogDetail(SecureChannel, "Sent PBKDF param response"); return CHIP_NO_ERROR; } @@ -445,7 +445,7 @@ CHIP_ERROR PASESession::HandlePBKDFParamResponse(const System::PacketBufferHandl static_assert(CHAR_BIT == 8, "Assuming that sizeof returns octets"); size_t fixed_resplen = kPBKDFParamRandomNumberSize + sizeof(uint64_t) + sizeof(uint32_t); - ChipLogDetail(PASE, "Received PBKDF param response"); + ChipLogDetail(SecureChannel, "Received PBKDF param response"); VerifyOrExit(resp != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(resplen >= fixed_resplen, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); @@ -502,7 +502,7 @@ CHIP_ERROR PASESession::SendMsg1() // Call delegate to send the Msg1 to peer ReturnErrorOnFailure(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PASE_Spake2p1, bbuf.Finalize(), SendFlags(SendMessageFlags::kExpectResponse))); - ChipLogDetail(PASE, "Sent spake2p msg1"); + ChipLogDetail(SecureChannel, "Sent spake2p msg1"); return CHIP_NO_ERROR; } @@ -524,7 +524,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const System::PacketBufferHandle uint16_t encryptionKeyId = 0; - ChipLogDetail(PASE, "Received spake2p msg1"); + ChipLogDetail(SecureChannel, "Received spake2p msg1"); VerifyOrExit(buf != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(buf_len == sizeof(encryptionKeyId) + kMAX_Point_Length, err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); @@ -539,7 +539,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const System::PacketBufferHandle err = mSpake2p.ComputeRoundOne(msg->Start(), msg->DataLength(), Y, &Y_len); SuccessOrExit(err); - ChipLogDetail(PASE, "Peer assigned session key ID %d", encryptionKeyId); + ChipLogDetail(SecureChannel, "Peer assigned session key ID %d", encryptionKeyId); mConnectionState.SetPeerKeyID(encryptionKeyId); err = mSpake2p.ComputeRoundTwo(msg->Start(), msg->DataLength(), verifier, &verifier_len); @@ -566,7 +566,7 @@ CHIP_ERROR PASESession::HandleMsg1_and_SendMsg2(const System::PacketBufferHandle SuccessOrExit(err); } - ChipLogDetail(PASE, "Sent spake2p msg2"); + ChipLogDetail(SecureChannel, "Sent spake2p msg2"); exit: @@ -594,7 +594,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle uint16_t encryptionKeyId = 0; - ChipLogDetail(PASE, "Received spake2p msg2"); + ChipLogDetail(SecureChannel, "Received spake2p msg2"); VerifyOrExit(buf != nullptr, err = CHIP_ERROR_MESSAGE_INCOMPLETE); VerifyOrExit(buf_len == sizeof(encryptionKeyId) + kMAX_Point_Length + kMAX_Hash_Length, @@ -605,7 +605,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle buf = msg->Start(); buf_len = msg->DataLength(); - ChipLogDetail(PASE, "Peer assigned session key ID %d", encryptionKeyId); + ChipLogDetail(SecureChannel, "Peer assigned session key ID %d", encryptionKeyId); mConnectionState.SetPeerKeyID(encryptionKeyId); err = mSpake2p.ComputeRoundTwo(buf, kMAX_Point_Length, verifier, &verifier_len_raw); @@ -625,7 +625,7 @@ CHIP_ERROR PASESession::HandleMsg2_and_SendMsg3(const System::PacketBufferHandle SuccessOrExit(err); } - ChipLogDetail(PASE, "Sent spake2p msg3"); + ChipLogDetail(SecureChannel, "Sent spake2p msg3"); { const uint8_t * hash = &buf[kMAX_Point_Length]; @@ -663,7 +663,7 @@ CHIP_ERROR PASESession::HandleMsg3(const System::PacketBufferHandle & msg) const uint8_t * hash = msg->Start(); Spake2pErrorType spake2pErr = Spake2pErrorType::kUnexpected; - ChipLogDetail(PASE, "Received spake2p msg3"); + ChipLogDetail(SecureChannel, "Received spake2p msg3"); // We will set NextExpectedMsg to PASE_Spake2pError in all cases // However, when we are using IP rendezvous, we might set it to PASE_Spake2p1. @@ -706,7 +706,7 @@ void PASESession::SendErrorMsg(Spake2pErrorType errorCode) Spake2pErrorMsg * pMsg = nullptr; msg = System::PacketBufferHandle::New(msglen); - VerifyOrReturn(!msg.IsNull(), ChipLogError(PASE, "Failed to allocate error message")); + VerifyOrReturn(!msg.IsNull(), ChipLogError(SecureChannel, "Failed to allocate error message")); pMsg = reinterpret_cast(msg->Start()); pMsg->error = errorCode; @@ -714,15 +714,19 @@ void PASESession::SendErrorMsg(Spake2pErrorType errorCode) msg->SetDataLength(msglen); VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PASE_Spake2pError, std::move(msg)), - ChipLogError(PASE, "Failed to send error message")); + ChipLogError(SecureChannel, "Failed to send error message")); } CHIP_ERROR PASESession::HandleErrorMsg(const System::PacketBufferHandle & msg) { ReturnErrorCodeIf(msg->Start() == nullptr || msg->DataLength() != sizeof(Spake2pErrorMsg), CHIP_ERROR_MESSAGE_INCOMPLETE); + static_assert( + sizeof(Spake2pErrorMsg) == sizeof(uint8_t), + "Assuming size of Spake2pErrorMsg message is 1 octet, so that endian-ness conversion and memory alignment is not needed"); + Spake2pErrorMsg * pMsg = reinterpret_cast(msg->Start()); - ChipLogError(PASE, "Received error during pairing process. %s", ErrorStr(pMsg->error)); + ChipLogError(SecureChannel, "Received error during pairing process. %s", ErrorStr(pMsg->error)); return pMsg->error; } @@ -803,7 +807,7 @@ void PASESession::OnMessageReceived(ExchangeContext * exchange, const PacketHead if (err != CHIP_NO_ERROR) { Clear(); - ChipLogError(PASE, "Failed during PASE session setup. %s", ErrorStr(err)); + ChipLogError(SecureChannel, "Failed during PASE session setup. %s", ErrorStr(err)); mDelegate->OnSessionEstablishmentError(err); } } From 7f9bed6ac41c3e76326efa0721dde06d4cb51ade Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 25 May 2021 09:11:44 -0700 Subject: [PATCH 4/5] fix error check --- src/protocols/secure_channel/PASESession.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 0b1bba57a1e984..864a93998d5951 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -713,7 +713,8 @@ void PASESession::SendErrorMsg(Spake2pErrorType errorCode) msg->SetDataLength(msglen); - VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PASE_Spake2pError, std::move(msg)), + VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::PASE_Spake2pError, std::move(msg)) == + CHIP_NO_ERROR, ChipLogError(SecureChannel, "Failed to send error message")); } From 9d6a6d3b32afc926efb0d1afc399c75b3dca8916 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Tue, 25 May 2021 14:22:47 -0700 Subject: [PATCH 5/5] address review comments --- src/protocols/secure_channel/PASESession.cpp | 21 +++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index 864a93998d5951..433e027bdeb14a 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -250,10 +250,13 @@ CHIP_ERROR PASESession::SetupSpake2p(uint32_t pbkdf2IterCount, const uint8_t * s CHIP_ERROR PASESession::WaitForPairing(uint32_t mySetUpPINCode, uint32_t pbkdf2IterCount, const uint8_t * salt, size_t saltLen, uint16_t myKeyId, SessionEstablishmentDelegate * delegate) { + // Return early on error here, as we have not initalized any state yet ReturnErrorCodeIf(salt == nullptr, CHIP_ERROR_INVALID_ARGUMENT); ReturnErrorCodeIf(saltLen == 0, CHIP_ERROR_INVALID_ARGUMENT); CHIP_ERROR err = Init(myKeyId, mySetUpPINCode, delegate); + // From here onwards, let's go to exit on error, as some state might have already + // been initialized SuccessOrExit(err); VerifyOrExit(CanCastTo(saltLen), err = CHIP_ERROR_INVALID_ARGUMENT); @@ -729,7 +732,23 @@ CHIP_ERROR PASESession::HandleErrorMsg(const System::PacketBufferHandle & msg) Spake2pErrorMsg * pMsg = reinterpret_cast(msg->Start()); ChipLogError(SecureChannel, "Received error during pairing process. %s", ErrorStr(pMsg->error)); - return pMsg->error; + CHIP_ERROR err = CHIP_NO_ERROR; + switch (pMsg->error) + { + case Spake2pErrorType::kInvalidKeyConfirmation: + err = CHIP_ERROR_KEY_CONFIRMATION_FAILED; + break; + + case Spake2pErrorType::kUnexpected: + err = CHIP_ERROR_INVALID_PASE_PARAMETER; + break; + + default: + err = CHIP_ERROR_INTERNAL; + break; + }; + + return err; } CHIP_ERROR PASESession::ValidateReceivedMessage(ExchangeContext * exchange, const PacketHeader & packetHeader,