From ec3e9402e312565c80fa227d611b6d11fdc32576 Mon Sep 17 00:00:00 2001 From: Esko Dijk Date: Thu, 27 Aug 2026 17:43:55 +0200 Subject: [PATCH] [tcat] Fix dataset-write authorization/callback for Decommission command (#13182) Previously, the case of the TCAT Commissioner doing a decommission operation did not restore its authorization to perform active-dataset-writes, if that authorization was already revoked due to an 'external-triggered' active dataset change. This PR fixes it: a decommission by the TCAT Commissioner will now restore its authorization for active-dataset-writes within the same session. It renames the related flag to `mIsSourceOfDatasetChange`, making it more generic, and adds a test to check the new behavior for multiple decommission operations invoked. To be able to test this, the HandleDecommission() function needed to be split into 2 parts such that the unit test mock function can do Decommission() also. The callback to the application for HandleDecommission() is also fixed to be called in unsuccessful error cases, as required by the API. --- src/core/common/settings.cpp | 2 +- src/core/common/settings.hpp | 2 +- src/core/meshcop/tcat_agent.cpp | 26 ++++++----- src/core/meshcop/tcat_agent.hpp | 3 +- tests/unit/test_tcat.cpp | 76 +++++++++++++++++++++++++++++++-- 5 files changed, 93 insertions(+), 16 deletions(-) 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