3548. [bug] The NSID request code in resolver.c was broken

                        resulting in invalid EDNS options being sent.
                        [RT #33153]

CHANGES

+3548.	[bug]		The NSID request code in resolver.c was broken
+			resulting in invalid EDNS options being sent.
+			[RT #33153]
+
 3547.	[bug]		Some malformed unknown rdata records were not properly
 			detected and rejected. [RT #33129]
 

lib/dns/include/dns/message.h

@@ -251,6 +251,12 @@ struct dns_message {
 	const void *			order_arg;
 };
 
+struct dns_ednsopt {
+	isc_uint16_t			code;
+	isc_uint16_t			length;
+	unsigned char			*value;
+};
+
 /***
  *** Functions
  ***/
@@ -1359,6 +1365,24 @@ dns_message_logpacket(dns_message_t *message, const char *description,
  * normally end with a newline.
  */
 
+isc_result_t
+dns_message_buildopt(dns_message_t *msg, dns_rdataset_t **opt,
+		     unsigned int version, isc_uint16_t udpsize,
+		     unsigned int flags, dns_ednsopt_t *ednsopts, size_t count);
+/*%<
+ * Built a opt record.
+ *
+ * Requires: 
+ * \li   msg be a valid message.
+ * \li   opt to be a non NULL and *opt to be NULL.
+ *
+ * Returns:
+ * \li	 ISC_R_SUCCESS on success.
+ * \li	 ISC_R_NOMEMORY
+ * \li	 ISC_R_NOSPACE
+ * \li	 other.
+ */
+
 
 ISC_LANG_ENDDECLS
 

lib/dns/include/dns/types.h

@@ -77,6 +77,7 @@ typedef struct dns_dnsseckey			dns_dnsseckey_t;
 typedef ISC_LIST(dns_dnsseckey_t)		dns_dnsseckeylist_t;
 typedef isc_uint8_t				dns_dsdigest_t;
 typedef struct dns_dumpctx			dns_dumpctx_t;
+typedef struct dns_ednsopt			dns_ednsopt_t;
 typedef struct dns_fetch			dns_fetch_t;
 typedef struct dns_fixedname			dns_fixedname_t;
 typedef struct dns_forwarders			dns_forwarders_t;

lib/dns/message.c

@@ -2642,9 +2642,9 @@ dns_message_setopt(dns_message_t *msg, dns_rdataset_t *opt) {
 	return (ISC_R_SUCCESS);
 
  cleanup:
+	dns_rdataset_disassociate(opt);
 	dns_message_puttemprdataset(msg, &opt);
 	return (result);
-
 }
 
 dns_rdataset_t *
@@ -3497,3 +3497,95 @@ dns_message_logpacket(dns_message_t *message, const char *description,
 	if (buf != NULL)
 		isc_mem_put(mctx, buf, len);
 }
+
+isc_result_t
+dns_message_buildopt(dns_message_t *message, dns_rdataset_t **rdatasetp,
+		     unsigned int version, isc_uint16_t udpsize,
+		     unsigned int flags, dns_ednsopt_t *ednsopts, size_t count)
+{
+	dns_rdataset_t *rdataset = NULL;
+	dns_rdatalist_t *rdatalist = NULL;
+	dns_rdata_t *rdata = NULL;
+	isc_result_t result;
+	size_t len = 0, i;
+
+	REQUIRE(DNS_MESSAGE_VALID(message));
+	REQUIRE(rdatasetp != NULL && *rdatasetp == NULL);
+
+	result = dns_message_gettemprdatalist(message, &rdatalist);
+	if (result != ISC_R_SUCCESS)
+		return (result);
+	result = dns_message_gettemprdata(message, &rdata);
+	if (result != ISC_R_SUCCESS)
+		goto cleanup;
+	result = dns_message_gettemprdataset(message, &rdataset);
+	if (result != ISC_R_SUCCESS)
+		goto cleanup;
+	dns_rdataset_init(rdataset);
+
+	rdatalist->type = dns_rdatatype_opt;
+	rdatalist->covers = 0;
+
+	/*
+	 * Set Maximum UDP buffer size.
+	 */
+	rdatalist->rdclass = udpsize;
+
+	/*
+	 * Set EXTENDED-RCODE and Z to 0.
+	 */
+	rdatalist->ttl = (version << 16);
+	rdatalist->ttl |= (flags & 0xffff);
+
+	/*
+	 * Set EDNS options if applicable
+	 */
+	if (count != 0) {
+		isc_buffer_t *buf = NULL;
+		for (i = 0; i < count; i++)
+			len += ednsopts[i].length + 4;
+
+		if (len > 0xffffU) {
+			result = ISC_R_NOSPACE;
+			goto cleanup;
+		}
+
+		result = isc_buffer_allocate(message->mctx, &buf, len);
+		if (result != ISC_R_SUCCESS)
+			goto cleanup;
+
+		for (i = 0; i < count; i++)  {
+			isc_buffer_putuint16(buf, ednsopts[i].code);
+			isc_buffer_putuint16(buf, ednsopts[i].length);
+			isc_buffer_putmem(buf, ednsopts[i].value,
+					  ednsopts[i].length);
+		}
+		rdata->data = isc_buffer_base(buf);
+		rdata->length = len;
+		dns_message_takebuffer(message, &buf);
+	} else {
+		rdata->data = NULL;
+		rdata->length = 0;
+	}
+
+	rdata->rdclass = rdatalist->rdclass;
+	rdata->type = rdatalist->type;
+	rdata->flags = 0;
+
+	ISC_LIST_INIT(rdatalist->rdata);
+	ISC_LIST_APPEND(rdatalist->rdata, rdata, link);
+	result = dns_rdatalist_tordataset(rdatalist, rdataset);
+	RUNTIME_CHECK(result == ISC_R_SUCCESS);
+
+	*rdatasetp = rdataset;
+	return (ISC_R_SUCCESS);
+
+ cleanup:
+	if (rdata != NULL)
+		dns_message_puttemprdata(message, &rdata);
+	if (rdataset != NULL)
+		dns_message_puttemprdataset(message, &rdataset);
+	if (rdatalist != NULL)
+		dns_message_puttemprdatalist(message, &rdatalist);
+	return (result);
+}

lib/dns/resolver.c

@@ -134,6 +134,7 @@
  * Maximum EDNS0 input packet size.
  */
 #define RECV_BUFFER_SIZE                4096            /* XXXRTH  Constant. */
+#define EDNSOPTS			2
 
 /*%
  * This defines the maximum number of timeouts we will permit before we
@@ -1288,67 +1289,15 @@ resquery_senddone(isc_task_t *task, isc_event_t *event) {
 
 static inline isc_result_t
 fctx_addopt(dns_message_t *message, unsigned int version,
-	    isc_uint16_t udpsize, isc_boolean_t request_nsid)
+	    isc_uint16_t udpsize, dns_ednsopt_t *ednsopts, size_t count)
 {
-	dns_rdataset_t *rdataset;
-	dns_rdatalist_t *rdatalist;
-	dns_rdata_t *rdata;
+	dns_rdataset_t *rdataset = NULL;
 	isc_result_t result;
 
-	rdatalist = NULL;
-	result = dns_message_gettemprdatalist(message, &rdatalist);
-	if (result != ISC_R_SUCCESS)
-		return (result);
-	rdata = NULL;
-	result = dns_message_gettemprdata(message, &rdata);
-	if (result != ISC_R_SUCCESS)
-		return (result);
-	rdataset = NULL;
-	result = dns_message_gettemprdataset(message, &rdataset);
+	result = dns_message_buildopt(message, &rdataset, version, udpsize,
+				      DNS_MESSAGEEXTFLAG_DO, ednsopts, count);
 	if (result != ISC_R_SUCCESS)
 		return (result);
-	dns_rdataset_init(rdataset);
-
-	rdatalist->type = dns_rdatatype_opt;
-	rdatalist->covers = 0;
-
-	/*
-	 * Set Maximum UDP buffer size.
-	 */
-	rdatalist->rdclass = udpsize;
-
-	/*
-	 * Set EXTENDED-RCODE and Z to 0, DO to 1.
-	 */
-	rdatalist->ttl = (version << 16);
-	rdatalist->ttl |= DNS_MESSAGEEXTFLAG_DO;
-
-	/*
-	 * Set EDNS options if applicable
-	 */
-	if (request_nsid) {
-		/* Send empty NSID option (RFC5001) */
-		unsigned char data[4];
-		isc_buffer_t buf;
-
-		isc_buffer_init(&buf, data, sizeof(data));
-		isc_buffer_putuint16(&buf, DNS_OPT_NSID);
-		isc_buffer_putuint16(&buf, 0);
-		rdata->data = data;
-		rdata->length = sizeof(data);
-	} else {
-		rdata->data = NULL;
-		rdata->length = 0;
-	}
-
-	rdata->rdclass = rdatalist->rdclass;
-	rdata->type = rdatalist->type;
-	rdata->flags = 0;
-
-	ISC_LIST_INIT(rdatalist->rdata);
-	ISC_LIST_APPEND(rdatalist->rdata, rdata, link);
-	RUNTIME_CHECK(dns_rdatalist_tordataset(rdatalist, rdataset) == ISC_R_SUCCESS);
-
 	return (dns_message_setopt(message, rdataset));
 }
 
@@ -1748,6 +1697,8 @@ resquery_send(resquery_t *query) {
 	isc_boolean_t cleanup_cctx = ISC_FALSE;
 	isc_boolean_t secure_domain;
 	isc_boolean_t connecting = ISC_FALSE;
+	dns_ednsopt_t ednsopts[EDNSOPTS];
+	unsigned ednsopt = 0;
 
 	fctx = query->fctx;
 	QTRACE("send");
@@ -1930,8 +1881,15 @@ resquery_send(resquery_t *query) {
 			/* request NSID for current view or peer? */
 			if (peer != NULL)
 				(void) dns_peer_getrequestnsid(peer, &reqnsid);
+			if (reqnsid) {
+				INSIST(ednsopt < EDNSOPTS);
+				ednsopts[ednsopt].code = DNS_OPT_NSID;
+				ednsopts[ednsopt].length = 0;
+				ednsopts[ednsopt].value = NULL;
+				ednsopt++;
+			}
 			result = fctx_addopt(fctx->qmessage, version,
-					     udpsize, reqnsid);
+					     udpsize, ednsopts, ednsopt);
 			if (reqnsid && result == ISC_R_SUCCESS) {
 				query->options |= DNS_FETCHOPT_WANTNSID;
 			} else if (result != ISC_R_SUCCESS) {
@@ -6711,44 +6669,24 @@ checknames(dns_message_t *message) {
 /*
  * Log server NSID at log level 'level'
  */
-static isc_result_t
-log_nsid(dns_rdataset_t *opt, resquery_t *query, int level, isc_mem_t *mctx)
+static void
+log_nsid(isc_buffer_t *opt, size_t nsid_len, resquery_t *query,
+	 int level, isc_mem_t *mctx)
 {
 	static const char hex[17] = "0123456789abcdef";
 	char addrbuf[ISC_SOCKADDR_FORMATSIZE];
-	isc_uint16_t optcode, nsid_len, buflen, i;
-	isc_result_t result;
-	isc_buffer_t nsidbuf;
-	dns_rdata_t rdata;
+	isc_uint16_t buflen, i;
 	unsigned char *p, *buf, *nsid;
 
-	/* Extract rdata from OPT rdataset */
-	result = dns_rdataset_first(opt);
-	if (result != ISC_R_SUCCESS)
-		return (ISC_R_FAILURE);
-
-	dns_rdata_init(&rdata);
-	dns_rdataset_current(opt, &rdata);
-	if (rdata.length < 4)
-		return (ISC_R_FAILURE);
-
-	/* Check for NSID */
-	isc_buffer_init(&nsidbuf, rdata.data, rdata.length);
-	isc_buffer_add(&nsidbuf, rdata.length);
-	optcode = isc_buffer_getuint16(&nsidbuf);
-	nsid_len = isc_buffer_getuint16(&nsidbuf);
-	if (optcode != DNS_OPT_NSID || nsid_len == 0)
-		return (ISC_R_FAILURE);
-
 	/* Allocate buffer for storing hex version of the NSID */
 	buflen = nsid_len * 2 + 1;
 	buf = isc_mem_get(mctx, buflen);
 	if (buf == NULL)
-		return (ISC_R_NOSPACE);
+		return;
 
 	/* Convert to hex */
 	p = buf;
-	nsid = rdata.data + 4;
+	nsid = isc_buffer_current(opt);
 	for (i = 0; i < nsid_len; i++) {
 		*p++ = hex[(nsid[0] >> 4) & 0xf];
 		*p++ = hex[nsid[0] & 0xf];
@@ -6764,7 +6702,7 @@ log_nsid(dns_rdataset_t *opt, resquery_t *query, int level, isc_mem_t *mctx)
 
 	/* Clean up */
 	isc_mem_put(mctx, buf, buflen);
-	return (ISC_R_SUCCESS);
+	return;
 }
 
 static isc_boolean_t
@@ -6800,6 +6738,41 @@ betterreferral(fetchctx_t *fctx) {
 	return (ISC_FALSE);
 }
 
+static void
+process_opt(resquery_t *query, dns_rdataset_t *opt) {
+	dns_rdata_t rdata;
+	isc_buffer_t optbuf;
+	isc_result_t result;
+	isc_uint16_t optcode;
+	isc_uint16_t optlen;
+
+	result = dns_rdataset_first(opt);
+	if (result == ISC_R_SUCCESS) {
+		dns_rdata_init(&rdata);
+		dns_rdataset_current(opt, &rdata);
+		isc_buffer_init(&optbuf, rdata.data, rdata.length);
+		isc_buffer_add(&optbuf, rdata.length);
+		while (isc_buffer_remaininglength(&optbuf) >= 4) {
+			optcode = isc_buffer_getuint16(&optbuf);
+			optlen = isc_buffer_getuint16(&optbuf);
+			INSIST(optlen <= isc_buffer_remaininglength(&optbuf));
+			switch (optcode) {
+			case DNS_OPT_NSID:
+				if (query->options & DNS_FETCHOPT_WANTNSID)
+					log_nsid(&optbuf, optlen, query,
+						 ISC_LOG_INFO,
+						 query->fctx->res->mctx);
+				isc_buffer_forward(&optbuf, optlen);
+				break;
+			default:
+				isc_buffer_forward(&optbuf, optlen);
+				break;
+			}
+		}
+		INSIST(isc_buffer_remaininglength(&optbuf) == 0U);
+	}
+}
+
 static void
 resquery_response(isc_task_t *task, isc_event_t *event) {
 	isc_result_t result = ISC_R_SUCCESS;
@@ -6991,12 +6964,11 @@ resquery_response(isc_task_t *task, isc_event_t *event) {
 			      ISC_LOG_DEBUG(10), fctx->res->mctx);
 
 	/*
-	 * Did we request NSID?  If so, and if the response contains
-	 * NSID data, log it at INFO level.
+	 * Process receive opt record.
 	 */
 	opt = dns_message_getopt(message);
-	if (opt != NULL && (query->options & DNS_FETCHOPT_WANTNSID) != 0)
-		log_nsid(opt, query, ISC_LOG_INFO, fctx->res->mctx);
+	if (opt != NULL)
+		process_opt(query, opt);
 
 	/*
 	 * If the message is signed, check the signature.  If not, this