From edb7f05047c8367f6f70a56a889dad167f2bc4da Mon Sep 17 00:00:00 2001 From: Eduardo Montoya Date: Tue, 11 Jul 2023 22:18:40 +0200 Subject: [PATCH] [mle] ensure key sequence is updated in MLE responses (#9271) Processing of the Key Sequence is happening after each individual MLE command processing, which leads to generating MLE responses with outdated Key Sequence. Make sure that the new greater Key Sequence is applied before generating any MLE response. A test case is updated to catch the case in which fragmented Child Id Response was generated with the old Key Sequence whereas each individual MAC fragment is already updated with the new Key Sequence, leading to a security error. --- src/core/thread/mle.cpp | 90 ++++++++++--------- src/core/thread/mle.hpp | 1 + src/core/thread/mle_router.cpp | 5 ++ .../thread-cert/test_mle_msg_key_seq_jump.py | 24 +++++ 4 files changed, 79 insertions(+), 41 deletions(-) diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index 8ce0a35d4..e64483b36 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -2657,47 +2657,7 @@ void Mle::HandleUdpReceive(Message &aMessage, const Ip6::MessageInfo &aMessageIn ExitNow(error = kErrorDrop); } - // In case key sequence is larger, we determine whether to adopt it - // or not. The `Handle{MleMsg}()` methods set the `rxInfo.mClass` - // based on the message command type and the included TLVs. If - // there is any error during parsing of the message the `mClass` - // remains as its default value of `RxInfo::kUnknown`. Message - // classes are determined based on this: - // - // Authoritative : Larger key seq MUST be adopted. - // Peer : If from a known neighbor - // If difference is 1, adopt - // Otherwise don't adopt and try to re-sync with - // neighbor. - // Otherwise larger key seq MUST NOT be adopted. - - if (keySequence > Get().GetCurrentKeySequence()) - { - switch (rxInfo.mClass) - { - case RxInfo::kAuthoritativeMessage: - Get().SetCurrentKeySequence(keySequence); - break; - - case RxInfo::kPeerMessage: - if ((neighbor != nullptr) && neighbor->IsStateValid()) - { - if (keySequence - Get().GetCurrentKeySequence() == 1) - { - Get().SetCurrentKeySequence(keySequence); - } - else - { - LogInfo("Large key seq jump in peer class msg from 0x%04x ", neighbor->GetRloc16()); - ReestablishLinkWithNeighbor(*neighbor); - } - } - break; - - case RxInfo::kUnknown: - break; - } - } + ProcessKeySequence(rxInfo); #if OPENTHREAD_CONFIG_MULTI_RADIO // If we could not find a neighbor matching the MAC address of the @@ -2730,6 +2690,53 @@ exit: } } +void Mle::ProcessKeySequence(RxInfo &aRxInfo) +{ + // In case key sequence is larger, we determine whether to adopt it + // or not. The `Handle{MleMsg}()` methods set the `rxInfo.mClass` + // based on the message command type and the included TLVs. If + // there is any error during parsing of the message the `mClass` + // remains as its default value of `RxInfo::kUnknown`. Message + // classes are determined based on this: + // + // Authoritative : Larger key seq MUST be adopted. + // Peer : If from a known neighbor + // If difference is 1, adopt + // Otherwise don't adopt and try to re-sync with + // neighbor. + // Otherwise larger key seq MUST NOT be adopted. + + VerifyOrExit(aRxInfo.mKeySequence > Get().GetCurrentKeySequence()); + + switch (aRxInfo.mClass) + { + case RxInfo::kAuthoritativeMessage: + Get().SetCurrentKeySequence(aRxInfo.mKeySequence); + break; + + case RxInfo::kPeerMessage: + if ((aRxInfo.mNeighbor != nullptr) && aRxInfo.mNeighbor->IsStateValid()) + { + if (aRxInfo.mKeySequence - Get().GetCurrentKeySequence() == 1) + { + Get().SetCurrentKeySequence(aRxInfo.mKeySequence); + } + else + { + LogInfo("Large key seq jump in peer class msg from 0x%04x ", aRxInfo.mNeighbor->GetRloc16()); + ReestablishLinkWithNeighbor(*aRxInfo.mNeighbor); + } + } + break; + + case RxInfo::kUnknown: + break; + } + +exit: + return; +} + void Mle::ReestablishLinkWithNeighbor(Neighbor &aNeighbor) { VerifyOrExit(IsAttached() && aNeighbor.IsStateValid()); @@ -3521,6 +3528,7 @@ void Mle::HandleChildUpdateRequest(RxInfo &aRxInfo) } aRxInfo.mClass = RxInfo::kPeerMessage; + ProcessKeySequence(aRxInfo); #if OPENTHREAD_CONFIG_MULTI_RADIO if ((aRxInfo.mNeighbor != nullptr) && (challenge.mLength != 0)) diff --git a/src/core/thread/mle.hpp b/src/core/thread/mle.hpp index 5e7a96087..9ebb07dd2 100644 --- a/src/core/thread/mle.hpp +++ b/src/core/thread/mle.hpp @@ -1738,6 +1738,7 @@ protected: #endif void ScheduleMessageTransmissionTimer(void); + void ProcessKeySequence(RxInfo &aRxInfo); private: // Declare early so we can use in as `TimerMilli` callbacks. diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp index cd2c3129e..944ebad76 100644 --- a/src/core/thread/mle_router.cpp +++ b/src/core/thread/mle_router.cpp @@ -730,6 +730,7 @@ void MleRouter::HandleLinkRequest(RxInfo &aRxInfo) #endif aRxInfo.mClass = RxInfo::kPeerMessage; + ProcessKeySequence(aRxInfo); SuccessOrExit(error = SendLinkAccept(aRxInfo.mMessageInfo, neighbor, requestedTlvList, challenge)); @@ -1025,6 +1026,7 @@ Error MleRouter::HandleLinkAccept(RxInfo &aRxInfo, bool aRequest) mNeighborTable.Signal(NeighborTable::kRouterAdded, *router); aRxInfo.mClass = RxInfo::kAuthoritativeMessage; + ProcessKeySequence(aRxInfo); if (aRequest) { @@ -1476,6 +1478,7 @@ void MleRouter::HandleParentRequest(RxInfo &aRxInfo) } aRxInfo.mClass = RxInfo::kPeerMessage; + ProcessKeySequence(aRxInfo); SendParentResponse(child, challenge, !ScanMaskTlv::IsEndDeviceFlagSet(scanMask)); @@ -2215,6 +2218,7 @@ void MleRouter::HandleChildIdRequest(RxInfo &aRxInfo) } aRxInfo.mClass = RxInfo::kAuthoritativeMessage; + ProcessKeySequence(aRxInfo); switch (mRole) { @@ -2671,6 +2675,7 @@ void MleRouter::HandleDataRequest(RxInfo &aRxInfo) } aRxInfo.mClass = RxInfo::kPeerMessage; + ProcessKeySequence(aRxInfo); SendDataResponse(aRxInfo.mMessageInfo.GetPeerAddr(), tlvList, /* aDelay */ 0, &aRxInfo.mMessage); diff --git a/tests/scripts/thread-cert/test_mle_msg_key_seq_jump.py b/tests/scripts/thread-cert/test_mle_msg_key_seq_jump.py index 90f1b2e26..d40055744 100755 --- a/tests/scripts/thread-cert/test_mle_msg_key_seq_jump.py +++ b/tests/scripts/thread-cert/test_mle_msg_key_seq_jump.py @@ -236,6 +236,30 @@ class MleMsgKeySeqJump(thread_cert.TestCase): self.simulator.go(2) self.assertEqual(child.get_key_sequence_counter(), 21) + #------------------------------------------------------------------- + # Force a reattachment from the child with a higher key seq counter, + # so that the leader generated a fragmented Child Id Response. Ensure + # the child becomes attached on first attempt while the leader adopts + # the higher counter value. + + router.stop() + reed.stop() + + child.factory_reset() + self.assertEqual(child.get_state(), 'disabled') + + child.set_active_dataset(channel=leader.get_channel(), + network_key=leader.get_networkkey(), + panid=leader.get_panid()) + child.set_key_sequence_counter(25) + self.assertEqual(child.get_key_sequence_counter(), 25) + + child.start() + self.simulator.go(2) + + self.assertEqual(child.get_state(), 'child') + self.assertEqual(leader.get_key_sequence_counter(), 25) + if __name__ == '__main__': unittest.main()