bug 1460327 - make the DNS packet decoder verify the answer qname r=mcmanus

... and before chasing a cname, check if the address record for that
CNAME is actually in fact already provided in the DNS packet that passed
on the CNAME!

Some existing tests ajusted for this. Two new tests added.

MozReview-Commit-ID: CBMO7N7jMEX

--HG--
extra : rebase_source : 1110a8df6d89fcbb0ad2a35b3762b837ce7a1e18

netwerk/dns/TRR.cpp

@@ -485,11 +485,71 @@ TRR::PassQName(unsigned int &index)
   return NS_OK;
 }
 
+// GetQname: retrieves the qname (stores in 'aQname') and stores the index
+// after qname was parsed into the 'aIndex'.
+
+nsresult
+TRR::GetQname(nsAutoCString &aQname, unsigned int &aIndex)
+{
+  uint8_t clength = 0;
+  unsigned int cindex = aIndex;
+  unsigned int loop = 128; // a valid DNS name can never loop this much
+  unsigned int endindex = 0; // index position after this data
+  do {
+    if (cindex >= mBodySize) {
+      LOG(("TRR: bad cname packet\n"));
+      return NS_ERROR_ILLEGAL_VALUE;
+    }
+    clength = static_cast<uint8_t>(mResponse[cindex]);
+    if ((clength & 0xc0) == 0xc0) {
+      // name pointer, get the new offset (14 bits)
+      if ((cindex +1) >= mBodySize) {
+        return NS_ERROR_ILLEGAL_VALUE;
+      }
+      // extract the new index position for the next label
+      uint16_t newpos = (clength & 0x3f) << 8 | mResponse[cindex+1];
+      if (!endindex) {
+        // only update on the first "jump"
+        endindex = cindex + 2;
+      }
+      cindex = newpos;
+      continue;
+    } else if (clength & 0xc0) {
+      // any of those bits set individually is an error
+      LOG(("TRR: bad cname packet\n"));
+      return NS_ERROR_ILLEGAL_VALUE;
+    } else {
+      cindex++;
+    }
+    if (clength) {
+      if (!aQname.IsEmpty()) {
+        aQname.Append(".");
+      }
+      if ((cindex + clength) > mBodySize) {
+        return NS_ERROR_ILLEGAL_VALUE;
+      }
+      aQname.Append((const char *)(&mResponse[cindex]), clength);
+      cindex += clength; // skip label
+    }
+  } while (clength && --loop);
+
+  if (!loop) {
+    LOG(("TRR::DohDecode pointer loop error\n"));
+    return NS_ERROR_ILLEGAL_VALUE;
+  }
+  if (!endindex) {
+    // there was no "jump"
+    endindex = cindex;
+  }
+  aIndex = endindex;
+  return NS_OK;
+}
+
 //
 // DohDecode() collects the TTL and the IP addresses in the response
 //
 nsresult
-TRR::DohDecode()
+TRR::DohDecode(nsCString &aHost)
 {
   // The response has a 12 byte header followed by the question (returned)
   // and then the answer. The answer section itself contains the name, type
@@ -509,7 +569,7 @@ TRR::DohDecode()
   nsAutoCString host;
   nsresult rv;
 
-  LOG(("doh decode %s %d bytes\n", mHost.get(), mBodySize));
+  LOG(("doh decode %s %d bytes\n", aHost.get(), mBodySize));
 
   mCname.Truncate();
 
@@ -519,7 +579,7 @@ TRR::DohDecode()
   }
   uint8_t rcode = mResponse[3] & 0x0F;
   if (rcode) {
-    LOG(("TRR Decode %s RCODE %d\n", mHost.get(), rcode));
+    LOG(("TRR Decode %s RCODE %d\n", aHost.get(), rcode));
     return NS_ERROR_FAILURE;
   }
 
@@ -556,7 +616,8 @@ TRR::DohDecode()
        answerRecords, mBodySize, host.get(), index));
 
   while (answerRecords) {
-    rv = PassQName(index);
+    nsAutoCString qname;
+    rv = GetQname(qname, index);
     if (NS_FAILED(rv)) {
       return rv;
     }
@@ -610,94 +671,70 @@ TRR::DohDecode()
       return NS_ERROR_ILLEGAL_VALUE;
     }
 
