[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

@@ -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<KeyManager>().GetCurrentKeySequence())
-    {
-        switch (rxInfo.mClass)
-        {
-        case RxInfo::kAuthoritativeMessage:
-            Get<KeyManager>().SetCurrentKeySequence(keySequence);
-            break;
-
-        case RxInfo::kPeerMessage:
-            if ((neighbor != nullptr) && neighbor->IsStateValid())
-            {
-                if (keySequence - Get<KeyManager>().GetCurrentKeySequence() == 1)
-                {
-                    Get<KeyManager>().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<KeyManager>().GetCurrentKeySequence());
+
+    switch (aRxInfo.mClass)
+    {
+    case RxInfo::kAuthoritativeMessage:
+        Get<KeyManager>().SetCurrentKeySequence(aRxInfo.mKeySequence);
+        break;
+
+    case RxInfo::kPeerMessage:
+        if ((aRxInfo.mNeighbor != nullptr) && aRxInfo.mNeighbor->IsStateValid())
+        {
+            if (aRxInfo.mKeySequence - Get<KeyManager>().GetCurrentKeySequence() == 1)
+            {
+                Get<KeyManager>().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))

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.

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);
 

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()