chg: usr: Drop malformed notify messages early instead of decompressing them

The DNS header shows if a message has multiple questions or invalid
NOTIFY sections. We can drop these messages early, right after parsing
the question. This matches RFC 9619 for multi-question messages and
Unbound's handling of NOTIFY. We still parse the question to include it in
our FORMERR response.

Add drop_msg_early() function to check for these conditions:
- Messages with more than one question, as required by RFC 9619
- NOTIFY query messages containing answer sections (like Unbound)
- NOTIFY messages containing authority sections (like Unbound)

Closes #5158, #3656

Merge branch '5158-early-formerr-on-bad-notify-or-bad-qdcount' into 'main'

See merge request isc-projects/bind9!10056

bin/tests/system/formerr/tests.sh

@@ -36,7 +36,7 @@ fi
 echo_i "two question types"
 $PERL formerr.pl -a 10.53.0.1 -p ${PORT} twoquestiontypes >twoquestiontypes.out
 ans=$(grep got: twoquestiontypes.out)
-if [ "${ans}" != "got: 0000800100020000000000000e41414141414141414141414141410000010001c00c00020001" ]; then
+if [ "${ans}" != "got: 000080010000000000000000" ]; then
   echo_i "failed"
   status=$((status + 1))
 fi

lib/dns/include/dns/types.h

@@ -317,8 +317,10 @@ enum {
 #define dns_opcode_status ((dns_opcode_t)dns_opcode_status)
 	dns_opcode_notify = 4,
 #define dns_opcode_notify ((dns_opcode_t)dns_opcode_notify)
-	dns_opcode_update = 5 /* dynamic update */
+	dns_opcode_update = 5, /* dynamic update */
 #define dns_opcode_update ((dns_opcode_t)dns_opcode_update)
+	dns_opcode_max = 6,
+#define dns_opcode_max ((dns_opcode_t)dns_opcode_max)
 };
 
 /*%

lib/dns/message.c

@@ -966,7 +966,6 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx,
 	isc_region_t r;
 	unsigned int count;
 	dns_name_t *name = NULL;
-	dns_name_t *found_name = NULL;
 	dns_rdataset_t *rdataset = NULL;
 	dns_rdatalist_t *rdatalist = NULL;
 	isc_result_t result = ISC_R_SUCCESS;
@@ -976,12 +975,8 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx,
 	bool best_effort = ((options & DNS_MESSAGEPARSE_BESTEFFORT) != 0);
 	bool seen_problem = false;
 	bool free_name = false;
-	bool free_hashmaps = false;
-	isc_hashmap_t *name_map = NULL;
 
-	if (msg->counts[DNS_SECTION_QUESTION] > 1) {
+	REQUIRE(msg->counts[DNS_SECTION_QUESTION] <= 1 || best_effort);
-		isc_hashmap_create(msg->mctx, 1, &name_map);
-	}
 
 	for (count = 0; count < msg->counts[DNS_SECTION_QUESTION]; count++) {
 		name = NULL;
@@ -999,48 +994,7 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx,
 			goto cleanup;
 		}
 
-		/* If there is only one QNAME, skip the duplicity checks */
+		ISC_LIST_APPEND(*section, name, link);
-		if (name_map == NULL) {
-			result = ISC_R_SUCCESS;
-			goto skip_name_check;
-		}
-
-		/*
-		 * Run through the section, looking to see if this name
-		 * is already there.  If it is found, put back the allocated
-		 * name since we no longer need it, and set our name pointer
-		 * to point to the name we found.
-		 */
-		result = isc_hashmap_add(name_map, dns_name_hash(name),
-					 name_match, name, name,
-					 (void **)&found_name);
-
-		/*
-		 * If it is the first name in the section, accept it.
-		 *
-		 * If it is not, but is not the same as the name already
-		 * in the question section, append to the section.  Note that
-		 * here in the question section this is illegal, so return
-		 * FORMERR.  In the future, check the opcode to see if
-		 * this should be legal or not.  In either case we no longer
-		 * need this name pointer.
-		 */
-	skip_name_check:
-		switch (result) {
-		case ISC_R_SUCCESS:
-			if (!ISC_LIST_EMPTY(*section)) {
-				DO_ERROR(DNS_R_FORMERR);
-			}
-			ISC_LIST_APPEND(*section, name, link);
-			break;
-		case ISC_R_EXISTS:
-			dns_message_puttempname(msg, &name);
-			name = found_name;
-			found_name = NULL;
-			break;
-		default:
-			UNREACHABLE();
-		}
 
 		free_name = false;
 
