From ef8deb506f93f0106eeedcb34bf1bfb9cd81268f Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Fri, 30 Jun 2023 15:09:50 -0700 Subject: [PATCH] [mle] delay router role downgrade on security policy change (#9187) This commit adds a mechanism to delay the downgrade of routers or leader when the security policy TLV changes in the Active Operational Dataset such that the device is no longer eligible to act as a router. If the decision to become a child is made due to a security policy change, the device first delays a random period up to the "router selection jitter" before downgrading. If the device is the leader, an additional fixed delay of 10 seconds is added to the random period. If the security policy changes again while the device is waiting to downgrade such that it becomes router-eligible again, the downgrade is cancelled and the device remains in its current role. This commit adds a `test_router_downgrade_on_sec_policy_change` to validate the behavior of newly added mechanism. This commit also updates the CLI `dataset` sub-commands to allow getting and setting the "version threshold for routing" (VR) field in security policy. --- src/cli/README_DATASET.md | 20 +-- src/cli/cli_dataset.cpp | 12 +- src/core/thread/mle.cpp | 7 + src/core/thread/mle_router.cpp | 32 +++- src/core/thread/mle_router.hpp | 2 + .../thread-cert/Cert_9_2_05_ActiveDataset.py | 12 +- tests/scripts/thread-cert/node.py | 18 ++- ...t_router_downgrade_on_sec_policy_change.py | 149 ++++++++++++++++++ tools/otci/otci/otci.py | 4 +- 9 files changed, 229 insertions(+), 27 deletions(-) create mode 100755 tests/scripts/thread-cert/test_router_downgrade_on_sec_policy_change.py diff --git a/src/cli/README_DATASET.md b/src/cli/README_DATASET.md index a1882af40..cba2b9bc5 100644 --- a/src/cli/README_DATASET.md +++ b/src/cli/README_DATASET.md @@ -44,7 +44,7 @@ The Pending Operational Dataset is used to communicate changes to the Active Ope Network Name: OpenThread-5938 PAN ID: 0x5938 PSKc: 3ca67c969efb0d0c74a4d8ee923b576c - Security Policy: 672 onrc + Security Policy: 672 onrc 0 Done ``` @@ -103,7 +103,7 @@ After the device successfully attaches to a Thread network, the device will retr Network Name: OpenThread-5938 PAN ID: 0x5938 PSKc: 3ca67c969efb0d0c74a4d8ee923b576c - Security Policy: 672 onrc + Security Policy: 672 onrc 0 Done ``` @@ -183,7 +183,7 @@ Network Key: f366cec7a446bab978d90d27abe38f23 Network Name: OpenThread-5938 PAN ID: 0x5938 PSKc: 3ca67c969efb0d0c74a4d8ee923b576c -Security Policy: 672 onrc +Security Policy: 672 onrc 0 Done ``` @@ -362,7 +362,7 @@ Usage: `dataset mgmtsetcommand [TLV Type list] [-x]` Send MGMT_ACTIVE_SET or MGMT_PENDING_SET. ```bash -> dataset mgmtsetcommand active activetimestamp 123 securitypolicy 1 onrc +> dataset mgmtsetcommand active activetimestamp 123 securitypolicy 1 onrc 0 Done ``` @@ -444,7 +444,7 @@ Network Key: ed916e454d96fd00184f10a6f5c9e1d3 Network Name: OpenThread-bff8 PAN ID: 0xbff8 PSKc: 264f78414adc683191863d968f72d1b7 -Security Policy: 672 onrc +Security Policy: 672 onrc 0 Done ``` @@ -500,13 +500,13 @@ Done ### securitypolicy -Usage: `dataset securitypolicy [ [onrcCepR]]` +Usage: `dataset securitypolicy [ [onrcCepR] [versionthreshold]]` Get security policy. ```bash > dataset securitypolicy -672 onrc +672 onrc 0 Done ``` @@ -521,8 +521,10 @@ Set security policy. - p: Thread 1.2 Network Key Provisioning is enabled. - R: Non-CCM routers are allowed in Thread 1.2 CCM networks. +If the `versionthreshold` parameter is not provided, a default value of zero is assumed. + ```bash -> dataset securitypolicy 672 onrc +> dataset securitypolicy 672 onrc 0 Done ``` @@ -561,7 +563,7 @@ Network Key: 9929154dbc363218bcd22f907caf5c15 Network Name: OpenThread-de2b PAN ID: 0xde2b PSKc: 15b2c16f7ba92ed4bc7b1ee054f1553f -Security Policy: 672 onrc +Security Policy: 672 onrc 0 Done > dataset tlvs diff --git a/src/cli/cli_dataset.cpp b/src/cli/cli_dataset.cpp index 633fa5b12..e74ec635a 100644 --- a/src/cli/cli_dataset.cpp +++ b/src/cli/cli_dataset.cpp @@ -1093,7 +1093,7 @@ exit: void Dataset::OutputSecurityPolicy(const otSecurityPolicy &aSecurityPolicy) { - OutputFormat("%d ", aSecurityPolicy.mRotationTime); + OutputFormat("%u ", aSecurityPolicy.mRotationTime); if (aSecurityPolicy.mObtainNetworkKeyEnabled) { @@ -1135,13 +1135,16 @@ void Dataset::OutputSecurityPolicy(const otSecurityPolicy &aSecurityPolicy) OutputFormat("R"); } - OutputNewLine(); + OutputLine(" %u", aSecurityPolicy.mVersionThresholdForRouting); } otError Dataset::ParseSecurityPolicy(otSecurityPolicy &aSecurityPolicy, Arg *&aArgs) { + static constexpr uint8_t kMaxVersionThreshold = 7; + otError error; otSecurityPolicy policy; + uint8_t versionThreshold; memset(&policy, 0, sizeof(policy)); @@ -1192,6 +1195,11 @@ otError Dataset::ParseSecurityPolicy(otSecurityPolicy &aSecurityPolicy, Arg *&aA } aArgs++; + VerifyOrExit(!aArgs->IsEmpty()); + + SuccessOrExit(error = aArgs->ParseAsUint8(versionThreshold)); + VerifyOrExit(versionThreshold <= kMaxVersionThreshold, error = OT_ERROR_INVALID_ARGS); + policy.mVersionThresholdForRouting = versionThreshold; exit: if (error == OT_ERROR_NONE) diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index 6ceed76b6..8ce0a35d4 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -1236,6 +1236,13 @@ void Mle::HandleNotifierEvents(Events aEvents) } } +#if OPENTHREAD_FTD + if (aEvents.Contains(kEventSecurityPolicyChanged)) + { + Get().HandleSecurityPolicyChanged(); + } +#endif + exit: return; } diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp index 3e63ebe1b..cd2c3129e 100644 --- a/src/core/thread/mle_router.cpp +++ b/src/core/thread/mle_router.cpp @@ -188,6 +188,30 @@ exit: return error; } +void MleRouter::HandleSecurityPolicyChanged(void) +{ + // If we are currently router or leader and no longer eligible to + // be a router (due to security policy change), we start jitter + // timeout to downgrade. + + VerifyOrExit(IsRouterOrLeader() && !IsRouterEligible()); + + // `mRouterSelectionJitterTimeout` is non-zero if we are already + // waiting to downgrade. + + VerifyOrExit(mRouterSelectionJitterTimeout == 0); + + mRouterSelectionJitterTimeout = 1 + Random::NonCrypto::GetUint8InRange(0, mRouterSelectionJitter); + + if (IsLeader()) + { + mRouterSelectionJitterTimeout += kLeaderDowngradeExtraDelay; + } + +exit: + return; +} + #if (OPENTHREAD_CONFIG_THREAD_VERSION >= OT_THREAD_VERSION_1_3_1) void MleRouter::SetDeviceProperties(const DeviceProperties &aDeviceProperties) { @@ -1592,9 +1616,15 @@ void MleRouter::HandleTimeTick(void) Attach(kDowngradeToReed); } - break; + OT_FALL_THROUGH; case kRoleLeader: + if (routerStateUpdate && !IsRouterEligible()) + { + LogInfo("No longer router eligible"); + IgnoreError(BecomeDetached()); + } + break; case kRoleDisabled: diff --git a/src/core/thread/mle_router.hpp b/src/core/thread/mle_router.hpp index 00cbade6a..e7caafe2d 100644 --- a/src/core/thread/mle_router.hpp +++ b/src/core/thread/mle_router.hpp @@ -566,6 +566,7 @@ private: static constexpr uint16_t kDiscoveryMaxJitter = 250; // Max jitter delay Discovery Responses (in msec). static constexpr uint16_t kChallengeTimeout = 2; // Challenge timeout (in sec). static constexpr uint16_t kUnsolicitedDataResponseJitter = 500; // Max delay for unsol Data Response (in msec). + static constexpr uint8_t kLeaderDowngradeExtraDelay = 10; // Extra delay to downgrade leader (in sec). static constexpr uint8_t kDefaultLeaderWeight = 64; // Threshold to accept a router upgrade request with reason @@ -585,6 +586,7 @@ private: void HandleDetachStart(void); void HandleChildStart(AttachMode aMode); + void HandleSecurityPolicyChanged(void); void HandleLinkRequest(RxInfo &aRxInfo); void HandleLinkAccept(RxInfo &aRxInfo); Error HandleLinkAccept(RxInfo &aRxInfo, bool aRequest); diff --git a/tests/scripts/thread-cert/Cert_9_2_05_ActiveDataset.py b/tests/scripts/thread-cert/Cert_9_2_05_ActiveDataset.py index 3c54cf04d..f495e8203 100755 --- a/tests/scripts/thread-cert/Cert_9_2_05_ActiveDataset.py +++ b/tests/scripts/thread-cert/Cert_9_2_05_ActiveDataset.py @@ -90,7 +90,7 @@ class Cert_9_2_05_ActiveDataset(thread_cert.TestCase): # Step 2: new, valid Timestamp TLV # all valid Active Operational Dataset parameters, # with new values in the TLVs that don’t affect connectivity - # binary = new pskc and security policy [3600, 0b11100000] + # binary = new pskc and security policy [3600, 0b11101111] self.nodes[ROUTER].send_mgmt_active_set( active_timestamp=100, channel_mask=0x3fff800, @@ -100,14 +100,14 @@ class Cert_9_2_05_ActiveDataset(thread_cert.TestCase): network_key='00112233445566778899aabbccddeeff', panid=0xface, channel=11, - binary='0410d2aa9cd8dff7919122d77d37ec3c1b5f0c030e10e0', + binary='0410d2aa9cd8dff7919122d77d37ec3c1b5f0c030e10ef', ) self.simulator.go(5) # Step 7: old, invalid Active Timestamp TLV # all valid Active Operational Dataset parameters, with # new values in the TLVs that don’t affect connectivity - # binary = new pskc and security policy [3600, 0b11110000] + # binary = new pskc and security policy [3600, 0b11111111] self.nodes[ROUTER].send_mgmt_active_set( active_timestamp=100, channel_mask=0x1fff800, @@ -117,14 +117,14 @@ class Cert_9_2_05_ActiveDataset(thread_cert.TestCase): network_key='00112233445566778899aabbccddeeff', panid=0xface, channel=11, - binary='041017d672be32b0c24a2f8385f2fbaf1d970c030e10f0', + binary='041017d672be32b0c24a2f8385f2fbaf1d970c030e10ff', ) self.simulator.go(5) # Step 9: new, valid Active Timestamp TLV # all of valid Commissioner Dataset parameters plus one bogus TLV, and # new values in the TLVs that don’t affect connectivity - # binary = new pskc and security policy [3600, 0b11111000] and BogusTLV=0x400 + # binary = new pskc and security policy [3600, 0b11111111] and BogusTLV=0x400 self.nodes[ROUTER].send_mgmt_active_set( active_timestamp=101, channel_mask=0xfff800, @@ -134,7 +134,7 @@ class Cert_9_2_05_ActiveDataset(thread_cert.TestCase): network_key='00112233445566778899aabbccddeeff', panid=0xface, channel=11, - binary='041008f4e9531e8efa8e852d5f4fb951b13e0c030e10f88202aa55', + binary='041008f4e9531e8efa8e852d5f4fb951b13e0c030e10ff8202aa55', ) self.simulator.go(5) diff --git a/tests/scripts/thread-cert/node.py b/tests/scripts/thread-cert/node.py index 4124cb10c..a5e78e372 100755 --- a/tests/scripts/thread-cert/node.py +++ b/tests/scripts/thread-cert/node.py @@ -2611,11 +2611,14 @@ class NodeImpl: self.send_command(cmd, go=False) self._expect_done() - if security_policy and len(security_policy) == 2: - cmd = 'dataset securitypolicy %s %s' % ( - str(security_policy[0]), - security_policy[1], - ) + if security_policy is not None: + if len(security_policy) >= 2: + cmd = 'dataset securitypolicy %s %s' % ( + str(security_policy[0]), + security_policy[1], + ) + if len(security_policy) >= 3: + cmd += ' %s' % (str(security_policy[2])) self.send_command(cmd, go=False) self._expect_done() @@ -2723,8 +2726,9 @@ class NodeImpl: cmd += 'networkname %s ' % self._escape_escapable(network_name) if security_policy is not None: - rotation, flags = security_policy - cmd += 'securitypolicy %d %s ' % (rotation, flags) + cmd += 'securitypolicy %d %s ' % (security_policy[0], security_policy[1]) + if (len(security_policy) >= 3): + cmd += '%d ' % (security_policy[2]) if binary is not None: cmd += '-x %s ' % binary diff --git a/tests/scripts/thread-cert/test_router_downgrade_on_sec_policy_change.py b/tests/scripts/thread-cert/test_router_downgrade_on_sec_policy_change.py new file mode 100755 index 000000000..5241ad17d --- /dev/null +++ b/tests/scripts/thread-cert/test_router_downgrade_on_sec_policy_change.py @@ -0,0 +1,149 @@ +#!/usr/bin/env python3 +# +# Copyright (c) 2023, The OpenThread Authors. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# 3. Neither the name of the copyright holder nor the +# names of its contributors may be used to endorse or promote products +# derived from this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE +# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR +# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF +# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN +# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) +# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE +# POSSIBILITY OF SUCH DAMAGE. +# + +import ipaddress +import unittest + +import command +import config +import thread_cert + +# Test description: +# This test verifies leader and router downgrade delay on security policy version threshold change. +# +# Topology: +# +# LEADER +# | +# | +# ROUTER +# + +LEADER = 1 +ROUTER = 2 + + +class DowngradeOnSecPolicyChange(thread_cert.TestCase): + USE_MESSAGE_FACTORY = False + SUPPORT_NCP = False + + TOPOLOGY = { + LEADER: { + 'name': 'LEADER', + 'mode': 'rdn', + }, + ROUTER: { + 'name': 'ROUTER', + 'mode': 'rdn', + }, + } + + def test(self): + leader = self.nodes[LEADER] + router = self.nodes[ROUTER] + + # Start the leader & router devices. + + leader.start() + self.simulator.go(config.LEADER_STARTUP_DELAY) + self.assertEqual(leader.get_state(), 'leader') + + router.start() + self.simulator.go(config.ROUTER_STARTUP_DELAY) + self.assertEqual(router.get_state(), 'router') + + self.assertTrue(leader.get_router_eligible()) + self.assertTrue(router.get_router_eligible()) + + # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + # Change security policy, disable `R` bit and set version threshold to max value (7). + + leader.send_mgmt_active_set( + active_timestamp=30, + security_policy=[3600, 'onc', 7], + ) + + self.assertEqual(leader.get_state(), 'leader') + self.assertEqual(router.get_state(), 'router') + + # Leader should take at least 10 seconds before downgrading. + + self.simulator.go(8) + self.assertEqual(leader.get_state(), 'leader') + self.assertFalse(leader.get_router_eligible()) + self.assertFalse(router.get_router_eligible()) + + # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + # Change back security policy. This should cancel the ongoing downgrade delay. + + leader.send_mgmt_active_set( + active_timestamp=60, + security_policy=[3600, 'ornc', 0], + ) + + self.simulator.go(1) + self.assertEqual(leader.get_state(), 'leader') + self.assertTrue(leader.get_router_eligible()) + + # Make sure both device stay as leader and router. + + self.simulator.go(600) + self.assertEqual(leader.get_state(), 'leader') + self.assertEqual(router.get_state(), 'router') + self.assertTrue(leader.get_router_eligible()) + self.assertTrue(router.get_router_eligible()) + + # - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + # Change security policy again, disable `R` bit and set version threshold to max value (7). + + leader.send_mgmt_active_set( + active_timestamp=90, + security_policy=[3600, 'onc', 7], + ) + + self.assertEqual(leader.get_state(), 'leader') + + # Leader should take at least 10 seconds before downgrading. + + self.simulator.go(8) + self.assertEqual(leader.get_state(), 'leader') + self.assertFalse(leader.get_router_eligible()) + + # Make sure both leader and router are downgraded and are now `detached`. + + self.simulator.go(80) + self.assertEqual(leader.get_state(), 'detached') + self.assertEqual(router.get_state(), 'detached') + + self.assertFalse(leader.get_router_eligible()) + self.assertFalse(router.get_router_eligible()) + + +if __name__ == '__main__': + unittest.main() diff --git a/tools/otci/otci/otci.py b/tools/otci/otci/otci.py index d9d5c1f25..7df5930fd 100644 --- a/tools/otci/otci/otci.py +++ b/tools/otci/otci/otci.py @@ -1738,7 +1738,7 @@ class OTCI(object): # Network Name: OpenThread-7caa # PAN ID: 0x7caa # PSKc: 167d89fd169e439ca0b8266de248090f - # Security Policy: 0, onrc + # Security Policy: 672 onrc 0 dataset = {} @@ -1765,7 +1765,7 @@ class OTCI(object): elif key == 'PSKc': dataset['pskc'] = val elif key == 'Security Policy': - rotation_time, flags = val.split(', ') if ', ' in val else val.split(' ') + rotation_time, flags, version_threshold = val.split(' ') rotation_time = int(rotation_time) dataset['security_policy'] = SecurityPolicy(rotation_time, flags) else: