From 4b8c434c26bb1416895170c1fc4e7bd297ae9905 Mon Sep 17 00:00:00 2001 From: Valentin Gosu Date: Tue, 9 Aug 2022 12:16:34 +0000 Subject: [PATCH] Bug 1779110 - Do not follow CNAME for NODATA records r=necko-reviewers,kershaw If the response has the RA flag set, that means the recursive resolver has probably followed the CNAME chain already, so issuing other requests would be pointless. Differential Revision: https://phabricator.services.mozilla.com/D153512 --- netwerk/dns/DNSPacket.cpp | 9 ++ netwerk/dns/DNSPacket.h | 1 + netwerk/dns/TRR.cpp | 25 +++- netwerk/dns/TRR.h | 2 + netwerk/test/unit/test_trr_cname_chain.js | 138 ++++++++++++++++++++-- 5 files changed, 165 insertions(+), 10 deletions(-) diff --git a/netwerk/dns/DNSPacket.cpp b/netwerk/dns/DNSPacket.cpp index 9c85ed134936..467a0a3fdf13 100644 --- a/netwerk/dns/DNSPacket.cpp +++ b/netwerk/dns/DNSPacket.cpp @@ -490,6 +490,15 @@ Result DNSPacket::GetRCode() const { return mResponse[3] & 0x0F; } +Result DNSPacket::RecursionAvailable() const { + if (mBodySize < 12) { + LOG(("DNSPacket::GetRCode - packet too small")); + return Err(NS_ERROR_ILLEGAL_VALUE); + } + + return mResponse[3] & 0x80; +} + nsresult DNSPacket::DecodeInternal( nsCString& aHost, enum TrrType aType, nsCString& aCname, bool aAllowRFC1918, DOHresp& aResp, TypeRecordResultType& aTypeResult, diff --git a/netwerk/dns/DNSPacket.h b/netwerk/dns/DNSPacket.h index 04729bf1cafd..46b85b806600 100644 --- a/netwerk/dns/DNSPacket.h +++ b/netwerk/dns/DNSPacket.h @@ -53,6 +53,7 @@ class DNSPacket { virtual ~DNSPacket() = default; Result GetRCode() const; + Result RecursionAvailable() const; // Called in order to feed data into the buffer. nsresult OnDataAvailable(nsIRequest* aRequest, nsIInputStream* aInputStream, diff --git a/netwerk/dns/TRR.cpp b/netwerk/dns/TRR.cpp index e48fd588dba3..a377d1bc3675 100644 --- a/netwerk/dns/TRR.cpp +++ b/netwerk/dns/TRR.cpp @@ -794,6 +794,19 @@ void TRR::HandleDecodeError(nsresult aStatusCode) { } } +bool TRR::HasUsableResponse() { + if (mType == TRRTYPE_A || mType == TRRTYPE_AAAA) { + return !mDNS.mAddresses.IsEmpty(); + } + if (mType == TRRTYPE_TXT) { + return mResult.is(); + } + if (mType == TRRTYPE_HTTPSSVC) { + return mResult.is(); + } + return false; +} + nsresult TRR::FollowCname(nsIChannel* aChannel) { nsresult rv = NS_OK; nsAutoCString cname; @@ -819,11 +832,21 @@ nsresult TRR::FollowCname(nsIChannel* aChannel) { // restore mCname as DohDecode() change it mCname = cname; - if (NS_SUCCEEDED(rv) && !mDNS.mAddresses.IsEmpty()) { + if (NS_SUCCEEDED(rv) && HasUsableResponse()) { ReturnData(aChannel); return NS_OK; } + bool ra = mPacket && mPacket->RecursionAvailable().unwrapOr(false); + LOG(("ra = %d", ra)); + if (rv == NS_ERROR_UNKNOWN_HOST && ra) { + // If recursion is available, but no addresses have been returned, + // we can just return a failure here. + LOG(("TRR::FollowCname not sending another request as RA flag is set.")); + FailData(NS_ERROR_UNKNOWN_HOST); + return NS_OK; + } + if (!mCnameLoop) { LOG(("TRR::On200Response CNAME loop, eject!\n")); return NS_ERROR_REDIRECT_LOOP; diff --git a/netwerk/dns/TRR.h b/netwerk/dns/TRR.h index d24e1d0c83bd..ede41f7f8ff4 100644 --- a/netwerk/dns/TRR.h +++ b/netwerk/dns/TRR.h @@ -105,6 +105,8 @@ class TRR : public Runnable, nsresult On200Response(nsIChannel* aChannel); nsresult FollowCname(nsIChannel* aChannel); + bool HasUsableResponse(); + bool UseDefaultServer(); void SaveAdditionalRecords( const nsClassHashtable& aRecords); diff --git a/netwerk/test/unit/test_trr_cname_chain.js b/netwerk/test/unit/test_trr_cname_chain.js index d5a740220023..010dc9c7dc8f 100644 --- a/netwerk/test/unit/test_trr_cname_chain.js +++ b/netwerk/test/unit/test_trr_cname_chain.js @@ -7,11 +7,7 @@ const dns = Cc["@mozilla.org/network/dns-service;1"].getService( Ci.nsIDNSService ); - -trr_test_setup(); -registerCleanupFunction(async () => { - trr_clear_prefs(); -}); +let trrServer; function makeChan(url) { let chan = NetUtil.newChannel({ @@ -30,8 +26,13 @@ function channelOpenPromise(chan) { }); } -add_task(async function test_follow_cnames_same_response() { - let trrServer = new TRRServer(); +add_setup(async function setup() { + trr_test_setup(); + registerCleanupFunction(async () => { + trr_clear_prefs(); + }); + + trrServer = new TRRServer(); registerCleanupFunction(async () => { await trrServer.stop(); }); @@ -47,7 +48,9 @@ add_task(async function test_follow_cnames_same_response() { "network.trr.uri", `https://foo.example.com:${trrServer.port}/dns-query` ); +}); +add_task(async function test_follow_cnames_same_response() { await trrServer.registerDoHAnswers("something.foo", "A", { answers: [ { @@ -109,6 +112,123 @@ add_task(async function test_follow_cnames_same_response() { ], }); await new TRRDNSListener("a.foo", { expectedAnswer: "2.3.4.5" }); - - await trrServer.stop(); +}); + +add_task(async function test_cname_nodata() { + // Test that we don't needlessly follow cname chains when the RA flag is set + // on the response. + + await trrServer.registerDoHAnswers("first.foo", "A", { + flags: 0x80, + answers: [ + { + name: "first.foo", + ttl: 55, + type: "CNAME", + flush: false, + data: "second.foo", + }, + { + name: "second.foo", + ttl: 55, + type: "A", + flush: false, + data: "1.2.3.4", + }, + ], + }); + await trrServer.registerDoHAnswers("first.foo", "AAAA", { + flags: 0x80, + answers: [ + { + name: "first.foo", + ttl: 55, + type: "CNAME", + flush: false, + data: "second.foo", + }, + ], + }); + + await new TRRDNSListener("first.foo", { expectedAnswer: "1.2.3.4" }); + equal(await trrServer.requestCount("first.foo", "A"), 1); + equal(await trrServer.requestCount("first.foo", "AAAA"), 1); + equal(await trrServer.requestCount("second.foo", "A"), 0); + equal(await trrServer.requestCount("second.foo", "AAAA"), 0); + + await trrServer.registerDoHAnswers("first.bar", "A", { + answers: [ + { + name: "first.bar", + ttl: 55, + type: "CNAME", + flush: false, + data: "second.bar", + }, + { + name: "second.bar", + ttl: 55, + type: "A", + flush: false, + data: "1.2.3.4", + }, + ], + }); + await trrServer.registerDoHAnswers("first.bar", "AAAA", { + answers: [ + { + name: "first.bar", + ttl: 55, + type: "CNAME", + flush: false, + data: "second.bar", + }, + ], + }); + + await new TRRDNSListener("first.bar", { expectedAnswer: "1.2.3.4" }); + equal(await trrServer.requestCount("first.bar", "A"), 1); + equal(await trrServer.requestCount("first.bar", "AAAA"), 1); + equal(await trrServer.requestCount("second.bar", "A"), 0); // addr included in first response + equal(await trrServer.requestCount("second.bar", "AAAA"), 1); // will follow cname because no flag is set + + // Check that it also works for HTTPS records + + await trrServer.registerDoHAnswers("first.bar", "HTTPS", { + flags: 0x80, + answers: [ + { + name: "second.bar", + ttl: 55, + type: "HTTPS", + flush: false, + data: { + priority: 1, + name: "h3pool", + values: [ + { key: "alpn", value: ["h2", "h3"] }, + { key: "no-default-alpn" }, + { key: "port", value: 8888 }, + { key: "ipv4hint", value: "1.2.3.4" }, + { key: "echconfig", value: "123..." }, + { key: "ipv6hint", value: "::1" }, + ], + }, + }, + { + name: "first.bar", + ttl: 55, + type: "CNAME", + flush: false, + data: "second.bar", + }, + ], + }); + + let { inStatus } = await new TRRDNSListener("first.bar", { + type: dns.RESOLVE_TYPE_HTTPSSVC, + }); + Assert.ok(Components.isSuccessCode(inStatus), `${inStatus} should work`); + equal(await trrServer.requestCount("first.bar", "HTTPS"), 1); + equal(await trrServer.requestCount("second.bar", "HTTPS"), 0); });