From: Pieter Lexis Date: Tue, 1 Jun 2021 14:30:22 +0000 (+0200) Subject: Don't store validation state in cookie object X-Git-Tag: dnsdist-1.7.0-alpha1~3^2~10 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=29992caa2f714617408045030083df37fd0caaea;p=thirdparty%2Fpdns.git Don't store validation state in cookie object --- diff --git a/pdns/dnspacket.cc b/pdns/dnspacket.cc index 131c2466a4..27aa562347 100644 --- a/pdns/dnspacket.cc +++ b/pdns/dnspacket.cc @@ -585,6 +585,7 @@ try d_haveednssubnet = false; d_haveednssection = false; d_haveednscookie = false; + d_ednscookievalid = false; if(getEDNSOpts(mdp, &edo)) { d_haveednssection=true; @@ -610,6 +611,7 @@ try else if (s_doEDNSCookieProcessing && option.first == EDNSOptionCode::COOKIE) { d_haveednscookie = true; d_eco.makeFromString(option.second); + d_ednscookievalid = d_eco.isValid(s_EDNSCookieKey, d_remote); } else { // cerr<<"Have an option #"<first<<": "<second)< 32)) + if (!isWellFormed()) return ret; ret.assign(client); if (server.length() != 0) @@ -64,9 +62,6 @@ string EDNSCookiesOpt::makeOptString() const void EDNSCookiesOpt::getEDNSCookiesOptFromString(const char* option, unsigned int len) { - checked = false; - valid = false; - should_refresh = false; client.clear(); server.clear(); if (len < 8) @@ -80,12 +75,6 @@ void EDNSCookiesOpt::getEDNSCookiesOptFromString(const char* option, unsigned in bool EDNSCookiesOpt::isValid(const string& secret, const ComboAddress& source) { #ifdef HAVE_CRYPTO_SHORTHASH - if (checked && valid) { - // Ignore the new check, we already validated it - // XXX this _might_ not be the best behaviour though... - return valid; - } - checked = true; if (server.length() != 16 || client.length() != 8) { return false; } @@ -97,15 +86,13 @@ bool EDNSCookiesOpt::isValid(const string& secret, const ComboAddress& source) memcpy(&ts, &server[4], sizeof(ts)); ts = ntohl(ts); uint32_t now = static_cast(time(nullptr)); + // RFC 9018 section 4.3: + // The DNS server + // SHOULD allow cookies within a 1-hour period in the past and a + // 5-minute period into the future if (rfc1982LessThan(now + 300, ts) && rfc1982LessThan(ts + 3600, now)) { return false; } - if (rfc1982LessThan(ts + 1800, now)) { - // RFC 9018 section 4.3: - // The DNS server SHOULD generate a new Server Cookie at least if the - // received Server Cookie from the client is more than half an hour old - should_refresh = true; - } if (secret.length() != crypto_shorthash_KEYBYTES) { // XXX should we throw std::range_error here? return false; @@ -119,22 +106,42 @@ bool EDNSCookiesOpt::isValid(const string& secret, const ComboAddress& source) reinterpret_cast(&toHash[0]), toHash.length(), reinterpret_cast(&secret[0])); - valid = (server.substr(8) == hashResult); - return valid; + return server.substr(8) == hashResult; #else return false; #endif } +bool EDNSCookiesOpt::shouldRefresh() +{ + if (server.size() < 16) { + return true; + } + uint32_t ts; + memcpy(&ts, &server[4], sizeof(ts)); + ts = ntohl(ts); + uint32_t now = static_cast(time(nullptr)); + // RFC 9018 section 4.3: + // The DNS server + // SHOULD allow cookies within a 1-hour period in the past and a + // 5-minute period into the future + // If this is not the case, we need to refresh + if (rfc1982LessThan(now + 300, ts) && rfc1982LessThan(ts + 3600, now)) { + return true; + } + + // RFC 9018 section 4.3: + // The DNS server SHOULD generate a new Server Cookie at least if the + // received Server Cookie from the client is more than half an hour old + return rfc1982LessThan(ts + 1800, now); +} + bool EDNSCookiesOpt::makeServerCookie(const string& secret, const ComboAddress& source) { #ifdef HAVE_CRYPTO_SHORTHASH - if (valid && !should_refresh) { + if (isValid(secret, source) && !shouldRefresh()) { return true; } - checked = false; - valid = false; - should_refresh = false; if (secret.length() != crypto_shorthash_KEYBYTES) { return false; @@ -156,9 +163,6 @@ bool EDNSCookiesOpt::makeServerCookie(const string& secret, const ComboAddress& reinterpret_cast(&toHash[0]), toHash.length(), reinterpret_cast(&secret[0])); - checked = true; - valid = true; - should_refresh = false; return true; #else return false; diff --git a/pdns/ednscookies.hh b/pdns/ednscookies.hh index 0894d0e851..c55542f99d 100644 --- a/pdns/ednscookies.hh +++ b/pdns/ednscookies.hh @@ -61,12 +61,7 @@ struct EDNSCookiesOpt } private: - // Whether or not we checked this cookie - bool checked{false}; - // Whether or not the cookie is valid - bool valid{false}; - // Whether or not he cookie will expire within 30 minutes - bool should_refresh{false}; + bool shouldRefresh(); // the client cookie string client; diff --git a/pdns/test-ednscookie_cc.cc b/pdns/test-ednscookie_cc.cc index f7d06535fa..0359b4792e 100644 --- a/pdns/test-ednscookie_cc.cc +++ b/pdns/test-ednscookie_cc.cc @@ -102,9 +102,6 @@ BOOST_AUTO_TEST_CASE(test_createEDNSServerCookie) BOOST_CHECK(!eco2.isValid(secret, ComboAddress("192.0.2.1"))); BOOST_CHECK(!eco2.isValid("blablablablabla1", remote)); BOOST_CHECK(eco2.isValid(secret, remote)); - - // Check is we marked it as valid before - BOOST_CHECK(eco2.isValid("blablablablabla1", remote)); } #endif diff --git a/regression-tests.auth-py/test_Cookies.py b/regression-tests.auth-py/test_Cookies.py index 50bc75f93c..0eec8d954e 100644 --- a/regression-tests.auth-py/test_Cookies.py +++ b/regression-tests.auth-py/test_Cookies.py @@ -86,6 +86,7 @@ www.example.org. 3600 IN A 192.0.2.5 def testBrokenCookie(self): data = self.getCookieFromServer().data + # replace a byte in the client cookie data = data.replace(b'\x11', b'\x12') opts = [dns.edns.GenericOption(dns.edns.COOKIE, data)] query = dns.message.make_query('www.example.org', 'A', options=opts)