Skip to content

Commit

Permalink
Change mrp-parameter-struct to hold 32-bit milliseconds (#17978)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
msandstedt authored and pull[bot] committed Feb 1, 2024
1 parent 7323038 commit de254da
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 9 deletions.
16 changes: 13 additions & 3 deletions src/protocols/secure_channel/tests/TestCASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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)
Expand Down
9 changes: 3 additions & 6 deletions src/transport/PairingSession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint16_t>(mrpConfig.mIdleRetransTimeout.count()), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(CanCastTo<uint16_t>(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<uint16_t>(mrpConfig.mIdleRetransTimeout.count())));
ReturnErrorOnFailure(tlvWriter.Put(TLV::ContextTag(2), static_cast<uint16_t>(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);
}

Expand All @@ -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());

Expand Down

0 comments on commit de254da

Please sign in to comment.