From 442b7e82f67617cf754ed905d8e1dec9e1f9de80 Mon Sep 17 00:00:00 2001 From: Kangping Date: Fri, 7 Jan 2022 13:47:04 +0800 Subject: [PATCH] [srp-server] identify name conflicts by comparing only ECDSA public keys (#7281) Current `Srp::Server` is comparing the entire KEY RRs to determine if the two SRP messages are using the same signing key. But the TTL in the KEY RR can change even between two SRP messages of the same client and this will result in false name conflicts. This commit fixes this issue by changing to compare only the rdata of the KEY RR. --- src/core/crypto/ecdsa.hpp | 2 +- src/core/net/srp_server.cpp | 34 +++++++++++++++++----------------- src/core/net/srp_server.hpp | 25 ++++++++++++++----------- 3 files changed, 32 insertions(+), 29 deletions(-) diff --git a/src/core/crypto/ecdsa.hpp b/src/core/crypto/ecdsa.hpp index 7111363a4..6f9d9b3dd 100644 --- a/src/core/crypto/ecdsa.hpp +++ b/src/core/crypto/ecdsa.hpp @@ -228,7 +228,7 @@ public: * */ OT_TOOL_PACKED_BEGIN - class PublicKey + class PublicKey : public Equatable { friend class KeyPair; diff --git a/src/core/net/srp_server.cpp b/src/core/net/srp_server.cpp index db96e6dbd..cee8633ad 100644 --- a/src/core/net/srp_server.cpp +++ b/src/core/net/srp_server.cpp @@ -308,7 +308,7 @@ bool Server::HasNameConflictsWith(Host &aHost) const bool hasConflicts = false; const Host *existingHost = mHosts.FindMatching(aHost.GetFullName()); - if (existingHost != nullptr && *aHost.GetKey() != *existingHost->GetKey()) + if (existingHost != nullptr && aHost.GetKeyRecord()->GetKey() != existingHost->GetKeyRecord()->GetKey()) { ExitNow(hasConflicts = true); } @@ -323,7 +323,7 @@ bool Server::HasNameConflictsWith(Host &aHost) const { if (host.HasServiceInstance(service.GetInstanceName())) { - VerifyOrExit(*aHost.GetKey() == *host.GetKey(), hasConflicts = true); + VerifyOrExit(aHost.GetKeyRecord()->GetKey() == host.GetKeyRecord()->GetKey(), hasConflicts = true); } } } @@ -799,14 +799,14 @@ Error Server::ProcessHostDescriptionInstruction(Host & aHost, else if (record.GetType() == Dns::ResourceRecord::kTypeKey) { // We currently support only ECDSA P-256. - Dns::Ecdsa256KeyRecord key; + Dns::Ecdsa256KeyRecord keyRecord; VerifyOrExit(record.GetClass() == aMetadata.mDnsZone.GetClass(), error = kErrorFailed); - SuccessOrExit(error = aMessage.Read(offset, key)); - VerifyOrExit(key.IsValid(), error = kErrorParse); + SuccessOrExit(error = aMessage.Read(offset, keyRecord)); + VerifyOrExit(keyRecord.IsValid(), error = kErrorParse); - VerifyOrExit(aHost.GetKey() == nullptr || *aHost.GetKey() == key, error = kErrorSecurity); - aHost.SetKey(key); + VerifyOrExit(aHost.GetKeyRecord() == nullptr || *aHost.GetKeyRecord() == keyRecord, error = kErrorSecurity); + aHost.SetKeyRecord(keyRecord); } offset += record.GetSize(); @@ -815,7 +815,7 @@ Error Server::ProcessHostDescriptionInstruction(Host & aHost, // Verify that we have a complete Host Description Instruction. VerifyOrExit(aHost.GetFullName() != nullptr, error = kErrorFailed); - VerifyOrExit(aHost.GetKey() != nullptr, error = kErrorFailed); + VerifyOrExit(aHost.GetKeyRecord() != nullptr, error = kErrorFailed); // We check the number of host addresses after processing of the // Lease Option in the Addition Section and determining whether @@ -1060,8 +1060,8 @@ Error Server::ProcessAdditionalSection(Host *aHost, const Message &aMessage, Mes VerifyOrExit(sigRecord.GetTypeCovered() == 0, error = kErrorFailed); VerifyOrExit(signatureLength == Crypto::Ecdsa::P256::Signature::kSize, error = kErrorParse); - SuccessOrExit(error = VerifySignature(*aHost->GetKey(), aMessage, aMetadata.mDnsHeader, sigOffset, sigRdataOffset, - sigRecord.GetLength(), signerName)); + SuccessOrExit(error = VerifySignature(*aHost->GetKeyRecord(), aMessage, aMetadata.mDnsHeader, sigOffset, + sigRdataOffset, sigRecord.GetLength(), signerName)); aMetadata.mOffset = offset; @@ -1069,7 +1069,7 @@ exit: return error; } -Error Server::VerifySignature(const Dns::Ecdsa256KeyRecord &aKey, +Error Server::VerifySignature(const Dns::Ecdsa256KeyRecord &aKeyRecord, const Message & aMessage, Dns::UpdateHeader aDnsHeader, uint16_t aSigOffset, @@ -1109,7 +1109,7 @@ Error Server::VerifySignature(const Dns::Ecdsa256KeyRecord &aKey, signatureOffset = aSigRdataOffset + aSigRdataLength - Crypto::Ecdsa::P256::Signature::kSize; SuccessOrExit(error = aMessage.Read(signatureOffset, signature)); - error = aKey.GetKey().Verify(hash, signature); + error = aKeyRecord.GetKey().Verify(hash, signature); exit: FreeMessage(signerNameMessage); @@ -1649,7 +1649,7 @@ Server::Host::Host(Instance &aInstance, TimeMilli aUpdateTime) , mKeyLease(0) , mUpdateTime(aUpdateTime) { - mKey.Clear(); + mKeyRecord.Clear(); } Server::Host::~Host(void) @@ -1682,11 +1682,11 @@ bool Server::Host::Matches(const char *aFullName) const return StringMatch(mFullName.AsCString(), aFullName, kStringCaseInsensitiveMatch); } -void Server::Host::SetKey(Dns::Ecdsa256KeyRecord &aKey) +void Server::Host::SetKeyRecord(Dns::Ecdsa256KeyRecord &aKeyRecord) { - OT_ASSERT(aKey.IsValid()); + OT_ASSERT(aKeyRecord.IsValid()); - mKey = aKey; + mKeyRecord = aKeyRecord; } TimeMilli Server::Host::GetExpireTime(void) const @@ -1811,7 +1811,7 @@ Error Server::Host::MergeServicesAndResourcesFrom(Host &aHost) otLogInfoSrp("[server] update host %s", GetFullName()); mAddresses = aHost.mAddresses; - mKey = aHost.mKey; + mKeyRecord = aHost.mKeyRecord; mLease = aHost.mLease; mKeyLease = aHost.mKeyLease; mUpdateTime = TimerMilli::GetNow(); diff --git a/src/core/net/srp_server.hpp b/src/core/net/srp_server.hpp index 14c80b475..e3cd50339 100644 --- a/src/core/net/srp_server.hpp +++ b/src/core/net/srp_server.hpp @@ -453,13 +453,13 @@ public: uint32_t GetKeyLease(void) const { return mKeyLease; } /** - * This method returns the KEY resource of the host. + * This method returns the KEY resource record of the host. * - * @returns A pointer to the ECDSA P 256 public key if there is valid one. - * `nullptr` if no valid key exists. + * @returns A pointer to the ECDSA P 256 public key resource record + * if there is valid one. `nullptr` if no valid key exists. * */ - const Dns::Ecdsa256KeyRecord *GetKey(void) const { return mKey.IsValid() ? &mKey : nullptr; } + const Dns::Ecdsa256KeyRecord *GetKeyRecord(void) const { return mKeyRecord.IsValid() ? &mKeyRecord : nullptr; } /** * This method returns the expire time (in milliseconds) of the host. @@ -518,7 +518,7 @@ public: ~Host(void); Error SetFullName(const char *aFullName); - void SetKey(Dns::Ecdsa256KeyRecord &aKey); + void SetKeyRecord(Dns::Ecdsa256KeyRecord &aKeyRecord); void SetLease(uint32_t aLease) { mLease = aLease; } void SetKeyLease(uint32_t aKeyLease) { mKeyLease = aKeyLease; } LinkedList &GetServices(void) { return mServices; } @@ -540,11 +540,14 @@ public: Host * mNext; Heap::String mFullName; Array mAddresses; - Dns::Ecdsa256KeyRecord mKey; - uint32_t mLease; // The LEASE time in seconds. - uint32_t mKeyLease; // The KEY-LEASE time in seconds. - TimeMilli mUpdateTime; - LinkedList mServices; + + // TODO(wgtdkp): there is no necessary to save the entire resource + // record, saving only the ECDSA-256 public key should be enough. + Dns::Ecdsa256KeyRecord mKeyRecord; + uint32_t mLease; // The LEASE time in seconds. + uint32_t mKeyLease; // The KEY-LEASE time in seconds. + TimeMilli mUpdateTime; + LinkedList mServices; }; /** @@ -845,7 +848,7 @@ private: void ProcessDnsUpdate(Message &aMessage, MessageMetadata &aMetadata); Error ProcessUpdateSection(Host &aHost, const Message &aMessage, MessageMetadata &aMetadata) const; Error ProcessAdditionalSection(Host *aHost, const Message &aMessage, MessageMetadata &aMetadata) const; - Error VerifySignature(const Dns::Ecdsa256KeyRecord &aKey, + Error VerifySignature(const Dns::Ecdsa256KeyRecord &aKeyRecord, const Message & aMessage, Dns::UpdateHeader aDnsHeader, uint16_t aSigOffset,