From c66d91bdd7268e81402c1064d0e0a87002c7d4f6 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Thu, 7 Mar 2024 21:39:52 -0800 Subject: [PATCH] [key-manager] update how key guard time is determined and applied (#9871) This commit makes changes/fixes to `KeyManager` regarding key switch guard time. Key Rotation Time updates: - When the Key Rotation Time changes (due to security policy updates), the key switch guard time (`mKeySwitchGuardTime`) is also adjusted. It's set to 93% of the Rotation Time (rounded down). - Immediately checks if the new rotation time indicates a rotation is due and keys are rotated. New variable `mKeySwitchGuardTimer`: - This is reset to the current guard time whenever the key sequence is updated. - It decrements hourly until reaching zero. - Key switch guard comparison is made with this value, aligning the implementation with the Thread specification. `SetCurrentKeySequence()` modification: - Now accepts a new input parameter that determines whether to apply or ignore the key switch guard when updating the key sequence. - During a key rotation check (when the rotation time has passed), the key switch guard is ignored and we always move to the next key sequence number. Other changes: - Variables handling guard and rotation time now use `uint16_t` instead of `uint32_t` to align with security policy definitions. - API and CLI command documentation for setting the "key switch guard time" emphasize that they are intended for testing purposes. --- include/openthread/instance.h | 2 +- include/openthread/thread.h | 4 +- src/cli/README.md | 6 +- src/core/api/thread_api.cpp | 6 +- src/core/mac/mac.cpp | 2 +- src/core/thread/key_manager.cpp | 77 ++++++++++++------- src/core/thread/key_manager.hpp | 60 +++++++++++---- src/core/thread/mle.cpp | 6 +- src/ncp/ncp_base_mtd.cpp | 2 +- .../thread-cert/test_mle_msg_key_seq_jump.py | 12 +-- 10 files changed, 116 insertions(+), 61 deletions(-) diff --git a/include/openthread/instance.h b/include/openthread/instance.h index a78459a43..6371d21f5 100644 --- a/include/openthread/instance.h +++ b/include/openthread/instance.h @@ -53,7 +53,7 @@ extern "C" { * @note This number versions both OpenThread platform and user APIs. * */ -#define OPENTHREAD_API_VERSION (399) +#define OPENTHREAD_API_VERSION (400) /** * @addtogroup api-instance diff --git a/include/openthread/thread.h b/include/openthread/thread.h index b077001e4..4d814c119 100644 --- a/include/openthread/thread.h +++ b/include/openthread/thread.h @@ -709,7 +709,7 @@ void otThreadSetKeySequenceCounter(otInstance *aInstance, uint32_t aKeySequenceC * @sa otThreadSetKeySwitchGuardTime * */ -uint32_t otThreadGetKeySwitchGuardTime(otInstance *aInstance); +uint16_t otThreadGetKeySwitchGuardTime(otInstance *aInstance); /** * Sets the thrKeySwitchGuardTime (in hours). @@ -723,7 +723,7 @@ uint32_t otThreadGetKeySwitchGuardTime(otInstance *aInstance); * @sa otThreadGetKeySwitchGuardTime * */ -void otThreadSetKeySwitchGuardTime(otInstance *aInstance, uint32_t aKeySwitchGuardTime); +void otThreadSetKeySwitchGuardTime(otInstance *aInstance, uint16_t aKeySwitchGuardTime); /** * Detach from the Thread network. diff --git a/src/cli/README.md b/src/cli/README.md index e2b43c8cc..c8dfe2744 100644 --- a/src/cli/README.md +++ b/src/cli/README.md @@ -1826,6 +1826,8 @@ Done Set the Thread Key Sequence Counter. +This command is reserved for testing and demo purposes only. Changing Key Sequence Counter will render a production application non-compliant with the Thread Specification. + ```bash > keysequence counter 10 Done @@ -1843,7 +1845,9 @@ Done ### keysequence guardtime \ -Set Thread Key Switch Guard Time (in hours) 0 means Thread Key Switch immediately if key index match +Set Thread Key Switch Guard Time (in hours). + +This command is reserved for testing and demo purposes only. Changing Key Switch Guard Time will render a production application non-compliant with the Thread Specification. ```bash > keysequence guardtime 0 diff --git a/src/core/api/thread_api.cpp b/src/core/api/thread_api.cpp index 6bf2ca39d..97e5508f0 100644 --- a/src/core/api/thread_api.cpp +++ b/src/core/api/thread_api.cpp @@ -275,15 +275,15 @@ uint32_t otThreadGetKeySequenceCounter(otInstance *aInstance) void otThreadSetKeySequenceCounter(otInstance *aInstance, uint32_t aKeySequenceCounter) { - AsCoreType(aInstance).Get().SetCurrentKeySequence(aKeySequenceCounter); + AsCoreType(aInstance).Get().SetCurrentKeySequence(aKeySequenceCounter, KeyManager::kForceUpdate); } -uint32_t otThreadGetKeySwitchGuardTime(otInstance *aInstance) +uint16_t otThreadGetKeySwitchGuardTime(otInstance *aInstance) { return AsCoreType(aInstance).Get().GetKeySwitchGuardTime(); } -void otThreadSetKeySwitchGuardTime(otInstance *aInstance, uint32_t aKeySwitchGuardTime) +void otThreadSetKeySwitchGuardTime(otInstance *aInstance, uint16_t aKeySwitchGuardTime) { AsCoreType(aInstance).Get().SetKeySwitchGuardTime(aKeySwitchGuardTime); } diff --git a/src/core/mac/mac.cpp b/src/core/mac/mac.cpp index 243db1d8a..4fe93fe4e 100644 --- a/src/core/mac/mac.cpp +++ b/src/core/mac/mac.cpp @@ -1637,7 +1637,7 @@ Error Mac::ProcessReceiveSecurity(RxFrame &aFrame, const Address &aSrcAddr, Neig if (keySequence > keyManager.GetCurrentKeySequence()) { - keyManager.SetCurrentKeySequence(keySequence); + keyManager.SetCurrentKeySequence(keySequence, KeyManager::kApplyKeySwitchGuard); } } diff --git a/src/core/thread/key_manager.cpp b/src/core/thread/key_manager.cpp index 3d9337d93..d38fdb8b5 100644 --- a/src/core/thread/key_manager.cpp +++ b/src/core/thread/key_manager.cpp @@ -60,6 +60,9 @@ const uint8_t KeyManager::kTrelInfoString[] = {'T', 'h', 'r', 'e', 'a', 'd', 'O' 'r', 'I', 'n', 'f', 'r', 'a', 'K', 'e', 'y'}; #endif +//--------------------------------------------------------------------------------------------------------------------- +// SecurityPolicy + void SecurityPolicy::SetToDefault(void) { mRotationTime = kDefaultKeyRotationTime; @@ -163,6 +166,9 @@ exit: return; } +//--------------------------------------------------------------------------------------------------------------------- +// KeyManager + KeyManager::KeyManager(Instance &aInstance) : InstanceLocator(aInstance) , mKeySequence(0) @@ -171,7 +177,7 @@ KeyManager::KeyManager(Instance &aInstance) , mStoredMleFrameCounter(0) , mHoursSinceKeyRotation(0) , mKeySwitchGuardTime(kDefaultKeySwitchGuardTime) - , mKeySwitchGuardEnabled(false) + , mKeySwitchGuardTimer(0) , mKeyRotationTimer(aInstance) , mKekFrameCounter(0) , mIsPskcSet(false) @@ -198,8 +204,8 @@ KeyManager::KeyManager(Instance &aInstance) void KeyManager::Start(void) { - mKeySwitchGuardEnabled = false; - StartKeyRotationTimer(); + mKeySwitchGuardTimer = 0; + ResetKeyRotationTimer(); } void KeyManager::Stop(void) { mKeyRotationTimer.Stop(); } @@ -362,20 +368,13 @@ void KeyManager::UpdateKeyMaterial(void) #endif } -void KeyManager::SetCurrentKeySequence(uint32_t aKeySequence) +void KeyManager::SetCurrentKeySequence(uint32_t aKeySequence, KeySequenceUpdateMode aUpdateMode) { VerifyOrExit(aKeySequence != mKeySequence, Get().SignalIfFirst(kEventThreadKeySeqCounterChanged)); - if ((aKeySequence == (mKeySequence + 1)) && mKeyRotationTimer.IsRunning()) + if (aUpdateMode == kApplyKeySwitchGuard) { - if (mKeySwitchGuardEnabled) - { - // Check if the guard timer has expired if key rotation is requested. - VerifyOrExit(mHoursSinceKeyRotation >= mKeySwitchGuardTime); - StartKeyRotationTimer(); - } - - mKeySwitchGuardEnabled = true; + VerifyOrExit(mKeySwitchGuardTimer == 0); } mKeySequence = aKeySequence; @@ -384,6 +383,9 @@ void KeyManager::SetCurrentKeySequence(uint32_t aKeySequence) SetAllMacFrameCounters(0, /* aSetIfLarger */ false); mMleFrameCounter = 0; + ResetKeyRotationTimer(); + mKeySwitchGuardTimer = mKeySwitchGuardTime; + Get().Signal(kEventThreadKeySeqCounterChanged); exit: @@ -476,40 +478,57 @@ void KeyManager::SetKek(const Kek &aKek) void KeyManager::SetSecurityPolicy(const SecurityPolicy &aSecurityPolicy) { - if (aSecurityPolicy.mRotationTime < SecurityPolicy::kMinKeyRotationTime) + SecurityPolicy newPolicy = aSecurityPolicy; + + if (newPolicy.mRotationTime < SecurityPolicy::kMinKeyRotationTime) { - LogNote("Key Rotation Time too small: %d", aSecurityPolicy.mRotationTime); - ExitNow(); + newPolicy.mRotationTime = SecurityPolicy::kMinKeyRotationTime; + LogNote("Key Rotation Time in SecurityPolicy is set to min allowed value of %u", newPolicy.mRotationTime); } - IgnoreError(Get().Update(mSecurityPolicy, aSecurityPolicy, kEventSecurityPolicyChanged)); + if (newPolicy.mRotationTime != mSecurityPolicy.mRotationTime) + { + uint32_t newGuardTime = newPolicy.mRotationTime; -exit: - return; + // Calculations are done using a `uint32_t` variable to prevent + // potential overflow. + + newGuardTime *= kKeySwitchGuardTimePercentage; + newGuardTime /= 100; + + mKeySwitchGuardTime = static_cast(newGuardTime); + } + + IgnoreError(Get().Update(mSecurityPolicy, newPolicy, kEventSecurityPolicyChanged)); + + CheckForKeyRotation(); } -void KeyManager::StartKeyRotationTimer(void) +void KeyManager::ResetKeyRotationTimer(void) { mHoursSinceKeyRotation = 0; - mKeyRotationTimer.Start(kOneHourIntervalInMsec); + mKeyRotationTimer.Start(Time::kOneHourInMsec); } void KeyManager::HandleKeyRotationTimer(void) { + mKeyRotationTimer.Start(Time::kOneHourInMsec); + mHoursSinceKeyRotation++; - // Order of operations below is important. We should restart the timer (from - // last fire time for one hour interval) before potentially calling - // `SetCurrentKeySequence()`. `SetCurrentKeySequence()` uses the fact that - // timer is running to decide to check for the guard time and to reset the - // rotation timer (and the `mHoursSinceKeyRotation`) if it updates the key - // sequence. + if (mKeySwitchGuardTimer > 0) + { + mKeySwitchGuardTimer--; + } - mKeyRotationTimer.StartAt(mKeyRotationTimer.GetFireTime(), kOneHourIntervalInMsec); + CheckForKeyRotation(); +} +void KeyManager::CheckForKeyRotation(void) +{ if (mHoursSinceKeyRotation >= mSecurityPolicy.mRotationTime) { - SetCurrentKeySequence(mKeySequence + 1); + SetCurrentKeySequence(mKeySequence + 1, kForceUpdate); } } diff --git a/src/core/thread/key_manager.hpp b/src/core/thread/key_manager.hpp index 18f11f20b..099854c45 100644 --- a/src/core/thread/key_manager.hpp +++ b/src/core/thread/key_manager.hpp @@ -77,8 +77,17 @@ public: */ static constexpr uint8_t kVersionThresholdOffsetVersion = 3; - static constexpr uint16_t kMinKeyRotationTime = 1; ///< The minimum Key Rotation Time in hours. - static constexpr uint16_t kDefaultKeyRotationTime = 672; ///< Default Key Rotation Time (in unit of hours). + /** + * Default Key Rotation Time (in unit of hours). + * + */ + static constexpr uint16_t kDefaultKeyRotationTime = 672; + + /** + * Minimum Key Rotation Time (in unit of hours). + * + */ + static constexpr uint16_t kMinKeyRotationTime = 2; /** * Initializes the object with default Key Rotation Time @@ -211,6 +220,18 @@ typedef Mac::KeyMaterial KekKeyMaterial; class KeyManager : public InstanceLocator, private NonCopyable { public: + /** + * Determines whether to apply or ignore key switch guard when updating the key sequence. + * + * Used as input by `SetCurrentKeySequence()`. + * + */ + enum KeySequenceUpdateMode : uint8_t + { + kApplyKeySwitchGuard, ///< Apply key switch guard check before setting the new key sequence. + kForceUpdate, ///< Ignore key switch guard check and forcibly update the key sequence to new value. + }; + /** * Initializes the object. * @@ -321,10 +342,14 @@ public: /** * Sets the current key sequence value. * - * @param[in] aKeySequence The key sequence value. + * If @p aMode is `kApplyKeySwitchGuard`, the current key switch guard timer is checked and only if it is zero, key + * sequence will be updated. + * + * @param[in] aKeySequence The key sequence value. + * @param[in] aUpdateMode Whether or not to apply the key switch guard. * */ - void SetCurrentKeySequence(uint32_t aKeySequence); + void SetCurrentKeySequence(uint32_t aKeySequence, KeySequenceUpdateMode aUpdateMode); #if OPENTHREAD_CONFIG_RADIO_LINK_TREL_ENABLE /** @@ -500,17 +525,19 @@ public: * @returns The KeySwitchGuardTime value in hours. * */ - uint32_t GetKeySwitchGuardTime(void) const { return mKeySwitchGuardTime; } + uint16_t GetKeySwitchGuardTime(void) const { return mKeySwitchGuardTime; } /** * Sets the KeySwitchGuardTime. * * The KeySwitchGuardTime is the time interval during which key rotation procedure is prevented. * - * @param[in] aKeySwitchGuardTime The KeySwitchGuardTime value in hours. + * Intended for testing only. Changing the guard time will render device non-compliant with the Thread spec. + * + * @param[in] aGuardTime The KeySwitchGuardTime value in hours. * */ - void SetKeySwitchGuardTime(uint32_t aKeySwitchGuardTime) { mKeySwitchGuardTime = aKeySwitchGuardTime; } + void SetKeySwitchGuardTime(uint16_t aGuardTime) { mKeySwitchGuardTime = aGuardTime; } /** * Returns the Security Policy. @@ -565,9 +592,13 @@ public: #endif private: - static constexpr uint32_t kDefaultKeySwitchGuardTime = 624; - static constexpr uint32_t kOneHourIntervalInMsec = 3600u * 1000u; - static constexpr bool kExportableMacKeys = OPENTHREAD_CONFIG_PLATFORM_MAC_KEYS_EXPORTABLE_ENABLE; + static constexpr uint16_t kDefaultKeySwitchGuardTime = 624; // ~ 93% of 672 (default key rotation time) + static constexpr uint32_t kKeySwitchGuardTimePercentage = 93; // Percentage of key rotation time. + static constexpr bool kExportableMacKeys = OPENTHREAD_CONFIG_PLATFORM_MAC_KEYS_EXPORTABLE_ENABLE; + + static_assert(kDefaultKeySwitchGuardTime == + SecurityPolicy::kDefaultKeyRotationTime * kKeySwitchGuardTimePercentage / 100, + "Default key switch guard time value is not correct"); OT_TOOL_PACKED_BEGIN struct Keys @@ -591,8 +622,9 @@ private: void ComputeTrelKey(uint32_t aKeySequence, Mac::Key &aKey) const; #endif - void StartKeyRotationTimer(void); + void ResetKeyRotationTimer(void); void HandleKeyRotationTimer(void); + void CheckForKeyRotation(void); #if OPENTHREAD_CONFIG_PLATFORM_KEY_REFERENCES_ENABLE void StoreNetworkKey(const NetworkKey &aNetworkKey, bool aOverWriteExisting); @@ -630,9 +662,9 @@ private: uint32_t mStoredMacFrameCounter; uint32_t mStoredMleFrameCounter; - uint32_t mHoursSinceKeyRotation; - uint32_t mKeySwitchGuardTime; - bool mKeySwitchGuardEnabled; + uint16_t mHoursSinceKeyRotation; + uint16_t mKeySwitchGuardTime; + uint16_t mKeySwitchGuardTimer; RotationTimer mKeyRotationTimer; #if OPENTHREAD_CONFIG_PLATFORM_KEY_REFERENCES_ENABLE diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp index 786a7949a..a92c790c1 100644 --- a/src/core/thread/mle.cpp +++ b/src/core/thread/mle.cpp @@ -378,7 +378,7 @@ void Mle::Restore(void) SuccessOrExit(Get().Read(networkInfo)); - Get().SetCurrentKeySequence(networkInfo.GetKeySequence()); + Get().SetCurrentKeySequence(networkInfo.GetKeySequence(), KeyManager::kForceUpdate); Get().SetMleFrameCounter(networkInfo.GetMleFrameCounter()); Get().SetAllMacFrameCounters(networkInfo.GetMacFrameCounter(), /* aSetIfLarger */ false); @@ -2726,7 +2726,7 @@ void Mle::ProcessKeySequence(RxInfo &aRxInfo) switch (aRxInfo.mClass) { case RxInfo::kAuthoritativeMessage: - Get().SetCurrentKeySequence(aRxInfo.mKeySequence); + Get().SetCurrentKeySequence(aRxInfo.mKeySequence, KeyManager::kForceUpdate); break; case RxInfo::kPeerMessage: @@ -2734,7 +2734,7 @@ void Mle::ProcessKeySequence(RxInfo &aRxInfo) { if (aRxInfo.mKeySequence - Get().GetCurrentKeySequence() == 1) { - Get().SetCurrentKeySequence(aRxInfo.mKeySequence); + Get().SetCurrentKeySequence(aRxInfo.mKeySequence, KeyManager::kApplyKeySwitchGuard); } else { diff --git a/src/ncp/ncp_base_mtd.cpp b/src/ncp/ncp_base_mtd.cpp index badc6a5e4..aa87efa51 100644 --- a/src/ncp/ncp_base_mtd.cpp +++ b/src/ncp/ncp_base_mtd.cpp @@ -690,7 +690,7 @@ template <> otError NcpBase::HandlePropertySet(keyGuardTime)); exit: return error; 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 d40055744..c3ea2cb8b 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 @@ -221,20 +221,20 @@ class MleMsgKeySeqJump(thread_cert.TestCase): self.assertEqual(reed.get_key_sequence_counter(), 20) #------------------------------------------------------------------- - # Move forward the key seq counter by one on router. Wait for max + # Move forward the key seq counter by two on router. Wait for max # time between advertisements. Validate that leader adopts the higher # counter value. - router.set_key_sequence_counter(21) - self.assertEqual(router.get_key_sequence_counter(), 21) + router.set_key_sequence_counter(22) + self.assertEqual(router.get_key_sequence_counter(), 22) self.simulator.go(52) - self.assertEqual(leader.get_key_sequence_counter(), 21) - self.assertEqual(reed.get_key_sequence_counter(), 21) + self.assertEqual(leader.get_key_sequence_counter(), 22) + self.assertEqual(reed.get_key_sequence_counter(), 22) child.set_mode('r') self.simulator.go(2) - self.assertEqual(child.get_key_sequence_counter(), 21) + self.assertEqual(child.get_key_sequence_counter(), 22) #------------------------------------------------------------------- # Force a reattachment from the child with a higher key seq counter,