From 3969e2c5f7b51fe56bb363bf786cf0d5b7f9c264 Mon Sep 17 00:00:00 2001
From: Mark Andrews <marka@isc.org>
Date: Thu, 6 Jul 2023 16:58:53 +1000
Subject: [PATCH] Return BADCOOKIE on validly formed bad SERVER COOKIES

The server was previously tolerant of out-of-date or otherwise bad
DNS SERVER COOKIES that where well formed unless require-cookie was
set.  BADCOOKIE is now return for these conditions.
---
 lib/ns/client.c            | 22 ++++++++++------------
 lib/ns/include/ns/client.h |  3 ++-
 lib/ns/query.c             | 13 +++++++++----
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/lib/ns/client.c b/lib/ns/client.c
index 05af2cadb4..d407cb3568 100644
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -1147,15 +1147,10 @@ static void
 compute_cookie(ns_client_t *client, uint32_t when, uint32_t nonce,
 	       const unsigned char *secret, isc_buffer_t *buf) {
 	unsigned char digest[ISC_MAX_MD_SIZE] ISC_NONSTRING = { 0 };
-	STATIC_ASSERT(ISC_MAX_MD_SIZE >= ISC_SIPHASH24_TAG_LENGTH, "You need "
-								   "to "
-								   "increase "
-								   "the digest "
-								   "buffer.");
-	STATIC_ASSERT(ISC_MAX_MD_SIZE >= ISC_AES_BLOCK_LENGTH, "You need to "
-							       "increase the "
-							       "digest "
-							       "buffer.");
+	STATIC_ASSERT(ISC_MAX_MD_SIZE >= ISC_SIPHASH24_TAG_LENGTH,
+		      "You need to increase the digest buffer.");
+	STATIC_ASSERT(ISC_MAX_MD_SIZE >= ISC_AES_BLOCK_LENGTH,
+		      "You need to increase the digest buffer.");
 
 	switch (client->manager->sctx->cookiealg) {
 	case ns_cookiealg_siphash24: {
@@ -1278,6 +1273,7 @@ process_cookie(ns_client_t *client, isc_buffer_t *buf, size_t optlen) {
 		} else {
 			ns_stats_increment(client->manager->sctx->nsstats,
 					   ns_statscounter_cookiebadsize);
+			client->attributes |= NS_CLIENTATTR_BADCOOKIE;
 		}
 		return;
 	}
@@ -1297,9 +1293,10 @@ process_cookie(ns_client_t *client, isc_buffer_t *buf, size_t optlen) {
 	 * Only accept COOKIE if we have talked to the client in the last hour.
 	 */
 	now = isc_stdtime_now();
-	if (isc_serial_gt(when, (now + 300)) || /* In the future. */
-	    isc_serial_lt(when, (now - 3600)))
-	{ /* In the past. */
+	if (isc_serial_gt(when, (now + 300)) /* In the future. */ ||
+	    isc_serial_lt(when, (now - 3600)) /* In the past. */)
+	{
+		client->attributes |= NS_CLIENTATTR_BADCOOKIE;
 		ns_stats_increment(client->manager->sctx->nsstats,
 				   ns_statscounter_cookiebadtime);
 		return;
@@ -1328,6 +1325,7 @@ process_cookie(ns_client_t *client, isc_buffer_t *buf, size_t optlen) {
 		}
 	}
 
+	client->attributes |= NS_CLIENTATTR_BADCOOKIE;
 	ns_stats_increment(client->manager->sctx->nsstats,
 			   ns_statscounter_cookienomatch);
 }
diff --git a/lib/ns/include/ns/client.h b/lib/ns/include/ns/client.h
index 04dbb9c4a8..111e107d58 100644
--- a/lib/ns/include/ns/client.h
+++ b/lib/ns/include/ns/client.h
@@ -234,7 +234,8 @@ struct ns_client {
 #define NS_CLIENTATTR_MULTICAST	 0x00008 /*%< recv'd from multicast */
 #define NS_CLIENTATTR_WANTDNSSEC 0x00010 /*%< include dnssec records */
 #define NS_CLIENTATTR_WANTNSID	 0x00020 /*%< include nameserver ID */
-/* Obsolete: NS_CLIENTATTR_FILTER_AAAA	0x00040 */
+#define NS_CLIENTATTR_BADCOOKIE \
+	0x00040 /*%< Presented cookie is bad/out-of-date */
 /* Obsolete: NS_CLIENTATTR_FILTER_AAAA_RC 0x00080 */
 #define NS_CLIENTATTR_WANTAD	   0x00100 /*%< want AD in response if possible */
 #define NS_CLIENTATTR_WANTCOOKIE   0x00200 /*%< return a COOKIE */
diff --git a/lib/ns/query.c b/lib/ns/query.c
index 18c34b6968..67461f78cd 100644
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -115,6 +115,8 @@
 #define WANTDNSSEC(c) (((c)->attributes & NS_CLIENTATTR_WANTDNSSEC) != 0)
 /*% Want WANTAD? */
 #define WANTAD(c) (((c)->attributes & NS_CLIENTATTR_WANTAD) != 0)
+/*% Client presented a bad COOKIE. */
+#define BADCOOKIE(c) (((c)->attributes & NS_CLIENTATTR_BADCOOKIE) != 0)
 /*% Client presented a valid COOKIE. */
 #define HAVECOOKIE(c) (((c)->attributes & NS_CLIENTATTR_HAVECOOKIE) != 0)
 /*% Client presented a COOKIE. */
@@ -5619,11 +5621,14 @@ ns__query_start(query_ctx_t *qctx) {
 	CALL_HOOK(NS_QUERY_START_BEGIN, qctx);
 
 	/*
-	 * If we require a server cookie then send back BADCOOKIE
-	 * before we have done too much work.
+	 * If we require a server cookie or the presented server
+	 * cookie was bad then send back BADCOOKIE before we have
+	 * done too much work.
 	 */
-	if (!TCP(qctx->client) && qctx->view->requireservercookie &&
-	    WANTCOOKIE(qctx->client) && !HAVECOOKIE(qctx->client))
+	if (!TCP(qctx->client) &&
+	    (BADCOOKIE(qctx->client) ||
+	     (qctx->view->requireservercookie && WANTCOOKIE(qctx->client) &&
+	      !HAVECOOKIE(qctx->client))))
 	{
 		qctx->client->message->flags &= ~DNS_MESSAGEFLAG_AA;
 		qctx->client->message->flags &= ~DNS_MESSAGEFLAG_AD;