From 06c7a72bf8291d346f1eaa54dee724b77c1d4320 Mon Sep 17 00:00:00 2001 From: Daniel Stenberg Date: Tue, 8 May 2018 19:30:07 +0200 Subject: [PATCH] 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 | 224 ++++++++++++++---------- netwerk/dns/TRR.h | 3 +- netwerk/test/unit/test_trr.js | 27 ++- testing/xpcshell/moz-http2/moz-http2.js | 53 +++++- 4 files changed, 213 insertions(+), 94 deletions(-) diff --git a/netwerk/dns/TRR.cpp b/netwerk/dns/TRR.cpp index 25489193adc4..b8b6820bb7c3 100644 --- a/netwerk/dns/TRR.cpp +++ b/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(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 - // - A (TYPE 1): 4 bytes - // - AAAA (TYPE 28): 16 bytes - // - NS (TYPE 2): N bytes + if (qname.Equals(aHost)) { - switch(TYPE) { - case TRRTYPE_A: - if (RDLENGTH != 4) { - LOG(("TRR bad length for A (%u)\n", RDLENGTH)); - 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; + // RDATA + // - A (TYPE 1): 4 bytes + // - AAAA (TYPE 28): 16 bytes + // - NS (TYPE 2): N bytes - case TRRTYPE_NS: - break; - case TRRTYPE_CNAME: - if (mCname.IsEmpty()) { - uint8_t clength = 0; - 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(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; + switch(TYPE) { + case TRRTYPE_A: + if (RDLENGTH != 4) { + LOG(("TRR bad length for A (%u)\n", RDLENGTH)); + 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; - LOG(("TRR::DohDecode CNAME host %s => %s\n", - host.get(), mCname.get())); + case TRRTYPE_NS: + 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")); - } - break; - default: - // skip unknown record types - LOG(("TRR unsupported TYPE (%u) RDLENGTH %u\n", TYPE, RDLENGTH)); - break; + } + else { + LOG(("TRR asked for %s data but got %s\n", aHost.get(), qname.get())); } 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 { diff --git a/netwerk/dns/TRR.h b/netwerk/dns/TRR.h index d982809ca38e..3df505bf3ed8 100644 --- a/netwerk/dns/TRR.h +++ b/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, diff --git a/netwerk/test/unit/test_trr.js b/netwerk/test/unit/test_trr.js index 05a1bddc8558..40203cc1c240 100644 --- a/netwerk/test/unit/test_trr.js +++ b/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 ]; diff --git a/testing/xpcshell/moz-http2/moz-http2.js b/testing/xpcshell/moz-http2/moz-http2.js index 4d966d0ec6fb..f39083fcac19 100644 --- a/testing/xpcshell/moz-http2/moz-http2.js +++ b/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 - var content= new Buffer("0000010000010001000000000577726F6E67076578616D706C6503636F6D00001C0001C00C001C00010000003700100001000000000000000000000000FFFF", "hex"); + // everything else is just wrong + return; } res.setHeader('Content-Type', 'application/dns-udpwireformat'); res.setHeader('Content-Length', content.length);