@@ -1090,40 +1044,6 @@ getquestions(isc_buffer_t *source, dns_message_t *msg, dns_decompress_t dctx,
 
 		rdataset->attributes |= DNS_RDATASETATTR_QUESTION;
 
-		/*
-		 * Skip the duplicity check for first rdataset
-		 */
-		if (ISC_LIST_EMPTY(name->list)) {
-			result = ISC_R_SUCCESS;
-			goto skip_rds_check;
-		}
-
-		/*
-		 * Can't ask the same question twice.
-		 */
-		if (name->hashmap == NULL) {
-			isc_hashmap_create(msg->mctx, 1, &name->hashmap);
-			free_hashmaps = true;
-
-			INSIST(ISC_LIST_HEAD(name->list) ==
-			       ISC_LIST_TAIL(name->list));
-
-			dns_rdataset_t *old_rdataset =
-				ISC_LIST_HEAD(name->list);
-
-			result = isc_hashmap_add(
-				name->hashmap, rds_hash(old_rdataset),
-				rds_match, old_rdataset, old_rdataset, NULL);
-
-			INSIST(result == ISC_R_SUCCESS);
-		}
-		result = isc_hashmap_add(name->hashmap, rds_hash(rdataset),
-					 rds_match, rdataset, rdataset, NULL);
-		if (result == ISC_R_EXISTS) {
-			DO_ERROR(DNS_R_FORMERR);
-		}
-
-	skip_rds_check:
 		ISC_LIST_APPEND(name->list, rdataset, link);
 
 		rdataset = NULL;
@@ -1146,14 +1066,6 @@ cleanup:
 		dns_message_puttempname(msg, &name);
 	}
 
-	if (free_hashmaps) {
-		cleanup_name_hashmaps(section);
-	}
-
-	if (name_map != NULL) {
-		isc_hashmap_destroy(&name_map);
-	}
-
 	return result;
 }
 
@@ -1663,6 +1575,38 @@ cleanup:
 	return result;
 }
 
+static isc_result_t
+early_sanity_check(dns_message_t *msg) {
+	bool is_unknown_opcode = msg->opcode >= dns_opcode_max;
+	bool is_query_response = (msg->flags & DNS_MESSAGEFLAG_QR) != 0;
+	bool no_questions = msg->counts[DNS_SECTION_QUESTION] == 0;
+	bool many_questions = msg->counts[DNS_SECTION_QUESTION] > 1;
+	bool has_answer = msg->counts[DNS_SECTION_ANSWER] > 0;
+	bool has_auth = msg->counts[DNS_SECTION_AUTHORITY] > 0;
+
+	if (is_unknown_opcode) {
+		return DNS_R_NOTIMP;
+	} else if (many_questions) {
+		return DNS_R_FORMERR;
+	} else if (no_questions && (msg->opcode != dns_opcode_query) &&
+		   (msg->opcode != dns_opcode_status))
+	{
+		/*
+		 * Per RFC9619, the two cases where qdcount == 0 is acceptable
+		 * are AXFR transfers and cookies, and both have opcode 0.
+		 *
+		 * RFC9619 also specifies that msg->opcode == dns_opcode_status
+		 * is unspecified, so we ignore it.
+		 */
+		return DNS_R_FORMERR;
+	} else if (msg->opcode == dns_opcode_notify &&
+		   ((is_query_response && has_answer) || has_auth))
+	{
+		return DNS_R_FORMERR;
+	}
+	return ISC_R_SUCCESS;
+}
+
 isc_result_t
 dns_message_parse(dns_message_t *msg, isc_buffer_t *source,
 		  unsigned int options) {
@@ -1717,6 +1661,12 @@ dns_message_parse(dns_message_t *msg, isc_buffer_t *source,
 
 	dctx = DNS_DECOMPRESS_ALWAYS;
 
+	bool strict_parse = ((options & DNS_MESSAGEPARSE_BESTEFFORT) == 0);
+	isc_result_t early_check_ret = early_sanity_check(msg);
+	if (strict_parse && (early_check_ret != ISC_R_SUCCESS)) {
+		return early_check_ret;
+	}
+
 	ret = getquestions(source, msg, dctx, options);
 
 	if (ret == ISC_R_UNEXPECTEDEND && ignore_tc) {