From fe5d7fece5fb9ecaf1692ab22760b89fa229f127 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Tue, 20 Aug 2024 12:26:33 +0200 Subject: [PATCH] dnsdist: Fix EDNS flags confusion when editing the OPT header We used to wrongly reverse the byte-ordering of the existing EDNS flags when editing the OPT header, for example when setting an extended DNS error status. (cherry picked from commit 010521a0197091642bbc654b2b371b462fa73033) --- pdns/dnsdist-ecs.cc | 2 +- pdns/test-dnsdist_cc.cc | 2 +- regression-tests.dnsdist/dnsdisttests.py | 4 ++ regression-tests.dnsdist/test_DOH.py | 4 +- regression-tests.dnsdist/test_EDE.py | 41 +++++++++++++++++++ .../test_EDNSSelfGenerated.py | 8 ++++ 6 files changed, 58 insertions(+), 3 deletions(-) diff --git a/pdns/dnsdist-ecs.cc b/pdns/dnsdist-ecs.cc index 2cad1945bca8..9ec1e776b69f 100644 --- a/pdns/dnsdist-ecs.cc +++ b/pdns/dnsdist-ecs.cc @@ -252,7 +252,7 @@ bool slowRewriteEDNSOptionInQueryWithRecords(const PacketBuffer& initialPacket, /* addOrReplaceEDNSOption will set it to false if there is already an existing option */ optionAdded = true; addOrReplaceEDNSOption(options, optionToReplace, optionAdded, overrideExisting, newOptionContent); - pw.addOpt(ah.d_class, edns0.extRCode, edns0.extFlags, options, edns0.version); + pw.addOpt(ah.d_class, edns0.extRCode, ntohs(edns0.extFlags), options, edns0.version); } } diff --git a/pdns/test-dnsdist_cc.cc b/pdns/test-dnsdist_cc.cc index 44f37a6d078c..d0b693f435d6 100644 --- a/pdns/test-dnsdist_cc.cc +++ b/pdns/test-dnsdist_cc.cc @@ -2261,7 +2261,7 @@ BOOST_AUTO_TEST_CASE(test_setEDNSOption) BOOST_REQUIRE(getEDNS0Record(dq.getData(), edns0)); BOOST_CHECK_EQUAL(edns0.version, 0U); BOOST_CHECK_EQUAL(edns0.extRCode, 0U); - BOOST_CHECK_EQUAL(edns0.extFlags, EDNS_HEADER_FLAG_DO); + BOOST_CHECK_EQUAL(ntohs(edns0.extFlags), EDNS_HEADER_FLAG_DO); BOOST_REQUIRE(parseEDNSOptions(dq)); BOOST_REQUIRE(dq.ednsOptions != nullptr); diff --git a/regression-tests.dnsdist/dnsdisttests.py b/regression-tests.dnsdist/dnsdisttests.py index 3f53bd451a53..b0346d17a5d6 100644 --- a/regression-tests.dnsdist/dnsdisttests.py +++ b/regression-tests.dnsdist/dnsdisttests.py @@ -862,11 +862,13 @@ def checkMessageNoEDNS(self, expected, received): def checkMessageEDNSWithoutOptions(self, expected, received): self.assertEqual(expected, received) self.assertEqual(received.edns, 0) + self.assertEqual(expected.ednsflags, received.ednsflags) self.assertEqual(expected.payload, received.payload) def checkMessageEDNSWithoutECS(self, expected, received, withCookies=0): self.assertEqual(expected, received) self.assertEqual(received.edns, 0) + self.assertEqual(expected.ednsflags, received.ednsflags) self.assertEqual(expected.payload, received.payload) self.assertEqual(len(received.options), withCookies) if withCookies: @@ -879,6 +881,7 @@ def checkMessageEDNSWithoutECS(self, expected, received, withCookies=0): def checkMessageEDNSWithECS(self, expected, received, additionalOptions=0): self.assertEqual(expected, received) self.assertEqual(received.edns, 0) + self.assertEqual(expected.ednsflags, received.ednsflags) self.assertEqual(expected.payload, received.payload) self.assertEqual(len(received.options), 1 + additionalOptions) hasECS = False @@ -894,6 +897,7 @@ def checkMessageEDNSWithECS(self, expected, received, additionalOptions=0): def checkMessageEDNS(self, expected, received): self.assertEqual(expected, received) self.assertEqual(received.edns, 0) + self.assertEqual(expected.ednsflags, received.ednsflags) self.assertEqual(expected.payload, received.payload) self.assertEqual(len(expected.options), len(received.options)) self.compareOptions(expected.options, received.options) diff --git a/regression-tests.dnsdist/test_DOH.py b/regression-tests.dnsdist/test_DOH.py index 562795402d40..c9554846ea86 100644 --- a/regression-tests.dnsdist/test_DOH.py +++ b/regression-tests.dnsdist/test_DOH.py @@ -184,6 +184,7 @@ def testDOHExistingECS(self): query.id = 0 response = dns.message.make_response(query) response.use_edns(edns=True, payload=4096, options=[rewrittenEcso]) + response.want_dnssec(True) rrset = dns.rrset.from_text(name, 3600, dns.rdataclass.IN, @@ -899,9 +900,10 @@ def testDOHExistingECS(self): rewrittenEcso = clientsubnetoption.ClientSubnetOption('127.0.0.0', 24) query = dns.message.make_query(name, 'A', 'IN', use_edns=True, payload=512, options=[ecso], want_dnssec=True) query.id = 0 - expectedQuery = dns.message.make_query(name, 'A', 'IN', use_edns=True, payload=512, options=[rewrittenEcso]) + expectedQuery = dns.message.make_query(name, 'A', 'IN', use_edns=True, payload=512, options=[rewrittenEcso], want_dnssec=True) response = dns.message.make_response(query) response.use_edns(edns=True, payload=4096, options=[rewrittenEcso]) + response.want_dnssec(True) rrset = dns.rrset.from_text(name, 3600, dns.rdataclass.IN, diff --git a/regression-tests.dnsdist/test_EDE.py b/regression-tests.dnsdist/test_EDE.py index e45ded5e1750..675f70c6bec5 100644 --- a/regression-tests.dnsdist/test_EDE.py +++ b/regression-tests.dnsdist/test_EDE.py @@ -90,6 +90,47 @@ def testExtendedErrorBackendResponse(self): (_, receivedResponse) = sender(query, response=None, useQueue=False) self.checkMessageEDNS(expectedResponse, receivedResponse) + def testExtendedErrorBackendResponse(self): + """ + EDE: Backend response (DO) + """ + name = 'backend-response-do.ede.tests.powerdns.com.' + ede = extendederrors.ExtendedErrorOption(16, b'my extended error status') + query = dns.message.make_query(name, 'A', 'IN', use_edns=True, want_dnssec=True) + + backendResponse = dns.message.make_response(query) + backendResponse.use_edns(edns=True, payload=4096, options=[]) + backendResponse.want_dnssec(True) + rrset = dns.rrset.from_text(name, + 60, + dns.rdataclass.IN, + dns.rdatatype.A, + '127.0.0.1') + + backendResponse.answer.append(rrset) + expectedResponse = dns.message.make_response(query) + expectedResponse.use_edns(edns=True, payload=4096, options=[ede]) + expectedResponse.want_dnssec(True) + rrset = dns.rrset.from_text(name, + 60, + dns.rdataclass.IN, + dns.rdatatype.A, + '127.0.0.1') + expectedResponse.answer.append(rrset) + + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (receivedQuery, receivedResponse) = sender(query, backendResponse) + receivedQuery.id = query.id + self.assertEqual(query, receivedQuery) + self.checkMessageEDNS(expectedResponse, receivedResponse) + + # testing the cache + for method in ("sendUDPQuery", "sendTCPQuery"): + sender = getattr(self, method) + (_, receivedResponse) = sender(query, response=None, useQueue=False) + self.checkMessageEDNS(expectedResponse, receivedResponse) + def testExtendedErrorBackendResponseWithExistingEDE(self): """ EDE: Backend response with existing EDE diff --git a/regression-tests.dnsdist/test_EDNSSelfGenerated.py b/regression-tests.dnsdist/test_EDNSSelfGenerated.py index f1ee0aec1ddc..6186841b5233 100644 --- a/regression-tests.dnsdist/test_EDNSSelfGenerated.py +++ b/regression-tests.dnsdist/test_EDNSSelfGenerated.py @@ -145,6 +145,7 @@ def testWithEDNSWithDO(self): query = dns.message.make_query(name, 'A', 'IN', use_edns=True, payload=4096, want_dnssec=True) query.flags &= ~dns.flags.RD expectedResponse = dns.message.make_response(query, our_payload=1042) + expectedResponse.want_dnssec(True) expectedResponse.set_rcode(dns.rcode.REFUSED) for method in ("sendUDPQuery", "sendTCPQuery"): @@ -159,6 +160,7 @@ def testWithEDNSWithDO(self): # dnsdist sets RA = RD for TC responses query.flags &= ~dns.flags.RD expectedResponse = dns.message.make_response(query, our_payload=1042) + expectedResponse.want_dnssec(True) expectedResponse.flags |= dns.flags.TC (_, receivedResponse) = self.sendUDPQuery(query, response=None, useQueue=False) @@ -169,6 +171,7 @@ def testWithEDNSWithDO(self): name = 'edns-do.lua.edns-self.tests.powerdns.com.' query = dns.message.make_query(name, 'A', 'IN', use_edns=True, payload=4096, want_dnssec=True) expectedResponse = dns.message.make_response(query, our_payload=1042) + expectedResponse.want_dnssec(True) expectedResponse.set_rcode(dns.rcode.NXDOMAIN) for method in ("sendUDPQuery", "sendTCPQuery"): @@ -183,6 +186,7 @@ def testWithEDNSWithDO(self): # dnsdist set RA = RD for spoofed responses query.flags &= ~dns.flags.RD expectedResponse = dns.message.make_response(query, our_payload=1042) + expectedResponse.want_dnssec(True) expectedResponse.answer.append(dns.rrset.from_text(name, 60, dns.rdataclass.IN, @@ -206,6 +210,7 @@ def testWithEDNSNoOptions(self): query.flags &= ~dns.flags.RD expectedResponse = dns.message.make_response(query, our_payload=1042) expectedResponse.set_rcode(dns.rcode.REFUSED) + expectedResponse.want_dnssec(True) for method in ("sendUDPQuery", "sendTCPQuery"): sender = getattr(self, method) @@ -219,6 +224,7 @@ def testWithEDNSNoOptions(self): # dnsdist sets RA = RD for TC responses query.flags &= ~dns.flags.RD expectedResponse = dns.message.make_response(query, our_payload=1042) + expectedResponse.want_dnssec(True) expectedResponse.flags |= dns.flags.TC (_, receivedResponse) = self.sendUDPQuery(query, response=None, useQueue=False) @@ -229,6 +235,7 @@ def testWithEDNSNoOptions(self): name = 'edns-options.lua.edns-self.tests.powerdns.com.' query = dns.message.make_query(name, 'A', 'IN', use_edns=True, options=[ecso], payload=512, want_dnssec=True) expectedResponse = dns.message.make_response(query, our_payload=1042) + expectedResponse.want_dnssec(True) expectedResponse.set_rcode(dns.rcode.NXDOMAIN) for method in ("sendUDPQuery", "sendTCPQuery"): @@ -243,6 +250,7 @@ def testWithEDNSNoOptions(self): # dnsdist set RA = RD for spoofed responses query.flags &= ~dns.flags.RD expectedResponse = dns.message.make_response(query, our_payload=1042) + expectedResponse.want_dnssec(True) expectedResponse.answer.append(dns.rrset.from_text(name, 60, dns.rdataclass.IN,