From: Remi Gacogne Date: Wed, 11 Mar 2020 17:26:37 +0000 (+0100) Subject: rec: Fix the padding size computation, remove outdated comment X-Git-Tag: rec-4.5.0-beta1~3^2~13 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=8ba588d00589e593d89a789daafe800b998a9abb;p=thirdparty%2Fpdns.git rec: Fix the padding size computation, remove outdated comment --- diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index f219de1672..6e516e76ad 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -1489,8 +1489,8 @@ static void startDoResolve(void *p) std::vector > ednsOpts; bool variableAnswer = dc->d_variable; bool haveEDNS=false; - bool paddingAllowed = g_paddingFrom.match(dc->d_remote); - bool padResponse = paddingAllowed ? (g_paddingMode == PaddingMode::Always) : false; + bool paddingAllowed = false; + bool padResponse = false; #ifdef NOD_ENABLED bool hasUDR = false; #endif /* NOD_ENABLED */ @@ -1511,6 +1511,13 @@ static void startDoResolve(void *p) ednsOpts = edo.d_options; maxanswersize -= 11; // EDNS header size + if (g_paddingFrom.match(dc->d_remote)) { + paddingAllowed = true; + if (g_paddingMode == PaddingMode::Always) { + padResponse = true; + } + } + for (const auto& o : edo.d_options) { if (o.first == EDNSOptionCode::ECS && g_useIncomingECS && !dc->d_ecsParsed) { dc->d_ecsFound = getEDNSSubnetOptsFromString(o.second, &dc->d_ednssubnet); @@ -1523,15 +1530,7 @@ static void startDoResolve(void *p) maxanswersize -= EDNSOptionCodeSize + EDNSOptionLengthSize + mode_server_id.size(); } } else if (paddingAllowed && padResponse == false && g_paddingMode == PaddingMode::PaddedQueries && o.first == EDNSOptionCode::PADDING) { - /* we should only pad on 'secure' cases to limit amplification: - - over TCP ; - - from 'known-good' sources (dnsdist for example). - We should also support a strict mode (only pad if the query has a padding option) - and a relaxed mode (pad if the client supports EDNS). - The block-size should be configurable as well. - */ padResponse = true; - maxanswersize -= 4; } } } @@ -1990,10 +1989,14 @@ static void startDoResolve(void *p) } } - if (padResponse) { + if (haveEDNS && padResponse) { size_t currentSize = pw.getSizeWithOpts(returnedEdnsOptions); - if (currentSize < (static_cast(maxanswersize) - 4)) { - size_t remaining = static_cast(maxanswersize) - currentSize; + /* we don't use maxawnswersize because it accounts for some EDNS options, but + not all of them (for example ECS) */ + size_t maxSize = min(static_cast(edo.d_packetsize >= 512 ? edo.d_packetsize : 512), g_udpTruncationThreshold); + + if (currentSize < (maxSize - 4)) { + size_t remaining = maxSize - (currentSize + 4); /* from rfc8647, "4.1. Recommended Strategy: Block-Length Padding": If a server receives a query that includes the EDNS(0) "Padding" option, it MUST pad the corresponding response (see Section 4 of diff --git a/regression-tests.recursor-dnssec/test_EDNSPadding.py b/regression-tests.recursor-dnssec/test_EDNSPadding.py index f2a40c4de7..d00f2cb49e 100644 --- a/regression-tests.recursor-dnssec/test_EDNSPadding.py +++ b/regression-tests.recursor-dnssec/test_EDNSPadding.py @@ -6,7 +6,7 @@ import paddingoption from recursortests import RecursorTest -class TestRecursorEDNSPadding(RecursorTest): +class RecursorEDNSPaddingTest(RecursorTest): @classmethod def setUpClass(cls): @@ -42,6 +42,9 @@ class TestRecursorEDNSPadding(RecursorTest): self.assertEqual(message.edns, 0) self.assertEquals(len(message.options), 0) + def checkNoEDNS(self, message): + self.assertEqual(message.edns, -1) + def sendUDPQueryOverIPv6(self, query, timeout=2.0): sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) sock.settimeout(2.0) @@ -64,7 +67,16 @@ class TestRecursorEDNSPadding(RecursorTest): message = dns.message.from_wire(data) return message -class PaddingDefaultTest(TestRecursorEDNSPadding): + def testQueryWithoutEDNS(self): + name = 'secure.example.' + expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17') + query = dns.message.make_query(name, 'A', want_dnssec=False) + query.flags |= dns.flags.CD + res = self.sendUDPQuery(query) + self.checkNoEDNS(res) + self.assertRRsetInAnswer(res, expected) + +class PaddingDefaultTest(RecursorEDNSPaddingTest): _confdir = 'PaddingDefault' @@ -78,7 +90,7 @@ class PaddingDefaultTest(TestRecursorEDNSPadding): self.checkNoPadding(res) self.assertRRsetInAnswer(res, expected) - def testqueryWithoutPadding(self): + def testQueryWithoutPadding(self): name = 'secure.example.' expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17') query = dns.message.make_query(name, 'A', want_dnssec=True) @@ -87,7 +99,7 @@ class PaddingDefaultTest(TestRecursorEDNSPadding): self.checkNoPadding(res) self.assertRRsetInAnswer(res, expected) -class PaddingAllowedAlwaysTest(TestRecursorEDNSPadding): +class PaddingAllowedAlwaysTest(RecursorEDNSPaddingTest): _confdir = 'PaddingAlways' _config_template = """edns-padding-from=127.0.0.1 @@ -105,7 +117,7 @@ edns-padding-tag=7830 self.checkPadding(res) self.assertRRsetInAnswer(res, expected) - def testqueryWithoutPadding(self): + def testQueryWithoutPadding(self): name = 'secure.example.' expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17') query = dns.message.make_query(name, 'A', want_dnssec=True) @@ -114,7 +126,7 @@ edns-padding-tag=7830 self.checkPadding(res) self.assertRRsetInAnswer(res, expected) -class PaddingAllowedWhenPaddedTest(TestRecursorEDNSPadding): +class PaddingAllowedWhenPaddedTest(RecursorEDNSPaddingTest): _confdir = 'PaddingWhenPadded' _config_template = """edns-padding-from=127.0.0.1 @@ -132,7 +144,7 @@ edns-padding-tag=7830 self.checkPadding(res) self.assertRRsetInAnswer(res, expected) - def testqueryWithoutPadding(self): + def testQueryWithoutPadding(self): name = 'secure.example.' expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17') query = dns.message.make_query(name, 'A', want_dnssec=True) @@ -141,7 +153,7 @@ edns-padding-tag=7830 self.checkNoPadding(res) self.assertRRsetInAnswer(res, expected) -class PaddingAllowedAlwaysSameTagTest(TestRecursorEDNSPadding): +class PaddingAllowedAlwaysSameTagTest(RecursorEDNSPaddingTest): # we use the default tag (0) for padded responses, which will cause # the same packet cache entry (with padding ) to be returned to a client @@ -167,7 +179,7 @@ local-address=127.0.0.1, ::1 self.checkPadding(res) self.assertRRsetInAnswer(res, expected) - def testqueryWithoutPadding(self): + def testQueryWithoutPadding(self): name = 'secure.example.' expected = dns.rrset.from_text(name, 0, dns.rdataclass.IN, 'A', '192.0.2.17') query = dns.message.make_query(name, 'A', want_dnssec=True)