From b599f69b2090a1269cdce49a53297201772c454c Mon Sep 17 00:00:00 2001 From: Doug Freed Date: Sun, 3 Mar 2024 09:24:34 +0000 Subject: [PATCH 1/4] tcpiohandler: Use server preference algoritm for ALPN selection This complies with RFC 7301 section 3.2 --- pdns/tcpiohandler.cc | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/pdns/tcpiohandler.cc b/pdns/tcpiohandler.cc index 841d9e3217e3..77eeedeaa817 100644 --- a/pdns/tcpiohandler.cc +++ b/pdns/tcpiohandler.cc @@ -878,23 +878,24 @@ class OpenSSLTLSIOCtx: public TLSCtx } OpenSSLTLSIOCtx* obj = reinterpret_cast(arg); - size_t pos = 0; - while (pos < inlen) { - size_t protoLen = in[pos]; - pos++; - if (protoLen > (inlen - pos)) { - /* something is very wrong */ - return SSL_TLSEXT_ERR_ALERT_WARNING; - } + // Server preference algorithm as per RFC 7301 section 3.2 + for (const auto& tentative : obj->d_alpnProtos) { + size_t pos = 0; + while (pos < inlen) { + size_t protoLen = in[pos]; + pos++; + if (protoLen > (inlen - pos)) { + /* something is very wrong */ + return SSL_TLSEXT_ERR_ALERT_WARNING; + } - for (const auto& tentative : obj->d_alpnProtos) { if (tentative.size() == protoLen && memcmp(in + pos, tentative.data(), tentative.size()) == 0) { *out = in + pos; *outlen = protoLen; return SSL_TLSEXT_ERR_OK; } + pos += protoLen; } - pos += protoLen; } return SSL_TLSEXT_ERR_NOACK; From 2a3c2b444812369cae285e6b67e2a72a5b8fed08 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Mon, 4 Mar 2024 10:13:36 +0100 Subject: [PATCH 2/4] dnsdist: Use a view for parsing ALPN data, add a regression test --- pdns/dnsdistdist/views.hh | 1 + pdns/dnsname.cc | 4 +- pdns/dnsname.hh | 23 +----------- pdns/recursordist/views.hh | 1 + pdns/tcpiohandler.cc | 10 +++-- pdns/views.hh | 55 ++++++++++++++++++++++++++++ regression-tests.dnsdist/test_DOH.py | 12 ++++++ 7 files changed, 79 insertions(+), 27 deletions(-) create mode 120000 pdns/dnsdistdist/views.hh create mode 120000 pdns/recursordist/views.hh create mode 100644 pdns/views.hh diff --git a/pdns/dnsdistdist/views.hh b/pdns/dnsdistdist/views.hh new file mode 120000 index 000000000000..2213b7d90032 --- /dev/null +++ b/pdns/dnsdistdist/views.hh @@ -0,0 +1 @@ +../views.hh \ No newline at end of file diff --git a/pdns/dnsname.cc b/pdns/dnsname.cc index c5728cc2cea2..bbac4ffcb6a4 100644 --- a/pdns/dnsname.cc +++ b/pdns/dnsname.cc @@ -124,7 +124,7 @@ static void checkLabelLength(uint8_t length) } // this parses a DNS name until a compression pointer is found -size_t DNSName::parsePacketUncompressed(const UnsignedCharView& view, size_t pos, bool uncompress) +size_t DNSName::parsePacketUncompressed(const pdns::views::UnsignedCharView& view, size_t pos, bool uncompress) { const size_t initialPos = pos; size_t totalLength = 0; @@ -189,7 +189,7 @@ void DNSName::packetParser(const char* qpos, size_t len, size_t offset, bool unc } unsigned char labellen{0}; - UnsignedCharView view(qpos, len); + pdns::views::UnsignedCharView view(qpos, len); auto pos = parsePacketUncompressed(view, offset, uncompress); labellen = view.at(pos); diff --git a/pdns/dnsname.hh b/pdns/dnsname.hh index e7a9f4b4eafd..12a2acb2cc36 100644 --- a/pdns/dnsname.hh +++ b/pdns/dnsname.hh @@ -57,6 +57,7 @@ inline unsigned char dns_tolower(unsigned char c) } #include "burtle.hh" +#include "views.hh" // #include "dns.hh" // #include "logger.hh" @@ -216,28 +217,8 @@ public: private: string_t d_storage; - class UnsignedCharView - { - public: - UnsignedCharView(const char* data_, size_t size_): view(data_, size_) - { - } - const unsigned char& at(std::string_view::size_type pos) const - { - return reinterpret_cast(view.at(pos)); - } - - size_t size() const - { - return view.size(); - } - - private: - std::string_view view; - }; - void packetParser(const char* qpos, size_t len, size_t offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset); - size_t parsePacketUncompressed(const UnsignedCharView& view, size_t position, bool uncompress); +size_t parsePacketUncompressed(const pdns::views::UnsignedCharView& view, size_t position, bool uncompress); static void appendEscapedLabel(std::string& appendTo, const char* orig, size_t len); static std::string unescapeLabel(const std::string& orig); static void throwSafeRangeError(const std::string& msg, const char* buf, size_t length); diff --git a/pdns/recursordist/views.hh b/pdns/recursordist/views.hh new file mode 120000 index 000000000000..2213b7d90032 --- /dev/null +++ b/pdns/recursordist/views.hh @@ -0,0 +1 @@ +../views.hh \ No newline at end of file diff --git a/pdns/tcpiohandler.cc b/pdns/tcpiohandler.cc index 77eeedeaa817..b048c9d1b124 100644 --- a/pdns/tcpiohandler.cc +++ b/pdns/tcpiohandler.cc @@ -876,21 +876,23 @@ class OpenSSLTLSIOCtx: public TLSCtx if (!arg) { return SSL_TLSEXT_ERR_ALERT_WARNING; } + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast): OpenSSL's API OpenSSLTLSIOCtx* obj = reinterpret_cast(arg); + const pdns::views::UnsignedCharView inView(in, inlen); // Server preference algorithm as per RFC 7301 section 3.2 for (const auto& tentative : obj->d_alpnProtos) { size_t pos = 0; - while (pos < inlen) { - size_t protoLen = in[pos]; + while (pos < inView.size()) { + size_t protoLen = inView.at(pos); pos++; if (protoLen > (inlen - pos)) { /* something is very wrong */ return SSL_TLSEXT_ERR_ALERT_WARNING; } - if (tentative.size() == protoLen && memcmp(in + pos, tentative.data(), tentative.size()) == 0) { - *out = in + pos; + if (tentative.size() == protoLen && memcmp(&inView.at(pos), tentative.data(), tentative.size()) == 0) { + *out = &inView.at(pos); *outlen = protoLen; return SSL_TLSEXT_ERR_OK; } diff --git a/pdns/views.hh b/pdns/views.hh new file mode 100644 index 000000000000..c3c8c898b3fe --- /dev/null +++ b/pdns/views.hh @@ -0,0 +1,55 @@ +/* + * This file is part of PowerDNS or dnsdist. + * Copyright -- PowerDNS.COM B.V. and its contributors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * In addition, for the avoidance of any doubt, permission is granted to + * link this program with OpenSSL and to (re)distribute the binaries + * produced as the result of such linking. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ +#pragma once + +#include + +namespace pdns::views +{ + +class UnsignedCharView +{ +public: + UnsignedCharView(const char* data_, size_t size_) : + view(data_, size_) + { + } + // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast): No unsigned char view in C++17 + UnsignedCharView(const unsigned char* data_, size_t size_) : + view(reinterpret_cast(data_), size_) + { + } + const unsigned char& at(std::string_view::size_type pos) const + { + return reinterpret_cast(view.at(pos)); + } + + size_t size() const + { + return view.size(); + } + +private: + std::string_view view; +}; + +} diff --git a/regression-tests.dnsdist/test_DOH.py b/regression-tests.dnsdist/test_DOH.py index 7fc186d982d4..69d76da1fd06 100644 --- a/regression-tests.dnsdist/test_DOH.py +++ b/regression-tests.dnsdist/test_DOH.py @@ -402,6 +402,18 @@ def testDOHHTTP1(self): self.assertEqual(rcode, 400) self.assertEqual(data, b'This server implements RFC 8484 - DNS Queries over HTTP, and requires HTTP/2 in accordance with section 5.2 of the RFC.\r\n') + def testDOHHTTP1NotSelectedOverH2(self): + """ + DOH: Check that HTTP/1.1 is not selected over H2 when offered in the wrong order by the client + """ + if self._dohLibrary == 'h2o': + raise unittest.SkipTest('h2o supports HTTP/1.1, this test is only relevant for nghttp2') + alpn = ['http/1.1', 'h2'] + conn = self.openTLSConnection(self._dohServerPort, self._serverName, self._caCert, alpn=alpn) + if not hasattr(conn, 'selected_alpn_protocol'): + raise unittest.SkipTest('Unable to check the selected ALPN, Python version is too old to support selected_alpn_protocol') + self.assertEqual(conn.selected_alpn_protocol(), 'h2') + def testDOHInvalid(self): """ DOH: Invalid DNS query From d4cd065a24fea7fd0270a2eb102f40a06309703a Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Mon, 4 Mar 2024 10:24:53 +0100 Subject: [PATCH 3/4] Add missing views.hh reference in the Makefiles --- pdns/Makefile.am | 1 + pdns/dnsdistdist/Makefile.am | 1 + pdns/recursordist/Makefile.am | 1 + 3 files changed, 3 insertions(+) diff --git a/pdns/Makefile.am b/pdns/Makefile.am index 263bf3cd0186..bee1d58ef06d 100644 --- a/pdns/Makefile.am +++ b/pdns/Makefile.am @@ -287,6 +287,7 @@ pdns_server_SOURCES = \ utility.hh \ uuid-utils.hh uuid-utils.cc \ version.cc version.hh \ + views.hh \ webserver.cc webserver.hh \ ws-api.cc ws-api.hh \ ws-auth.cc ws-auth.hh \ diff --git a/pdns/dnsdistdist/Makefile.am b/pdns/dnsdistdist/Makefile.am index 68ab167dcfbd..faaeefd37966 100644 --- a/pdns/dnsdistdist/Makefile.am +++ b/pdns/dnsdistdist/Makefile.am @@ -256,6 +256,7 @@ dnsdist_SOURCES = \ tcpiohandler.cc tcpiohandler.hh \ threadname.hh threadname.cc \ uuid-utils.hh uuid-utils.cc \ + views.hh \ xpf.cc xpf.hh \ xsk.cc xsk.hh diff --git a/pdns/recursordist/Makefile.am b/pdns/recursordist/Makefile.am index 2819adac31c7..9bb70569495e 100644 --- a/pdns/recursordist/Makefile.am +++ b/pdns/recursordist/Makefile.am @@ -218,6 +218,7 @@ pdns_recursor_SOURCES = \ uuid-utils.hh uuid-utils.cc \ validate.cc validate.hh validate-recursor.cc validate-recursor.hh \ version.cc version.hh \ + views.hh \ webserver.cc webserver.hh \ ws-api.cc ws-api.hh \ ws-recursor.cc ws-recursor.hh \ From 981c43ec9b61a942b0f539084fed1a24ed3ad301 Mon Sep 17 00:00:00 2001 From: Remi Gacogne Date: Mon, 4 Mar 2024 11:02:56 +0100 Subject: [PATCH 4/4] dnsname: Fix formatting issue --- pdns/dnsname.hh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pdns/dnsname.hh b/pdns/dnsname.hh index 12a2acb2cc36..0d9112b2ad21 100644 --- a/pdns/dnsname.hh +++ b/pdns/dnsname.hh @@ -218,7 +218,7 @@ private: string_t d_storage; void packetParser(const char* qpos, size_t len, size_t offset, bool uncompress, uint16_t* qtype, uint16_t* qclass, unsigned int* consumed, int depth, uint16_t minOffset); -size_t parsePacketUncompressed(const pdns::views::UnsignedCharView& view, size_t position, bool uncompress); + size_t parsePacketUncompressed(const pdns::views::UnsignedCharView& view, size_t position, bool uncompress); static void appendEscapedLabel(std::string& appendTo, const char* orig, size_t len); static std::string unescapeLabel(const std::string& orig); static void throwSafeRangeError(const std::string& msg, const char* buf, size_t length);