-    // RDATA
+    if (qname.Equals(aHost)) {
-    // - A (TYPE 1):  4 bytes
-    // - AAAA (TYPE 28): 16 bytes
-    // - NS (TYPE 2): N bytes
 
-    switch(TYPE) {
+      // RDATA
-    case TRRTYPE_A:
+      // - A (TYPE 1):  4 bytes
-      if (RDLENGTH != 4) {
+      // - AAAA (TYPE 28): 16 bytes
-        LOG(("TRR bad length for A (%u)\n", RDLENGTH));
+      // - NS (TYPE 2): N bytes
-        return NS_ERROR_UNEXPECTED;
-      }
-      rv = mDNS.Add(TTL, mResponse, index, RDLENGTH,
-                    mAllowRFC1918);
-      if (NS_FAILED(rv)) {
-        LOG(("TRR:DohDecode failed: local IP addresses or unknown IP family\n"));
-        return rv;
-      }
-      break;
-    case TRRTYPE_AAAA:
-      if (RDLENGTH != 16) {
-        LOG(("TRR bad length for AAAA (%u)\n", RDLENGTH));
-        return NS_ERROR_UNEXPECTED;
-      }
-      rv = mDNS.Add(TTL, mResponse, index, RDLENGTH,
-                    mAllowRFC1918);
-      if (NS_FAILED(rv)) {
-        LOG(("TRR got unique/local IPv6 address!\n"));
-        return rv;
-      }
-      break;
 
-    case TRRTYPE_NS:
+      switch(TYPE) {
-      break;
+      case TRRTYPE_A:
-    case TRRTYPE_CNAME:
+        if (RDLENGTH != 4) {
-      if (mCname.IsEmpty()) {
+          LOG(("TRR bad length for A (%u)\n", RDLENGTH));
-        uint8_t clength = 0;
+          return NS_ERROR_UNEXPECTED;
-        unsigned int cindex = index;
-        unsigned int loop = 128; // a valid DNS name can never loop this much
-        do {
-          if (cindex >= mBodySize) {
-            LOG(("TRR: bad cname packet\n"));
-            return NS_ERROR_ILLEGAL_VALUE;
-          }
-          clength = static_cast<uint8_t>(mResponse[cindex]);
-          if ((clength & 0xc0) == 0xc0) {
-            // name pointer, get the new offset (14 bits)
-            if ((cindex +1) >= mBodySize) {
-              return NS_ERROR_ILLEGAL_VALUE;
-            }
-            // extract the new index position for the next label
-            uint16_t newpos = (clength & 0x3f) << 8 | mResponse[cindex+1];
-            cindex = newpos;
-            continue;
-          } else if (clength & 0xc0) {
-            // any of those bits set individually is an error
-            LOG(("TRR: bad cname packet\n"));
-            return NS_ERROR_ILLEGAL_VALUE;
-          } else {
-            cindex++;
-          }
-          if (clength) {
-            if (!mCname.IsEmpty()) {
-              mCname.Append(".");
-            }
-            if ((cindex + clength) > mBodySize) {
-              return NS_ERROR_ILLEGAL_VALUE;
-            }
-            mCname.Append((const char *)(&mResponse[cindex]), clength);
-            cindex += clength; // skip label
-          }
-        } while (clength && --loop);
-
-        if (!loop) {
-          LOG(("TRR::DohDecode pointer loop error\n"));
-          return NS_ERROR_ILLEGAL_VALUE;
         }
+        rv = mDNS.Add(TTL, mResponse, index, RDLENGTH,
+                      mAllowRFC1918);
+        if (NS_FAILED(rv)) {
+          LOG(("TRR:DohDecode failed: local IP addresses or unknown IP family\n"));
+          return rv;
+        }
+        break;
+      case TRRTYPE_AAAA:
+        if (RDLENGTH != 16) {
+          LOG(("TRR bad length for AAAA (%u)\n", RDLENGTH));
+          return NS_ERROR_UNEXPECTED;
+        }
+        rv = mDNS.Add(TTL, mResponse, index, RDLENGTH,
+                      mAllowRFC1918);
+        if (NS_FAILED(rv)) {
+          LOG(("TRR got unique/local IPv6 address!\n"));
+          return rv;
+        }
+        break;
 
-        LOG(("TRR::DohDecode CNAME host %s => %s\n",
+      case TRRTYPE_NS:
-             host.get(), mCname.get()));
+        break;
+      case TRRTYPE_CNAME:
+        if (mCname.IsEmpty()) {
+          nsAutoCString qname;
+          unsigned int qnameindex = index;
+          rv = GetQname(qname, qnameindex);
+          if (NS_FAILED(rv)) {
+            return rv;
+          }
+          if(!qname.IsEmpty()) {
+            mCname = qname;
+            LOG(("TRR::DohDecode CNAME host %s => %s\n",
+                 host.get(), mCname.get()));
+          } else {
+            LOG(("TRR::DohDecode empty CNAME for host %s!\n",
+                 host.get()));
+          }
+        }
+        else {
+          LOG(("TRR::DohDecode CNAME - ignoring another entry\n"));
+        }
+        break;
+      default:
+        // skip unknown record types
+        LOG(("TRR unsupported TYPE (%u) RDLENGTH %u\n", TYPE, RDLENGTH));
+        break;
       }
-      else {
+    }
-        LOG(("TRR::DohDecode CNAME - ignoring another entry\n"));
+    else {
-      }
+      LOG(("TRR asked for %s data but got %s\n", aHost.get(), qname.get()));
-      break;
-    default:
-      // skip unknown record types
-      LOG(("TRR unsupported TYPE (%u) RDLENGTH %u\n", TYPE, RDLENGTH));
-      break;
     }
 
     index += RDLENGTH;
@@ -832,10 +869,21 @@ nsresult
 TRR::On200Response()
 {
   // decode body and create an AddrInfo struct for the response
-  nsresult rv = DohDecode();
+  nsresult rv = DohDecode(mHost);
 
   if (NS_SUCCEEDED(rv)) {
     if (!mDNS.mAddresses.getFirst() && !mCname.IsEmpty()) {
+      nsCString cname = mCname;
+      LOG(("TRR: check for CNAME record for %s within previous response\n",
+           cname.get()));
+      rv = DohDecode(cname);
+      if (NS_SUCCEEDED(rv) && mDNS.mAddresses.getFirst()) {
+        LOG(("TRR: Got the CNAME record without asking for it\n"));
+        ReturnData();
+        return NS_OK;
+      }
+      // restore mCname as DohDecode() change it
+      mCname = cname;
       if (!--mCnameLoop) {
         LOG(("TRR::On200Response CNAME loop, eject!\n"));
       } else  {

netwerk/dns/TRR.h

@@ -143,7 +143,8 @@ private:
   nsresult SendHTTPRequest();
   nsresult DohEncode(nsCString &target);
   nsresult PassQName(unsigned int &index);
-  nsresult DohDecode();
+  nsresult GetQname(nsAutoCString &aQname, unsigned int &aIndex);
+  nsresult DohDecode(nsCString &aHost);
   nsresult ReturnData();
   nsresult FailData();
   nsresult DohDecodeQuery(const nsCString &query,

netwerk/test/unit/test_trr.js

@@ -199,7 +199,7 @@ function test3()
   prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/dns-auth");
   prefs.setCharPref("network.trr.credentials", "user:password");
   test_answer = "127.0.0.1";
-  listen = dns.asyncResolve("auth.example.com", 0, listenerFine, mainThread, defaultOriginAttributes);
+  listen = dns.asyncResolve("bar.example.com", 0, listenerFine, mainThread, defaultOriginAttributes);
 }
 
 // verify failing credentials in DOH request
@@ -284,7 +284,8 @@ function test10()
   } catch (e) {
     // NS_ERROR_UNKNOWN_HOST exception is expected
     do_test_finished();
-    run_dns_tests();
+    do_timeout(200, run_dns_tests);
+    //run_dns_tests();
   }
 }
 
@@ -403,12 +404,30 @@ function test20()
 // TRR-shadow and a CNAME loop
 function test21()
 {
-  prefs.setIntPref("network.trr.mode", 4); // TRR-first
+  prefs.setIntPref("network.trr.mode", 4); // shadow
   prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/dns-cname-loop");
   test_answer = "127.0.0.1";
   listen = dns.asyncResolve("test21.example.com", 0, listenerFine, mainThread, defaultOriginAttributes);
 }
 
+// verify that basic A record name mismatch gets rejected. Gets the same DOH
+// response back as test1
+function test22()
+{
+  prefs.setIntPref("network.trr.mode", 3); // TRR-only to avoid native fallback
+  prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/dns");
+  listen = dns.asyncResolve("mismatch.example.com", 0, listenerFails, mainThread, defaultOriginAttributes);
+}
+
+// TRR-only, with a CNAME response with a bundled A record for that CNAME!
+function test23()
+{
+  prefs.setIntPref("network.trr.mode", 3); // TRR-only
+  prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/dns-cname-a");
+  test_answer = "9.8.7.6";
+  listen = dns.asyncResolve("cname-a.example.com", 0, listenerFine, mainThread, defaultOriginAttributes);
+}
+
 var tests = [ test1,
               test1b,
               test2,
@@ -433,6 +452,8 @@ var tests = [ test1,
               test19,
               test20,
               test21,
+              test22,
+              test23,
               testsDone
             ];
 

testing/xpcshell/moz-http2/moz-http2.js

@@ -549,6 +549,47 @@ function handleRequest(req, res) {
     res.end("");
     return;
 
+  }
+  else if (u.pathname === "/dns-cname-a") {
+    // test23 asks for cname-a.example.com
+    // this responds with a CNAME to here.example.com *and* an A record
+    // for here.example.com
+    var content;
+
+    content = new Buffer("0000" +
+                         "0100" +
+                         "0001" + // QDCOUNT
+                         "0002" + // ANCOUNT
+                         "00000000" + // NSCOUNT + ARCOUNT
+                         "07636E616D652d61" + // cname-a
+                         "076578616D706C6503636F6D00" + // .example.com
+                         "00010001" + // question type (A) + question class (IN)
+
+                         // answer record 1
+                         "C00C" + // name pointer to cname-a.example.com
+                         "0005" + // type (CNAME)
+                         "0001" + // class
+                         "00000037" + // TTL
+                         "0012" +   // RDLENGTH
+                         "0468657265" + // here
+                         "076578616D706C6503636F6D00" + // .example.com
+
+                         // answer record 2, the A entry for the CNAME above
+                         "0468657265" + // here
+                         "076578616D706C6503636F6D00" + // .example.com
+                         "0001" + // type (A)
+                         "0001" + // class
+                         "00000037" + // TTL
+                         "0004" + // RDLENGTH
+                         "09080706", // IPv4 address
+                         "hex");
+    res.setHeader('Content-Type', 'application/dns-udpwireformat');
+    res.setHeader('Content-Length', content.length);
+    res.writeHead(200);
+    res.write(content);
+    res.end("");
+    return;
+
   }
   else if (u.pathname === "/dns-cname-loop") {
     // asking for cname.example.com
@@ -611,9 +652,17 @@ function handleRequest(req, res) {
       // confirm.example.com has NS entry ns.example.com
       var content= new Buffer("00000100000100010000000007636F6E6669726D076578616D706C6503636F6D0000020001C00C00020001000000370012026E73076578616D706C6503636F6D010A00", "hex");
       ns_confirm++;
+    } else if (2 >= ns_confirm) {
+      // next response: 10b-100.example.com has AAAA entry 1::FFFF
+
+      // we expect two requests for this name (A + AAAA), respond identically
+      // for both and expect the client to reject the wrong one
+      var content= new Buffer("000001000001000100000000" + "073130622d313030" +
+                              "076578616D706C6503636F6D00001C0001C00C001C00010000003700100001000000000000000000000000FFFF", "hex");
+      ns_confirm++;
     } else {
-      // next response: wrong.example.com has AAAA entry 1::FFFF
+      // everything else is just wrong
-      var content= new Buffer("0000010000010001000000000577726F6E67076578616D706C6503636F6D00001C0001C00C001C00010000003700100001000000000000000000000000FFFF", "hex");
+      return;
     }
     res.setHeader('Content-Type', 'application/dns-udpwireformat');
     res.setHeader('Content-Length', content.length);