diff --git a/src/core/net/srp_client.cpp b/src/core/net/srp_client.cpp index b9aba373b..572b2ea3b 100644 --- a/src/core/net/srp_client.cpp +++ b/src/core/net/srp_client.cpp @@ -355,8 +355,7 @@ Client::Client(Instance &aInstance) , mServiceKeyRecordEnabled(false) , mUseShortLeaseOption(false) #endif - , mNextMessageId(0) - , mResponseMessageId(0) + , mCurMessageId(0) , mAutoHostAddressCount(0) , mRetryWaitInterval(kMinRetryWaitInterval) , mTtl(0) @@ -468,7 +467,6 @@ void Client::Stop(Requester aRequester, StopMode aMode) mShouldRemoveKeyLease = false; mTxFailureRetryCount = 0; - mResponseMessageId = mNextMessageId; if (aMode == kResetRetryInterval) { @@ -1004,19 +1002,11 @@ void Client::SendUpdate(void) { LogInfo("Msg len %lu is larger than MTU, enabling single service mode", ToUlong(length)); mSingleServiceMode = true; + SelectNewMessageId(); IgnoreError(info.mMessage->SetLength(0)); SuccessOrExit(error = PrepareUpdateMessage(info)); } - SuccessOrExit(error = mSocket.SendTo(*info.mMessage, Ip6::MessageInfo())); - - // Ownership of the message is transferred to the socket upon a - // successful `SendTo()` call. - - info.mMessage.Release(); - - LogInfo("Send update, msg-id:0x%x", mNextMessageId); - // State changes: // kToAdd -> kAdding // kToRefresh -> kRefreshing @@ -1024,26 +1014,21 @@ void Client::SendUpdate(void) anyChanged = ChangeHostAndServiceStates(kNewStateOnMessageTx, kForServicesAppendedInMessage); - // `mNextMessageId` tracks the message ID used in the prepared - // update message. It is incremented after a successful - // `mSocket.SendTo()` call. If unsuccessful, the same ID can be - // reused for the next update. - // - // Acceptable response message IDs fall within the range starting - // at `mResponseMessageId ` and ending before `mNextMessageId`. - // - // `anyChanged` tracks if any host or service states have changed. - // If not, the prepared message is identical to the last one with - // the same hosts/services, allowing us to accept earlier message - // IDs. If changes occur, `mResponseMessageId ` is updated to - // ensure only responses to the latest message are accepted. - if (anyChanged) { - mResponseMessageId = mNextMessageId; + SelectNewMessageId(); } - mNextMessageId++; + SuccessOrExit(error = UpdateIdAndSignatureInUpdateMessage(info)); + + SuccessOrExit(error = mSocket.SendTo(*info.mMessage, Ip6::MessageInfo())); + + // Ownership of the message is transferred to the socket upon a + // successful `SendTo()` call. + + info.mMessage.Release(); + + LogInfo("Send update, msg-id:0x%x", mCurMessageId); // Remember the update message tx time to use later to determine the // lease renew time. @@ -1115,7 +1100,7 @@ Error Client::PrepareUpdateMessage(MsgInfo &aInfo) SuccessOrExit(error = ReadOrGenerateKey(aInfo.mKeyInfo)); - header.SetMessageId(mNextMessageId); + header.SetMessageId(mCurMessageId); // SRP Update (DNS Update) message must have exactly one record in // Zone section, no records in Prerequisite Section, can have @@ -1148,7 +1133,24 @@ Error Client::PrepareUpdateMessage(MsgInfo &aInfo) // Prepare Additional Data section SuccessOrExit(error = AppendUpdateLeaseOptRecord(aInfo)); - SuccessOrExit(error = AppendSignature(aInfo)); + SuccessOrExit(error = AppendSignature(aInfo, kAppendEmptySignature)); + +exit: + return error; +} + +Error Client::UpdateIdAndSignatureInUpdateMessage(MsgInfo &aInfo) +{ + constexpr uint16_t kHeaderOffset = 0; + + Error error; + Dns::UpdateHeader header; + + IgnoreError(aInfo.mMessage->Read(kHeaderOffset, header)); + header.SetMessageId(mCurMessageId); + aInfo.mMessage->Write(kHeaderOffset, header); + + SuccessOrExit(error = AppendSignature(aInfo, kOverwriteWithNewSignature)); header.SetAdditionalRecordCount(2); // Lease OPT and SIG RRs aInfo.mMessage->Write(kHeaderOffset, header); @@ -1674,7 +1676,7 @@ exit: return error; } -Error Client::AppendSignature(MsgInfo &aInfo) +Error Client::AppendSignature(MsgInfo &aInfo, SignatureAppendMode aMode) { Error error; Dns::SigRecord sig; @@ -1694,37 +1696,49 @@ Error Client::AppendSignature(MsgInfo &aInfo) sig.Init(Dns::ResourceRecord::kClassAny); sig.SetAlgorithm(Dns::KeyRecord::kAlgorithmEcdsaP256Sha256); - // Append the SIG RR with full uncompressed form of the host name - // as the signer's name. This is used for SIG(0) calculation only. - // It will be overwritten with host name compressed. + switch (aMode) + { + case kAppendEmptySignature: + aInfo.mSigRecordOffset = aInfo.mMessage->GetLength(); + break; - offset = aInfo.mMessage->GetLength(); - SuccessOrExit(error = aInfo.mMessage->Append(sig)); - SuccessOrExit(error = AppendHostName(aInfo, /* aDoNotCompress */ true)); + case kOverwriteWithNewSignature: + // Revert back to the start of signature record. + IgnoreError(aInfo.mMessage->SetLength(aInfo.mSigRecordOffset)); - // Calculate signature (RFC 2931): Calculated over "data" which is - // concatenation of (1) the SIG RR RDATA wire format (including - // the canonical form of the signer's name), entirely omitting the - // signature subfield, (2) DNS query message, including DNS header - // but not UDP/IP header before the header RR counts have been - // adjusted for the inclusion of SIG(0). + // Append the SIG RR with full uncompressed form of the host name + // as the signer's name. This is used for SIG(0) calculation only. + // It will be overwritten with host name compressed. - sha256.Start(); + offset = aInfo.mMessage->GetLength(); + SuccessOrExit(error = aInfo.mMessage->Append(sig)); + SuccessOrExit(error = AppendHostName(aInfo, /* aDoNotCompress */ true)); - // (1) SIG RR RDATA wire format - len = aInfo.mMessage->GetLength() - offset - sizeof(Dns::ResourceRecord); - sha256.Update(*aInfo.mMessage, offset + sizeof(Dns::ResourceRecord), len); + // Calculate signature (RFC 2931): Calculated over "data" which is + // concatenation of (1) the SIG RR RDATA wire format (including + // the canonical form of the signer's name), entirely omitting the + // signature subfield, (2) DNS query message, including DNS header + // but not UDP/IP header before the header RR counts have been + // adjusted for the inclusion of SIG(0). - // (2) Message from DNS header before SIG - sha256.Update(*aInfo.mMessage, 0, offset); + sha256.Start(); - sha256.Finish(hash); - SuccessOrExit(error = aInfo.mKeyInfo.Sign(hash, signature)); + // (1) SIG RR RDATA wire format + len = aInfo.mMessage->GetLength() - offset - sizeof(Dns::ResourceRecord); + sha256.Update(*aInfo.mMessage, offset + sizeof(Dns::ResourceRecord), len); - // Move back in message and append SIG RR now with compressed host - // name (as signer's name) along with the calculated signature. + // (2) Message from DNS header before SIG + sha256.Update(*aInfo.mMessage, 0, offset); - IgnoreError(aInfo.mMessage->SetLength(offset)); + sha256.Finish(hash); + SuccessOrExit(error = aInfo.mKeyInfo.Sign(hash, signature)); + + // Move back in message and append SIG RR now with compressed host + // name (as signer's name) along with the calculated signature. + + IgnoreError(aInfo.mMessage->SetLength(offset)); + break; + } // SIG(0) uses owner name of root (single zero byte). SuccessOrExit(error = Dns::Name::AppendTerminator(*aInfo.mMessage)); @@ -1782,8 +1796,7 @@ void Client::ProcessResponse(Message &aMessage) VerifyOrExit(header.GetType() == Dns::Header::kTypeResponse, error = kErrorParse); VerifyOrExit(header.GetQueryType() == Dns::Header::kQueryTypeUpdate, error = kErrorParse); - VerifyOrExit(IsResponseMessageIdValid(header.GetMessageId()), error = kErrorDrop); - mResponseMessageId = header.GetMessageId() + 1; + VerifyOrExit(header.GetMessageId() == mCurMessageId, error = kErrorDrop); if (!Get().IsRxOnWhenIdle()) { @@ -1921,11 +1934,14 @@ exit: } } -bool Client::IsResponseMessageIdValid(uint16_t aId) const +void Client::SelectNewMessageId(void) { - // Semantically equivalent to `(aId >= mResponseMessageId) && (aId < mNextMessageId)` + uint16_t oldId = mCurMessageId; - return !SerialNumber::IsLess(aId, mResponseMessageId) && SerialNumber::IsLess(aId, mNextMessageId); + do + { + mCurMessageId = Random::NonCrypto::GetUint16(); + } while (oldId == mCurMessageId); } void Client::HandleUpdateDone(void) diff --git a/src/core/net/srp_client.hpp b/src/core/net/srp_client.hpp index fd54fd6a3..afaeb05ad 100644 --- a/src/core/net/srp_client.hpp +++ b/src/core/net/srp_client.hpp @@ -734,15 +734,6 @@ public: */ bool GetUseShortLeaseOption(void) const { return mUseShortLeaseOption; } - /** - * Set the next DNS message ID for client to use. - * - * This is intended for testing only. - * - * @pram[in] aMessageId A message ID. - */ - void SetNextMessageId(uint16_t aMessageId) { mNextMessageId = aMessageId; } - #endif // OPENTHREAD_CONFIG_REFERENCE_DEVICE_ENABLE private: @@ -903,6 +894,13 @@ private: kForServicesAppendedInMessage, }; + // Used in `AppendSignature()` + enum SignatureAppendMode : uint8_t + { + kAppendEmptySignature, + kOverwriteWithNewSignature, + }; + #if OPENTHREAD_CONFIG_PLATFORM_KEY_REFERENCES_ENABLE typedef Crypto::Ecdsa::P256::KeyPairAsRef KeyInfo; #else @@ -995,6 +993,7 @@ private: uint16_t mDomainNameOffset; uint16_t mHostNameOffset; uint16_t mRecordCount; + uint16_t mSigRecordOffset; KeyInfo mKeyInfo; }; @@ -1017,6 +1016,7 @@ private: void HandleHostInfoOrServiceChange(void); void SendUpdate(void); Error PrepareUpdateMessage(MsgInfo &aInfo); + Error UpdateIdAndSignatureInUpdateMessage(MsgInfo &aInfo); Error ReadOrGenerateKey(KeyInfo &aKeyInfo); Error AppendServiceInstructions(MsgInfo &aInfo); bool CanAppendService(const Service &aService); @@ -1027,10 +1027,10 @@ private: Error AppendHostName(MsgInfo &aInfo, bool aDoNotCompress = false) const; Error AppendAaaaRecord(const Ip6::Address &aAddress, MsgInfo &aInfo) const; Error AppendUpdateLeaseOptRecord(MsgInfo &aInfo); - Error AppendSignature(MsgInfo &aInfo); + Error AppendSignature(MsgInfo &aInfo, SignatureAppendMode aMode); void HandleUdpReceive(Message &aMessage, const Ip6::MessageInfo &aMessageInfo); void ProcessResponse(Message &aMessage); - bool IsResponseMessageIdValid(uint16_t aId) const; + void SelectNewMessageId(void); void HandleUpdateDone(void); void GetRemovedServices(LinkedList &aRemovedServices); static Error ReadResourceRecord(const Message &aMessage, uint16_t &aOffset, Dns::ResourceRecord &aRecord); @@ -1080,8 +1080,7 @@ private: bool mUseShortLeaseOption : 1; #endif - uint16_t mNextMessageId; - uint16_t mResponseMessageId; + uint16_t mCurMessageId; uint16_t mAutoHostAddressCount; uint32_t mRetryWaitInterval; diff --git a/tests/unit/test_srp_server.cpp b/tests/unit/test_srp_server.cpp index cc03dc61a..d9db36a08 100644 --- a/tests/unit/test_srp_server.cpp +++ b/tests/unit/test_srp_server.cpp @@ -1063,7 +1063,7 @@ void TestSrpClientDelayedResponse(void) srpClient = &sInstance->Get(); - for (uint8_t testIter = 0; testIter < 3; testIter++) + for (uint8_t testIter = 0; testIter < 2; testIter++) { Log("- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -"); Log("testIter = %u", testIter); @@ -1074,6 +1074,7 @@ void TestSrpClientDelayedResponse(void) Ip6::Udp::Socket udpSocket(*sInstance, HandleServerUdpReceive, nullptr); Ip6::SockAddr serverSockAddr; uint16_t firstMsgId; + uint16_t secondMsgId; Message *response; Dns::UpdateHeader header; @@ -1082,24 +1083,6 @@ void TestSrpClientDelayedResponse(void) SuccessOrQuit(udpSocket.Open(Ip6::kNetifThreadInternal)); SuccessOrQuit(udpSocket.Bind(kServerPort)); - //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - // Manually start the client with a message ID based on `testIter` - // We use zero in the first iteration, `0xffff` in the second - // iteration to test wrapping of 16-bit message ID. - - switch (testIter) - { - case 0: - srpClient->SetNextMessageId(0); - break; - case 1: - srpClient->SetNextMessageId(0xffff); - break; - case 2: - srpClient->SetNextMessageId(0xaaaa); - break; - } - serverSockAddr.SetAddress(sInstance->Get().GetMeshLocalRloc()); serverSockAddr.SetPort(kServerPort); SuccessOrQuit(srpClient->Start(serverSockAddr)); @@ -1120,36 +1103,44 @@ void TestSrpClientDelayedResponse(void) AdvanceTime(1 * 1000); VerifyOrQuit(sServerRxCount == 1); - firstMsgId = sServerLastMsgId; + firstMsgId = sServerLastMsgId; + sServerRxCount = 0; + + if (testIter == 1) + { + // In the second test iteration, register a second + // service. Ensure that client uses a new ID for new + // updated SRP message (containing both services). + + AdvanceTime(5 * 1000); + + PrepareService2(service2); + SuccessOrQuit(srpClient->AddService(service2)); + + AdvanceTime(20 * 1000); + VerifyOrQuit(sServerRxCount > 1); + VerifyOrQuit(sServerLastMsgId != firstMsgId); + secondMsgId = sServerLastMsgId; + sServerRxCount = 0; + } + + //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - + // Wait for longer to allow client to retry a bunch of times. + // Ensure the same ID is used for retries. + + AdvanceTime(60 * 1000); + VerifyOrQuit(sServerRxCount > 1); switch (testIter) { case 0: - VerifyOrQuit(firstMsgId == 0); + VerifyOrQuit(sServerLastMsgId == firstMsgId); break; case 1: - VerifyOrQuit(firstMsgId == 0xffff); - break; - case 2: - VerifyOrQuit(firstMsgId == 0xaaaa); + VerifyOrQuit(sServerLastMsgId == secondMsgId); break; } - if (testIter == 2) - { - AdvanceTime(2 * 1000); - - PrepareService2(service2); - SuccessOrQuit(srpClient->AddService(service2)); - } - - //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - // Wait for longer to allow client to retry a bunch of times - - AdvanceTime(20 * 1000); - VerifyOrQuit(sServerRxCount > 1); - VerifyOrQuit(sServerLastMsgId != firstMsgId); - VerifyOrQuit(srpClient->GetHostInfo().GetState() != Srp::Client::kRegistered); VerifyOrQuit(service1.GetState() != Srp::Client::kRegistered); @@ -1171,20 +1162,19 @@ void TestSrpClientDelayedResponse(void) AdvanceTime(10); //- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - // In the first two iterations, we ensure that client - // did successfully accept the response with older message ID. - // This should not be the case in the third iteration due to - // changes to client services after first UPdate message was - // sent by client. + // In the first test iteration, we ensure that client did + // successfully accept the response with first message ID. + // This should not be the case in the second iteration due to + // changes to client services after the first Update message + // was sent by client. switch (testIter) { case 0: - case 1: VerifyOrQuit(srpClient->GetHostInfo().GetState() == Srp::Client::kRegistered); VerifyOrQuit(service1.GetState() == Srp::Client::kRegistered); break; - case 2: + case 1: VerifyOrQuit(srpClient->GetHostInfo().GetState() != Srp::Client::kRegistered); VerifyOrQuit(service1.GetState() != Srp::Client::kRegistered); break;