diff --git a/src/core/common/settings.cpp b/src/core/common/settings.cpp index e072037e1..5fae02f86 100644 --- a/src/core/common/settings.cpp +++ b/src/core/common/settings.cpp @@ -226,7 +226,7 @@ void Settings::DeleteOperationalDataset(MeshCoP::Dataset::Type aType) } #if OPENTHREAD_CONFIG_BLE_TCAT_ENABLE -void Settings::SaveTcatCommissionerCertificate(uint8_t *aCert, uint16_t aCertLen) +void Settings::SaveTcatCommissionerCertificate(const uint8_t *aCert, uint16_t aCertLen) { Error error = Get().Set(kKeyTcatCommrCert, aCert, aCertLen); diff --git a/src/core/common/settings.hpp b/src/core/common/settings.hpp index 7ce35cb17..3089a73ee 100644 --- a/src/core/common/settings.hpp +++ b/src/core/common/settings.hpp @@ -770,7 +770,7 @@ public: * @param[in] aCert The DER-encoded X509 end-entity certificate to store. * @param[in] aCertLen Certificate length. */ - void SaveTcatCommissionerCertificate(uint8_t *aCert, uint16_t aCertLen); + void SaveTcatCommissionerCertificate(const uint8_t *aCert, uint16_t aCertLen); /** * Reads the Tcat Commissioner certificate. diff --git a/src/core/meshcop/tcat_agent.cpp b/src/core/meshcop/tcat_agent.cpp index f24d98cae..0773fcb2a 100644 --- a/src/core/meshcop/tcat_agent.cpp +++ b/src/core/meshcop/tcat_agent.cpp @@ -82,7 +82,7 @@ void TcatAgent::ClearCommissionerState(void) mInstallCodeVerified = false; mCanOverwriteDataset = false; mApplicationResponsePending = false; - mHasWrittenActiveDataset = false; + mIsSourceOfDatasetChange = false; } Error TcatAgent::Start(AppDataReceiveCallback aAppDataReceiveCallback, JoinCallback aJoinHandler, void *aContext) @@ -602,7 +602,7 @@ Error TcatAgent::HandleSetActiveOperationalDataset(const Message &aIncomingMessa // Flag lets HandleNotifierEvents() know that the Agent is the source of the change. In theory, this could be // coalesced with another module's dataset write at exactly the same time, but this is in practice impossible // to exploit as an attack vector by the TCAT Commissioner. - mHasWrittenActiveDataset = true; + mIsSourceOfDatasetChange = true; exit: return error; @@ -697,6 +697,15 @@ Error TcatAgent::HandleDecommission(void) VerifyOrExit(IsCommandClassAuthorized(kDecommissioning), error = kErrorRejected); SuccessOrExit(error = Get().GetPeerCertificateDer(buf, &bufLen, bufLen)); + Decommission(buf, static_cast(bufLen)); + +exit: + mJoinCallback.InvokeIfSet(&GetInstance(), /* aIsJoin */ false, error); + return error; +} + +void TcatAgent::Decommission(const uint8_t *aCommissionerCert, uint16_t aCertLength) +{ Get().Stop(); if (!mVendorInfo->mDoNotActivateAfterLeaving) @@ -708,7 +717,7 @@ Error TcatAgent::HandleDecommission(void) Get().Clear(); IgnoreReturnValue(Get().ErasePersistentInfo()); - Get().SaveTcatCommissionerCertificate(buf, static_cast(bufLen)); + Get().SaveTcatCommissionerCertificate(aCommissionerCert, aCertLength); #if !OPENTHREAD_CONFIG_PLATFORM_KEY_REFERENCES_ENABLE { @@ -718,11 +727,8 @@ Error TcatAgent::HandleDecommission(void) } #endif - mJoinCallback.InvokeIfSet(&GetInstance(), /* aIsJoin */ false, error); - mCanOverwriteDataset = true; // enable repeated commissioning/decommissioning cycles in a session - -exit: - return error; + mCanOverwriteDataset = true; // enable repeated commissioning/decommissioning cycles in a session + mIsSourceOfDatasetChange = true; // record that we made the dataset change (causing callback event later) } Error TcatAgent::HandlePing(const Message &aIncomingMessage, @@ -1119,11 +1125,11 @@ void TcatAgent::HandleNotifierEvents(Events aEvents) // Change of network key or ExtPanId by another process: it wrote a dataset for *another* Thread Network. // This event revokes the Commissioner's existing authorization (if any) to rewrite datasets. - if (!mHasWrittenActiveDataset && aEvents.ContainsAny(kEventNetworkKeyChanged | kEventThreadExtPanIdChanged)) + if (!mIsSourceOfDatasetChange && aEvents.ContainsAny(kEventNetworkKeyChanged | kEventThreadExtPanIdChanged)) { mCanOverwriteDataset = false; } - mHasWrittenActiveDataset = false; + mIsSourceOfDatasetChange = false; if (aEvents.Contains(kEventPskcChanged)) { diff --git a/src/core/meshcop/tcat_agent.hpp b/src/core/meshcop/tcat_agent.hpp index d0ba29db6..4f68b56ce 100644 --- a/src/core/meshcop/tcat_agent.hpp +++ b/src/core/meshcop/tcat_agent.hpp @@ -459,6 +459,7 @@ private: const OffsetRange &aOffsetRange, bool &aResponse); Error HandleDecommission(void); + void Decommission(const uint8_t *aCommissionerCert, uint16_t aCertLength); Error HandlePing(const Message &aIncomingMessage, Message &aOutgoingMessage, const OffsetRange &aOffsetRange, @@ -525,7 +526,7 @@ private: bool mInstallCodeVerified : 1; bool mCanOverwriteDataset : 1; bool mApplicationResponsePending : 1; - bool mHasWrittenActiveDataset : 1; + bool mIsSourceOfDatasetChange : 1; using ExpireTimer = TimerMilliIn; ExpireTimer mActiveOrStandbyTimer; uint32_t mTcatActiveDurationMs; diff --git a/tests/unit/test_tcat.cpp b/tests/unit/test_tcat.cpp index c93f735b9..bd9b9990d 100644 --- a/tests/unit/test_tcat.cpp +++ b/tests/unit/test_tcat.cpp @@ -606,7 +606,7 @@ private: { aInstance->Get().Disable(); aInstance->Get().SaveLocal(aDataset); - aInstance->Get().mHasWrittenActiveDataset = true; + aInstance->Get().mIsSourceOfDatasetChange = true; otTaskletsProcess(aInstance); } @@ -626,6 +626,22 @@ private: otTaskletsProcess(aInstance); } + // Mock operation: TCAT Commissioner sends the Decommission command. `HandleDecommission()` itself can't be + // called here because it reads the peer certificate from a real (mbedtls) TLS session, which these unit + // tests don't set up. So its authorization check is mimicked and the rest of it - `Decommission()` - is + // invoked with a stand-in commissioner certificate. + static void MockDecommission(Instance *aInstance) + { + static uint8_t sCommissionerCert[] = {0x30, 0x82, 0x01, 0x00}; + + TcatAgent &agent = aInstance->Get(); + + VerifyOrQuit(agent.IsCommandClassAuthorized(TcatAgent::kDecommissioning)); + agent.Decommission(sCommissionerCert, sizeof(sCommissionerCert)); + agent.mJoinCallback.InvokeIfSet(aInstance, /* aIsJoin */ false, kErrorNone); + otTaskletsProcess(aInstance); + } + // Mock operation: the device attaches to a Thread network. Implemented by directly becoming Leader, // which is synchronous (no need to drive the attach state machine) and signals `kEventThreadRoleChanged` // just like a real attach. Requires a complete Active Dataset to be present. @@ -1250,6 +1266,59 @@ public: testFreeInstance(instance); } + // Verifies that a Decommission command restores the Commissioner's authorization to (over)write the Active + // Dataset within the same TCAT session, so that commissioning/decommissioning cycles can be repeated. + static void TestTcatDecommissionRestoresDatasetWrite(void) + { + Instance *instance = TestInitInstanceTcat(); + TcatAgent *agent = &instance->Get(); + TcatJoinCounters counters = {}; + + // A device that was commissioned by some other entity (not this TCAT Agent) before the session started. + MockActiveDatasetChanged(instance, sFullDataset); + VerifyOrQuit(instance->Get().IsCommissioned()); + + // The Commissioner connects: it is not authorized to overwrite the existing Active Dataset, but it is + // authorized to decommission the device. + MockCommissionerConnected(agent, sCommAuth, sDeviceAuth, /* aIsCommissionedAtStart */ true); + agent->mJoinCallback.Set(HandleTcatJoin, &counters); + VerifyOrQuit(CommandClassesAuthorized(agent, kClassGeneral | kClassCommissioning | kClassExtraction | + kClassDecommissioning | kClassApplication)); + VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sFullDataset)); + VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sPartialDataset)); + + for (int i = 0; i < 3; i++) + { + // Decommission erases the Active Dataset and reports a successful 'leave'. + MockDecommission(instance); + VerifyOrQuit(!instance->Get().IsCommissioned()); + VerifyOrQuit(instance->Get().IsDisabled()); + VerifyOrQuit(counters.mLeaveCount == static_cast(i + 1) && counters.mJoinCount == 0 && + counters.mLastError == kErrorNone); + + // Regression test: decommissioning clears the Network Key, which signals a Notifier event that would + // otherwise be mistaken for an external dataset change and immediately revoke the authorization again. + VerifyOrQuit(agent->mCanOverwriteDataset, "Decommission must restore dataset-write authorization"); + VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sFullDataset)); + VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sPartialDataset)); + + // The Commissioner recommissions the device in the same session, and may still overwrite afterwards. + MockWriteActiveDataset(instance, sFullDataset); + VerifyOrQuit(instance->Get().IsCommissioned()); + VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sFullDataset)); + VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sPartialDataset)); + } + + // A dataset change by another module still revokes the authorization after a Decommission. + MockDecommission(instance); + VerifyOrQuit(agent->mCanOverwriteDataset); + MockActiveDatasetChanged(instance, sFullDataset); + VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sFullDataset)); + VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sPartialDataset)); + + testFreeInstance(instance); + } + // Verifies the OpenThread Notifier assumption that TcatAgent::HandleNotifierEvents() relies on: // multiple state changes that occur back-to-back are coalesced into a single notification carrying all of // the corresponding event flags. @@ -1279,7 +1348,7 @@ public: changed.mNetworkKey.m8[0] ^= 0xff; changed.mExtendedPanId.m8[0] ^= 0xff; instance->Get().SaveLocal(changed); - agent->mHasWrittenActiveDataset = true; // model that this Agent is the source of the change + agent->mIsSourceOfDatasetChange = true; // model that this Agent is the source of the change } // repeat the tasklets processing to ensure subsequent notifer calls are not made. @@ -1293,7 +1362,7 @@ public: VerifyOrQuit(observer.mExtPanIdCount == 1, "Extended PAN ID change must be reported exactly once"); VerifyOrQuit(observer.mBothInOneCount == 1, "both changes must be coalesced into the same notification"); - // Because the agent saw both events coalesced while mHasWrittenActiveDataset was set, it recognizes the + // Because the agent saw both events coalesced while mIsSourceOfDatasetChange was set, it recognizes the // change as its own and retains the Commissioner's authorization to overwrite the dataset. VerifyOrQuit(agent->mCanOverwriteDataset, "a self-made dataset change must not revoke authorization"); } @@ -1324,6 +1393,7 @@ int main(void) ot::MeshCoP::UnitTester::TestTcatDatasetOverwrite(); ot::MeshCoP::UnitTester::TestTcatDatasetOverwriteAfterAttach(); ot::MeshCoP::UnitTester::TestTcatRepeatedCommandActivation(); + ot::MeshCoP::UnitTester::TestTcatDecommissionRestoresDatasetWrite(); ot::MeshCoP::UnitTester::TestTcatNotifierCoalescesEvents(); printf("All tests passed\n"); #else