From 3b67fa9d7284018d89f78d0c792e06d4180c5a25 Mon Sep 17 00:00:00 2001 From: Martin Turon Date: Wed, 1 Jun 2022 10:07:34 -0700 Subject: [PATCH] [mrp] Fix #17471 - send standalone ack before crypto ops in CASE and PASE. (#18773) * [mrp] Fix #17471 - force send standalone ack for CASE. * [mrp] Adjust expected # of packets in CASE test. * [mrp] Add CHIP_CONFIG_SLOW_CRYPTO and support to standalone ack for PASE. * [mrp] Move CHIP_DEVICE_CONFIG_SLOW_CRYPTO to device layer config. * [mrp] Only send slow crypto ack for crypto ops. * [mrp] Call FlushAcks rather than SendStandaloneAckMessage. Co-authored-by: Boris Zbarsky * [mrp] Assume fast crypto on linux and darwin. Co-authored-by: Boris Zbarsky --- src/lib/core/CHIPConfig.h | 11 +++++++ src/platform/Darwin/CHIPPlatformConfig.h | 5 ++++ src/platform/Linux/CHIPPlatformConfig.h | 5 ++++ src/protocols/secure_channel/CASESession.cpp | 9 ++++++ src/protocols/secure_channel/PASESession.cpp | 13 ++++++-- .../secure_channel/tests/TestCASESession.cpp | 30 ++++++++++++------- .../secure_channel/tests/TestPASESession.cpp | 8 ++++- 7 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 632265f1f30c1c..d4d4dbb1597a7f 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -688,6 +688,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. + */ +#ifndef CHIP_CONFIG_SLOW_CRYPTO +#define CHIP_CONFIG_SLOW_CRYPTO 1 +#endif // CHIP_CONFIG_SLOW_CRYPTO + /** * @def CHIP_NON_PRODUCTION_MARKER * diff --git a/src/platform/Darwin/CHIPPlatformConfig.h b/src/platform/Darwin/CHIPPlatformConfig.h index 00107122d27d34..ea4252f93b1110 100644 --- a/src/platform/Darwin/CHIPPlatformConfig.h +++ b/src/platform/Darwin/CHIPPlatformConfig.h @@ -36,6 +36,11 @@ // ==================== Security Adaptations ==================== +// If unspecified, assume crypto is fast on Darwin +#ifndef CHIP_CONFIG_SLOW_CRYPTO +#define CHIP_CONFIG_SLOW_CRYPTO 0 +#endif // CHIP_CONFIG_SLOW_CRYPTO + // ==================== General Configuration Overrides ==================== #ifndef CHIP_CONFIG_MAX_UNSOLICITED_MESSAGE_HANDLERS diff --git a/src/platform/Linux/CHIPPlatformConfig.h b/src/platform/Linux/CHIPPlatformConfig.h index 4dcdd7d5418dd7..788fe9b80c75d9 100644 --- a/src/platform/Linux/CHIPPlatformConfig.h +++ b/src/platform/Linux/CHIPPlatformConfig.h @@ -41,6 +41,11 @@ using CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE = const char *; // ==================== Security Adaptations ==================== +// If unspecified, assume crypto is fast on Linux +#ifndef CHIP_CONFIG_SLOW_CRYPTO +#define CHIP_CONFIG_SLOW_CRYPTO 0 +#endif // CHIP_CONFIG_SLOW_CRYPTO + // ==================== General Configuration Overrides ==================== #ifndef CHIP_CONFIG_MAX_UNSOLICITED_MESSAGE_HANDLERS diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index ec6f106b0d231b..78e01a294a0cad 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -1643,6 +1643,15 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea Protocols::SecureChannel::MsgType msgType = static_cast(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) + { + SuccessOrExit(mExchangeCtxt->FlushAcks()); + } +#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; diff --git a/src/protocols/secure_channel/PASESession.cpp b/src/protocols/secure_channel/PASESession.cpp index c513f1d2808c99..d780c0821a55ff 100644 --- a/src/protocols/secure_channel/PASESession.cpp +++ b/src/protocols/secure_channel/PASESession.cpp @@ -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(payloadHeader.GetMessageType()); SuccessOrExit(err); - switch (static_cast(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->FlushAcks()); + } +#endif // CHIP_CONFIG_SLOW_CRYPTO + + switch (msgType) { case MsgType::PBKDFParamRequest: err = HandlePBKDFParamRequest(std::move(msg)); diff --git a/src/protocols/secure_channel/tests/TestCASESession.cpp b/src/protocols/secure_channel/tests/TestCASESession.cpp index 6772ba7a0c80f4..08d1adef9bc6f5 100644 --- a/src/protocols/secure_channel/tests/TestCASESession.cpp +++ b/src/protocols/secure_channel/tests/TestCASESession.cpp @@ -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; @@ -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)); @@ -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 @@ -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 }, }; diff --git a/src/protocols/secure_channel/tests/TestPASESession.cpp b/src/protocols/secure_channel/tests/TestPASESession.cpp index fe467e4a12eec2..5c4b5243f91368 100644 --- a/src/protocols/secure_channel/tests/TestPASESession.cpp +++ b/src/protocols/secure_channel/tests/TestPASESession.cpp @@ -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; @@ -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);