Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mrp] Fix #17471 - send standalone ack before crypto ops in CASE and PASE. #18773

Merged
merged 7 commits into from
Jun 1, 2022
11 changes: 11 additions & 0 deletions src/lib/core/CHIPConfig.h
Original file line number Diff line number Diff line change
Expand Up @@ -707,6 +707,17 @@
#define CHIP_CONFIG_MAX_GROUP_CONTROL_PEERS 2
#endif // CHIP_CONFIG_MAX_GROUP_CONTROL_PEER

/**
* @def CHIP_CONFIG_SLOW_CRYPTO
*
* @brief
* When enabled, CASE and PASE setup will proactively send standalone acknowledgements
* prior to engaging in crypto operations.
turon marked this conversation as resolved.
Show resolved Hide resolved
*/
#ifndef CHIP_CONFIG_SLOW_CRYPTO
#define CHIP_CONFIG_SLOW_CRYPTO 1
#endif // CHIP_CONFIG_SLOW_CRYPTO

/**
* @def CHIP_NON_PRODUCTION_MARKER
*
Expand Down
9 changes: 9 additions & 0 deletions src/protocols/secure_channel/CASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1642,6 +1642,15 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
Protocols::SecureChannel::MsgType msgType = static_cast<Protocols::SecureChannel::MsgType>(payloadHeader.GetMessageType());
SuccessOrExit(err);

#if CHIP_CONFIG_SLOW_CRYPTO
if (msgType == Protocols::SecureChannel::MsgType::CASE_Sigma1 || msgType == Protocols::SecureChannel::MsgType::CASE_Sigma2 ||
msgType == Protocols::SecureChannel::MsgType::CASE_Sigma2Resume ||
msgType == Protocols::SecureChannel::MsgType::CASE_Sigma3)
{
ReturnErrorOnFailure(mExchangeCtxt->SendStandaloneAckMessage());
turon marked this conversation as resolved.
Show resolved Hide resolved
}
#endif // CHIP_CONFIG_SLOW_CRYPTO

// By default, CHIP_ERROR_INVALID_MESSAGE_TYPE is returned if in the current state
// a message handler is not defined for the received message type.
err = CHIP_ERROR_INVALID_MESSAGE_TYPE;
Expand Down
13 changes: 11 additions & 2 deletions src/protocols/secure_channel/PASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -800,10 +800,19 @@ CHIP_ERROR PASESession::OnUnsolicitedMessageReceived(const PayloadHeader & paylo
CHIP_ERROR PASESession::OnMessageReceived(ExchangeContext * exchange, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && msg)
{
CHIP_ERROR err = ValidateReceivedMessage(exchange, payloadHeader, msg);
CHIP_ERROR err = ValidateReceivedMessage(exchange, payloadHeader, msg);
MsgType msgType = static_cast<MsgType>(payloadHeader.GetMessageType());
SuccessOrExit(err);

switch (static_cast<MsgType>(payloadHeader.GetMessageType()))
#if CHIP_CONFIG_SLOW_CRYPTO
if (msgType == MsgType::PBKDFParamRequest || msgType == MsgType::PBKDFParamResponse || msgType == MsgType::PASE_Pake1 ||
msgType == MsgType::PASE_Pake2 || msgType == MsgType::PASE_Pake3)
{
SuccessOrExit(mExchangeCtxt->SendStandaloneAckMessage());
turon marked this conversation as resolved.
Show resolved Hide resolved
}
#endif // CHIP_CONFIG_SLOW_CRYPTO

switch (msgType)
{
case MsgType::PBKDFParamRequest:
err = HandlePBKDFParamRequest(std::move(msg));
Expand Down
30 changes: 20 additions & 10 deletions src/protocols/secure_channel/tests/TestCASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,14 @@ using namespace chip::Protocols;
using TestContext = Test::LoopbackMessagingContext;

namespace {
#if CHIP_CONFIG_SLOW_CRYPTO
constexpr uint32_t sTestCaseMessageCount = 8;
constexpr uint32_t sTestCaseResumptionMessageCount = 6;
#else // CHIP_CONFIG_SLOW_CRYPTO
constexpr uint32_t sTestCaseMessageCount = 5;
constexpr uint32_t sTestCaseResumptionMessageCount = 4;
#endif // CHIP_CONFIG_SLOW_CRYPTO

FabricTable gCommissionerFabrics;
FabricIndex gCommissionerFabricIndex;
GroupDataProviderImpl gCommissionerGroupDataProvider;
Expand Down Expand Up @@ -283,7 +291,7 @@ void CASE_SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inConte
MakeOptional(nonSleepyCommissionerRmpConfig)) == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 5);
NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == sTestCaseMessageCount);
NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 1);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 1);
NL_TEST_ASSERT(inSuite, pairingAccessory.GetRemoteMRPConfig().mIdleRetransTimeout == System::Clock::Milliseconds32(5000));
Expand Down Expand Up @@ -335,7 +343,7 @@ void CASE_SecurePairingHandshakeServerTest(nlTestSuite * inSuite, void * inConte
nullptr, &delegateCommissioner) == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == 5);
NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount == sTestCaseMessageCount);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 1);

// Validate that secure session is created
Expand Down Expand Up @@ -712,30 +720,32 @@ static void CASE_SessionResumptionStorage(nlTestSuite * inSuite, void * inContex
// Both peers have a matching session resumption record.
// This should succeed.
{
.initiatorStorage = SessionResumptionTestStorage(CHIP_NO_ERROR, responder, &resumptionIdA, &sharedSecretA),
.responderStorage = SessionResumptionTestStorage(CHIP_NO_ERROR, initiator, &resumptionIdA, &sharedSecretA),
.expectedSentMessageCount = 4, // we expect this number of sent messages with successful session resumption
.initiatorStorage = SessionResumptionTestStorage(CHIP_NO_ERROR, responder, &resumptionIdA, &sharedSecretA),
.responderStorage = SessionResumptionTestStorage(CHIP_NO_ERROR, initiator, &resumptionIdA, &sharedSecretA),
.expectedSentMessageCount =
sTestCaseResumptionMessageCount, // we expect this number of sent messages with successful session resumption
},
// Peers have mismatched session resumption records.
// This should succeed with fall back to CASE.
{
.initiatorStorage = SessionResumptionTestStorage(CHIP_NO_ERROR, responder, &resumptionIdA, &sharedSecretA),
.responderStorage = SessionResumptionTestStorage(CHIP_ERROR_KEY_NOT_FOUND),
.expectedSentMessageCount = 5, // we expect this number of sent message when we fall back to CASE
.expectedSentMessageCount = sTestCaseMessageCount, // we expect this number of sent message when we fall back to CASE
},
// Peers both have record of the same resumption ID, but a different shared secret.
// This should succeed with fall back to CASE.
{
.initiatorStorage = SessionResumptionTestStorage(CHIP_NO_ERROR, responder, &resumptionIdA, &sharedSecretA),
.responderStorage = SessionResumptionTestStorage(CHIP_NO_ERROR, initiator, &resumptionIdA, &sharedSecretB),
.expectedSentMessageCount = 5, // we expect this number of sent message when we fall back to CASE
.expectedSentMessageCount = sTestCaseMessageCount, // we expect this number of sent message when we fall back to CASE
},
// Neither peer has a session resumption record.
// This should succeed - no attempt at session resumption will be made.
{
.initiatorStorage = SessionResumptionTestStorage(CHIP_ERROR_KEY_NOT_FOUND),
.responderStorage = SessionResumptionTestStorage(CHIP_ERROR_KEY_NOT_FOUND),
.expectedSentMessageCount = 5, // we expect this number of sent messages if we do not attempt session resumption
.initiatorStorage = SessionResumptionTestStorage(CHIP_ERROR_KEY_NOT_FOUND),
.responderStorage = SessionResumptionTestStorage(CHIP_ERROR_KEY_NOT_FOUND),
.expectedSentMessageCount =
sTestCaseMessageCount, // we expect this number of sent messages if we do not attempt session resumption
},
};

Expand Down
8 changes: 7 additions & 1 deletion src/protocols/secure_channel/tests/TestPASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ using namespace chip::Protocols;

namespace {

#if CHIP_CONFIG_SLOW_CRYPTO
constexpr uint32_t sTestPaseMessageCount = 8;
#else // CHIP_CONFIG_SLOW_CRYPTO
constexpr uint32_t sTestPaseMessageCount = 5;
#endif // CHIP_CONFIG_SLOW_CRYPTO

// Test Set #01 of Spake2p Parameters (PIN Code, Iteration Count, Salt, and matching Verifier):
constexpr uint32_t sTestSpake2p01_PinCode = 20202021;
constexpr uint32_t sTestSpake2p01_IterationCount = 1000;
Expand Down Expand Up @@ -250,7 +256,7 @@ void SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inContext, S
// via piggybacked acks. So we cannot check for a specific value of mSentMessageCount.
// Let's make sure atleast number is >= than the minimum messages required to complete the
// handshake.
NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount >= 5);
NL_TEST_ASSERT(inSuite, loopback.mSentMessageCount >= sTestPaseMessageCount);
NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 1);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 1);

Expand Down