From 3920781c2e52e10c6f60718e32bc93579365178c Mon Sep 17 00:00:00 2001 From: Michael Sandstedt Date: Tue, 3 May 2022 08:33:24 -0500 Subject: [PATCH] Change mrp-parameter-struct to hold 32-bit milliseconds (#17978) * Change mrp-parameter-struct to hold 32-bit milliseconds Nodes store and advertise MRP parameters as 32-bit values. However, the mrp-parameter-struct had been specified to only hold 16-bit values on the wire. This would lead to session establishment failures with nodes attempting to exchange values in excess of 65536 milliseconds, despite the fact that values up to 360,000 milliseconds are legal. This corrects the problem to allow up-to 32-bit values per the spec change here: https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5173 In most cases, peers will be using smaller MRP values and and will therefore still exchange 1 or 2-byte fields on the wire, making this change mostly backward compatible. Testing: verification of successful exchange of larger MRP values up to 360,000 has been added to TestCASESession.cpp. TestTxtFields.cpp already has coverage for advertisement of large values. Fixes #17812 * per bzbarsky, s/verySleep/verySleepy --- .../secure_channel/tests/TestCASESession.cpp | 16 +++++++++++++--- src/transport/PairingSession.cpp | 9 +++------ 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/protocols/secure_channel/tests/TestCASESession.cpp b/src/protocols/secure_channel/tests/TestCASESession.cpp index ecc5c2cc9ef6ce..11a4e6928af9fc 100644 --- a/src/protocols/secure_channel/tests/TestCASESession.cpp +++ b/src/protocols/secure_channel/tests/TestCASESession.cpp @@ -258,6 +258,10 @@ void CASE_SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inConte // Test all combinations of invalid parameters TestCASESecurePairingDelegate delegateAccessory; CASESession pairingAccessory; + ReliableMessageProtocolConfig verySleepyAccessoryRmpConfig(System::Clock::Milliseconds32(360000), + System::Clock::Milliseconds32(100000)); + ReliableMessageProtocolConfig nonSleepyCommissionerRmpConfig(System::Clock::Milliseconds32(5000), + System::Clock::Milliseconds32(300)); gLoopback.mSentMessageCount = 0; @@ -272,16 +276,22 @@ void CASE_SecurePairingHandshakeTestCommon(nlTestSuite * inSuite, void * inConte pairingAccessory.SetGroupDataProvider(&gDeviceGroupDataProvider); NL_TEST_ASSERT(inSuite, - pairingAccessory.ListenForSessionEstablishment(sessionManager, &gDeviceFabrics, nullptr, &delegateAccessory) == - CHIP_NO_ERROR); + pairingAccessory.ListenForSessionEstablishment(sessionManager, &gDeviceFabrics, nullptr, &delegateAccessory, + MakeOptional(verySleepyAccessoryRmpConfig)) == CHIP_NO_ERROR); NL_TEST_ASSERT(inSuite, pairingCommissioner.EstablishSession(sessionManager, fabric, Node01_01, contextCommissioner, nullptr, - &delegateCommissioner) == CHIP_NO_ERROR); + &delegateCommissioner, + MakeOptional(nonSleepyCommissionerRmpConfig)) == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); NL_TEST_ASSERT(inSuite, gLoopback.mSentMessageCount == 5); 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)); + NL_TEST_ASSERT(inSuite, pairingAccessory.GetRemoteMRPConfig().mActiveRetransTimeout == System::Clock::Milliseconds32(300)); + NL_TEST_ASSERT(inSuite, pairingCommissioner.GetRemoteMRPConfig().mIdleRetransTimeout == System::Clock::Milliseconds32(360000)); + NL_TEST_ASSERT(inSuite, + pairingCommissioner.GetRemoteMRPConfig().mActiveRetransTimeout == System::Clock::Milliseconds32(100000)); } void CASE_SecurePairingHandshakeTest(nlTestSuite * inSuite, void * inContext) diff --git a/src/transport/PairingSession.cpp b/src/transport/PairingSession.cpp index 6eef6c538b95cc..ac98f4dc7a7421 100644 --- a/src/transport/PairingSession.cpp +++ b/src/transport/PairingSession.cpp @@ -60,13 +60,10 @@ CHIP_ERROR PairingSession::ActivateSecureSession(const Transport::PeerAddress & CHIP_ERROR PairingSession::EncodeMRPParameters(TLV::Tag tag, const ReliableMessageProtocolConfig & mrpConfig, TLV::TLVWriter & tlvWriter) { - VerifyOrReturnError(CanCastTo(mrpConfig.mIdleRetransTimeout.count()), CHIP_ERROR_INVALID_ARGUMENT); - VerifyOrReturnError(CanCastTo(mrpConfig.mActiveRetransTimeout.count()), CHIP_ERROR_INVALID_ARGUMENT); - TLV::TLVType mrpParamsContainer; ReturnErrorOnFailure(tlvWriter.StartContainer(tag, TLV::kTLVType_Structure, mrpParamsContainer)); - ReturnErrorOnFailure(tlvWriter.Put(TLV::ContextTag(1), static_cast(mrpConfig.mIdleRetransTimeout.count()))); - ReturnErrorOnFailure(tlvWriter.Put(TLV::ContextTag(2), static_cast(mrpConfig.mActiveRetransTimeout.count()))); + ReturnErrorOnFailure(tlvWriter.Put(TLV::ContextTag(1), mrpConfig.mIdleRetransTimeout.count())); + ReturnErrorOnFailure(tlvWriter.Put(TLV::ContextTag(2), mrpConfig.mActiveRetransTimeout.count())); return tlvWriter.EndContainer(mrpParamsContainer); } @@ -81,7 +78,7 @@ CHIP_ERROR PairingSession::DecodeMRPParametersIfPresent(TLV::Tag expectedTag, TL TLV::TLVType containerType = TLV::kTLVType_Structure; ReturnErrorOnFailure(tlvReader.EnterContainer(containerType)); - uint16_t tlvElementValue = 0; + uint32_t tlvElementValue = 0; ReturnErrorOnFailure(tlvReader.Next());