Don't store validation state in cookie object

diff --git a/pdns/dnspacket.cc b/pdns/dnspacket.cc
index 131c2466a49dc538ae631a145d194bba0737fbe3..27aa5623479010e1ffc6784e69b67d880d9cadd0 100644 (file)
--- 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 #"<<iter->first<<": "<<makeHexDump(iter->second)<<endl;
@@ -696,7 +698,7 @@ bool DNSPacket::hasValidEDNSCookie()
   if (!hasWellFormedEDNSCookie()) {
     return false;
   }
-  return d_eco.isValid(s_EDNSCookieKey, d_remote);
+  return d_ednscookievalid;
 }
 
 Netmask DNSPacket::getRealRemote() const

diff --git a/pdns/dnspacket.hh b/pdns/dnspacket.hh
index 9ee8895e3b6b078e05a6a155de025a36d2ced3be..6a7d178d6f56a6d7f56f2f8bdbf3aa9d4f47f26f 100644 (file)
--- a/pdns/dnspacket.hh
+++ b/pdns/dnspacket.hh
@@ -200,6 +200,7 @@ private:
   bool d_wantsnsid{false};
   bool d_haveednssubnet{false};
   bool d_haveednscookie{false};
+  bool d_ednscookievalid{false};
   bool d_haveednssection{false};
   bool d_isQuery;
 };

diff --git a/pdns/ednscookies.cc b/pdns/ednscookies.cc
index cba399d4fd8bb9322beee5ed8f0fd8124fdb3195..8c232406d1fdb202eca5b88c4557d2d8db265d42 100644 (file)
--- a/pdns/ednscookies.cc
+++ b/pdns/ednscookies.cc
@@ -52,9 +52,7 @@ bool EDNSCookiesOpt::makeFromString(const char* option, unsigned int len)
 string EDNSCookiesOpt::makeOptString() const
 {
   string ret;
-  if (client.length() != 8)
-    return ret;
-  if (server.length() != 0 && (server.length() < 8 || server.length() > 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<uint32_t>(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<const unsigned char*>(&toHash[0]),
     toHash.length(),
     reinterpret_cast<const unsigned char*>(&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<uint32_t>(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<const unsigned char*>(&toHash[0]),
     toHash.length(),
     reinterpret_cast<const unsigned char*>(&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 0894d0e8515e85ffd1c364b3c75bc3b8bcc5fe3b..c55542f99d77b45d256c080177b6c35f9e437139 100644 (file)
--- 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 f7d06535fa5552e9c30eab3b38920fa5030434f6..0359b4792ed113b7e2fd2686192ce30397ecf2ce 100644 (file)
--- 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 50bc75f93c6511d069a08bb82306fd8a6b787ce0..0eec8d954e66d3579b59e197c4fb63a470c7773b 100644 (file)
--- 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)