From 70adcabca6270eead21f126533c697d4d1637b77 Mon Sep 17 00:00:00 2001
From: Abtin Keshavarzian <abtink@google.com>
Date: Fri, 4 Aug 2023 15:27:31 -0700
Subject: [PATCH] [dns-client] handle multiple CNAME record in response (#9339)

This commit updates the `CheckForHostNameAlias()` method to handle the
case where the response may include multiple CNAME records. For
example, host name mapped to another name which is itself mapped
again. In order to detect CNAME record loops, we limit the number
of CNAME record name changes to `kMaxCnameAliasNameChanges = 40`.
---
 src/core/net/dns_client.cpp | 36 ++++++++++++++++++++++--------------
 src/core/net/dns_client.hpp |  2 ++
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/src/core/net/dns_client.cpp b/src/core/net/dns_client.cpp
index 2dd9531de..ef3fb3da2 100644
--- a/src/core/net/dns_client.cpp
+++ b/src/core/net/dns_client.cpp
@@ -168,7 +168,10 @@ Error Client::Response::CheckForHostNameAlias(Section aSection, Name &aHostName)
 {
     // If the response includes a CNAME record mapping the query host
     // name to a canonical name, we update `aHostName` to the new alias
-    // name. Otherwise `aHostName` remains as before.
+    // name. Otherwise `aHostName` remains as before. This method handles
+    // when there are multiple CNAME records mapping the host name multiple
+    // times. We limit number of changes to `kMaxCnameAliasNameChanges`
+    // to detect and handle if the response contains CNAME record loops.
 
     Error       error;
     uint16_t    offset;
@@ -177,27 +180,32 @@ Error Client::Response::CheckForHostNameAlias(Section aSection, Name &aHostName)
 
     VerifyOrExit(mMessage != nullptr, error = kErrorNotFound);
 
-    SelectSection(aSection, offset, numRecords);
-    error = ResourceRecord::FindRecord(*mMessage, offset, numRecords, /* aIndex */ 0, aHostName, cnameRecord);
-
-    switch (error)
+    for (uint16_t counter = 0; counter < kMaxCnameAliasNameChanges; counter++)
     {
-    case kErrorNone:
+        SelectSection(aSection, offset, numRecords);
+        error = ResourceRecord::FindRecord(*mMessage, offset, numRecords, /* aIndex */ 0, aHostName, cnameRecord);
+
+        if (error == kErrorNotFound)
+        {
+            error = kErrorNone;
+            ExitNow();
+        }
+
+        SuccessOrExit(error);
+
         // A CNAME record was found. `offset` now points to after the
         // last read byte within the `mMessage` into the `cnameRecord`
         // (which is the start of the new canonical name).
+
         aHostName.SetFromMessage(*mMessage, offset);
-        error = Name::ParseName(*mMessage, offset);
-        break;
+        SuccessOrExit(error = Name::ParseName(*mMessage, offset));
 
-    case kErrorNotFound:
-        error = kErrorNone;
-        break;
-
-    default:
-        break;
+        // Loop back to check if there may be a CNAME record for the
+        // new `aHostName`.
     }
 
+    error = kErrorParse;
+
 exit:
     return error;
 }
diff --git a/src/core/net/dns_client.hpp b/src/core/net/dns_client.hpp
index d663c91f8..e086f746b 100644
--- a/src/core/net/dns_client.hpp
+++ b/src/core/net/dns_client.hpp
@@ -770,6 +770,8 @@ public:
 #endif // OPENTHREAD_CONFIG_DNS_CLIENT_SERVICE_DISCOVERY_ENABLE
 
 private:
+    static constexpr uint16_t kMaxCnameAliasNameChanges = 40;
+
     enum QueryType : uint8_t
     {
         kIp6AddressQuery, // IPv6 Address resolution.