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
--- a/netwerk/dns/TRR.cpp
+++ b/netwerk/dns/TRR.cpp
@@ -480,21 +480,81 @@ TRR::PassQName(unsigned int &index)
LOG(("TRR: PassQName:%d fail at index %d\n", __LINE__, index));
return NS_ERROR_ILLEGAL_VALUE;
}
index += 1 + length;
} while (length);
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
// and class again and THEN the record data.
// www.example.com response:
// header:
// abcd 8180 0001 0001 0000 0000
@@ -504,27 +564,27 @@ TRR::DohDecode()
// 03 7777 7707 6578 616d 706c 6503 636f 6d00 0001 0001
// 0000 0080 0004 5db8 d822
unsigned int index = 12;
uint8_t length;
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();
if (mBodySize < 12 || mResponse[0] || mResponse[1]) {
LOG(("TRR bad incoming DOH, eject!\n"));
return NS_ERROR_ILLEGAL_VALUE;
}
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;
}
uint16_t questionRecords = get16bit(mResponse, 4); // qdcount
// iterate over the single(?) host name in question
while (questionRecords) {
do {
if (mBodySize < (index + 1)) {
@@ -551,17 +611,18 @@ TRR::DohDecode()
// Figure out the number of answer records from ANCOUNT
uint16_t answerRecords = get16bit(mResponse, 6);
LOG(("TRR Decode: %d answer records (%u bytes body) %s index=%u\n",
answerRecords, mBodySize, host.get(), index));
while (answerRecords) {
- rv = PassQName(index);
+ nsAutoCString qname;
+ rv = GetQname(qname, index);
if (NS_FAILED(rv)) {
return rv;
}
// 16 bit TYPE
if (mBodySize < (index + 2)) {
LOG(("TRR: Dohdecode:%d fail at index %d\n", __LINE__, index + 2));
return NS_ERROR_ILLEGAL_VALUE;
}
@@ -605,104 +666,80 @@ TRR::DohDecode()
index += 2;
if (mBodySize < (index + RDLENGTH)) {
LOG(("TRR: Dohdecode:%d fail RDLENGTH=%d at index %d\n", __LINE__,
RDLENGTH, index));
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)) {
+
+ // RDATA
+ // - A (TYPE 1): 4 bytes
+ // - AAAA (TYPE 28): 16 bytes
+ // - NS (TYPE 2): N bytes
- 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;
+ 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;
- 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;
+ 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;
}
- 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;
+ if(!qname.IsEmpty()) {
+ mCname = qname;
+ LOG(("TRR::DohDecode CNAME host %s => %s\n",
+ host.get(), mCname.get()));
} else {
- cindex++;
+ LOG(("TRR::DohDecode empty CNAME for host %s!\n",
+ host.get()));
}
- 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;
+ }
+ else {
+ LOG(("TRR::DohDecode CNAME - ignoring another entry\n"));
}
-
- LOG(("TRR::DohDecode CNAME host %s => %s\n",
- host.get(), mCname.get()));
+ 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;
LOG(("done with record type %u len %u index now %u of %u\n",
TYPE, RDLENGTH, index, mBodySize));
answerRecords--;
}
@@ -827,20 +864,31 @@ TRR::FailData()
mRec = nullptr;
return NS_OK;
}
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 {
LOG(("TRR::On200Response CNAME %s => %s (%u)\n", mHost.get(), mCname.get(),
mCnameLoop));
RefPtr<TRR> trr = new TRR(mHostResolver, mRec, mCname,
mType, mCnameLoop, mPB);
rv = NS_DispatchToMainThread(trr);
--- a/netwerk/dns/TRR.h
+++ b/netwerk/dns/TRR.h
@@ -138,17 +138,18 @@ public:
RefPtr<nsHostRecord> mRec;
RefPtr<AHostResolver> mHostResolver;
private:
~TRR() = default;
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,
nsCString &host, enum TrrType &type);
nsresult ReceivePush(nsIHttpChannel *pushed, nsHostRecord *pushedRec);
nsresult On200Response();
nsCOMPtr<nsIChannel> mChannel;
--- a/netwerk/test/unit/test_trr.js
+++ b/netwerk/test/unit/test_trr.js
@@ -194,17 +194,17 @@ function test2()
// verify working credentials in DOH request
function test3()
{
prefs.setIntPref("network.trr.mode", 3); // TRR-only
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
function test4()
{
prefs.setIntPref("network.trr.mode", 3); // TRR-only
prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/dns-auth");
prefs.setCharPref("network.trr.credentials", "evil:person");
@@ -279,17 +279,18 @@ function test10()
prefs.setCharPref("network.trr.confirmationNS", "confirm.example.com");
test_loops = 100; // set for test10b
try {
listen = dns.asyncResolve("wrong.example.com", 0, listenerFails,
mainThread, defaultOriginAttributes);
} catch (e) {
// NS_ERROR_UNKNOWN_HOST exception is expected
do_test_finished();
- run_dns_tests();
+ do_timeout(200, run_dns_tests);
+ //run_dns_tests();
}
}
// confirmationNS, retry until the confirmed NS works
function test10b()
{
print("test confirmationNS, retry until the confirmed NS works");
prefs.setIntPref("network.trr.mode", 3); // TRR-only
@@ -398,22 +399,40 @@ function test20()
prefs.setCharPref("network.trr.uri", "https://foo.example.com:" + h2Port + "/dns-cname-loop");
test_answer = "127.0.0.1";
listen = dns.asyncResolve("test20.example.com", 0, listenerFine, mainThread, defaultOriginAttributes);
}
// 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,
test3,
test4,
test5,
test5b,
test6,
@@ -428,16 +447,18 @@ var tests = [ test1,
test14,
test15,
test16,
test17,
test18,
test19,
test20,
test21,
+ test22,
+ test23,
testsDone
];
var current_test = 0;
function run_dns_tests()
{
if (current_test < tests.length) {
--- a/testing/xpcshell/moz-http2/moz-http2.js
+++ b/testing/xpcshell/moz-http2/moz-http2.js
@@ -545,16 +545,57 @@ function handleRequest(req, res) {
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-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
var content;
// ... this always sends a CNAME back to pointing-elsewhere.example.com. Loop time!
content = new Buffer("00000100000100010000000005636E616D65076578616D706C6503636F6D0000050001C00C0005000100000037002012706F696E74696E672D656C73657768657265076578616D706C65C01A00", "hex");
res.setHeader('Content-Type', 'application/dns-udpwireformat');
res.setHeader('Content-Length', content.length);
res.writeHead(200);
@@ -606,19 +647,27 @@ function handleRequest(req, res) {
return;
}
// for use with test_trr.js
else if (u.pathname === "/dns-confirm") {
if (0 == ns_confirm) {
// 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);
res.writeHead(200);
res.write(content);
res.end("");
return;
}