Skip to content

Commit

Permalink
Change mrp-parameter-struct to hold 32-bit milliseconds
Browse files Browse the repository at this point in the history
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
  • Loading branch information
msandstedt committed May 2, 2022
1 parent 3c5b7ab commit 2a87ab1
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 verySleepAccessoryRmpConfig(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(verySleepAccessoryRmpConfig)) == 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 2a87ab1

Please sign in to comment.