dnspr: only the answer to the query asked is relayed, or kept

Answers from the name servers now go into the shared name cache, so
they are checked before they are taken:

- a query goes to a name server under a random id of its own, the
  client's put back only in what is relayed to it;
- an answer over UDP is taken only from the address and port of the
  name server asked;
- the answer must have that id, be a response, and carry the question
  asked, byte for byte, over UDP and TCP alike;
- of its records only one owned by the question's name itself (a
  pointer to it) is kept in the cache;
- a name with a label of other characters than a host's, a '.' or a
  NUL among them, reads as another name once made text, so its answer
  is relayed but not kept.

Tests: an answer with another id is neither relayed nor kept, and a
name with a '.' inside a label is answered but not cached under the
name its text reads as.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/dnspr.c

@@ -30,9 +30,16 @@ struct dnsquery {
 	   TCP */
 	unsigned char *query;
 	int len;
+	/* the client's id, replaced in what goes to a name server by one of
+	   our own, and the length of the question section */
+	unsigned char cid[2];
+	int qend;
 	/* the name server asked, and over TCP what of its answer came */
 	int server;
 	int tcp, connecting;
+	/* a name with a label not of a host's characters, a '.' in one
+	   among them: it reads as another name, so its answer is not kept */
+	int nocache;
 	unsigned char *answer;
 	int got;
 };
@@ -74,22 +81,29 @@ static void dnscache(struct clientparam *param, struct dnsquery *q, unsigned cha
 	unsigned type;
 	uint32_t ttl;
 
-	if((q->type != 0x01 && q->type != 0x1c) || !q->host) return;
+	if((q->type != 0x01 && q->type != 0x1c) || !q->host || q->nocache) return;
 	while(k < len && buf[k]) k += buf[k] + 1;
 	k += 5;
 	n = buf[7] + (((unsigned)buf[6])<<8);
 	for(j = 0; j < n && k < len; j++){
-		/* the name: a pointer, or labels to the end */
+		/* the owner name: only a record of the question's name itself, a
-		if((buf[k] & 0xc0) == 0xc0) k += 2;
+		   pointer to it, is kept; any other is skipped */
+		int own = 0;
+
+		if(k + 1 < len && buf[k] == 0xc0 && buf[k+1] == 0x0c){
+			own = 1;
+			k += 2;
+		}
+		else if((buf[k] & 0xc0) == 0xc0) k += 2;
 		else {
-			while(k < len && buf[k]) k += buf[k] + 1;
+			while(k < len && buf[k] && !(buf[k] & 0xc0)) k += buf[k] + 1;
-			k++;
+			k += (k < len && (buf[k] & 0xc0))? 2 : 1;
 		}
 		if(k + 10 > len) return;
 		type = buf[k+1] + (((unsigned)buf[k])<<8);
 		rlen = buf[k+9] + (((unsigned)buf[k+8])<<8);
 		if(k + 10 + rlen > len) return;
-		if(type == q->type && rlen == (q->type == 0x01? 4 : 16)){
+		if(own && type == q->type && rlen == (q->type == 0x01? 4 : 16)){
 			ttl = ntohl(*(uint32_t *)(buf + k + 4));
 			if(ttl > 3600*12) ttl = 3600*12;
 			if(!ttl) ttl = 1;
@@ -109,6 +123,12 @@ static int dnsreply(struct udploop *u, struct udpsession *ss, unsigned char *buf
 	struct clientparam *param = ss->param;
 	struct dnsquery *q = ss->data;
 
+	/* an answer to this query: our id, and the question asked */
+	if(len < q->qend || buf[0] != q->query[2] || buf[1] != q->query[3] || !(buf[2] & 0x80) ||
+	   memcmp(buf + 4, q->query + 2 + 4, 2) || memcmp(buf + 12, q->query + 2 + 12, q->qend - 12))
+		return 0;
+	buf[0] = q->cid[0];
+	buf[1] = q->cid[1];
 	param->statssrv64 += len;
 	param->nreads++;
 	if(buf[6] || buf[7]){
@@ -138,6 +158,12 @@ static int dnsforward(struct udploop *u, struct udpsession *ss){
 		}
 		q->tcp = c->nservers[q->server].usetcp;
 		q->got = 0;
+		{
+			unsigned short id = (unsigned short)myrand();
+
+			q->query[2] = (unsigned char)(id >> 8);
+			q->query[3] = (unsigned char)(id & 0xff);
+		}
 		if((param->remsock = param->srv->so._socket(param->sostate, SASOCK(&c->nservers[q->server].addr),
 			q->tcp? SOCK_STREAM : SOCK_DGRAM, q->tcp? IPPROTO_TCP : IPPROTO_UDP)) == INVALID_SOCKET) continue;
 #ifdef _WIN32
@@ -201,8 +227,12 @@ static int dnsprfirst(struct udploop *u, struct udpsession *ss, unsigned char *q
 
 	if(buf[4]!=0 || buf[5]!=1) { free(buf); return (param->res = 816); }
 	for(len = 12; len<i; len+=(c+1)){
+		int k;
+
 		c = buf[len];
 		if(!c)break;
+		for(k = 1; k <= c && len + k < i; k++)
+			if(!isalnum(buf[len+k]) && buf[len+k] != '-' && buf[len+k] != '_') q->nocache = 1;
 		buf[len] = '.';
 	}
 	if(len > (i-4)) { free(buf); return (param->res = 817); }
@@ -211,6 +241,10 @@ static int dnsprfirst(struct udploop *u, struct udpsession *ss, unsigned char *q
 	*s2 = (len - (int)(s2 - buf)) - 1;
 
 	q->type = ((unsigned)buf[len+1])*256 + (unsigned)buf[len+2];
+	/* the question: the name, its type and class */
+	q->qend = len + 5;
+	q->cid[0] = q->query[2];
+	q->cid[1] = q->query[3];
 	q->ttl = 3600;
 	if(q->type==0x01 && param->srv->fakeip){
 		ip = 1;
@@ -304,12 +338,23 @@ static int dnsprserver(struct udploop *u, struct udpsession *ss){
 		us = (unsigned short)((q->answer[0] << 8) | q->answer[1]);
 		if(us > 4096 || us <= 13) return (param->res = 833);
 		if(q->got < us + 2) return 0;
-		return dnsreply(u, ss, q->answer + 2, us);
+		if(dnsreply(u, ss, q->answer + 2, us)) return 1;
+		/* not the answer asked for: the next name server */
+		q->server++;
+		return dnsforward(u, ss)? (param->res = 821) : 0;
+	}
+	{
+		PROXYSOCKADDRTYPE from;
+		SASIZETYPE size = sizeof(from);
+
+		len = param->srv->so._recvfrom(param->sostate, param->remsock, (char *)u->buf, UDPBUFSIZE, 0,
+			(struct sockaddr *)&from, &size);
+		if(len < 0 && (errno == EAGAIN || errno == EINTR)) return 0;
+		/* none at all, or not from the name server asked */
+		if(len <= 13 || *SAFAMILY(&from) != *SAFAMILY(&param->sinsr) ||
+		   memcmp(SAADDR(&from), SAADDR(&param->sinsr), SAADDRLEN(&param->sinsr)) ||
+		   *SAPORT(&from) != *SAPORT(&param->sinsr)) return 0;
 	}
-	len = param->srv->so._recvfrom(param->sostate, param->remsock, (char *)u->buf, UDPBUFSIZE, 0, NULL, NULL);
-	if(len < 0 && (errno == EAGAIN || errno == EINTR)) return 0;
-	/* an answer to another query, or none at all */
-	if(len <= 13 || u->buf[0] != q->query[2] || u->buf[1] != q->query[3]) return 0;
 	return dnsreply(u, ss, u->buf, len);
 }
 

tests/cases/udploop.py

@@ -43,7 +43,7 @@ def dns_reply(data, answer):
     return bytes(reply)
 
 
-def fake_name_server(answer, tcp=False):
+def fake_name_server(answer, tcp=False, wrongid=False):
     """A name server answering every query (see dns_reply), over UDP or
     TCP; with answer None, one which answers nothing."""
     sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM if tcp else socket.SOCK_DGRAM)
@@ -78,7 +78,10 @@ def fake_name_server(answer, tcp=False):
             except OSError:
                 return
             if answer is not None:
-                sock.sendto(dns_reply(data, answer), peer)
+                reply = dns_reply(data, answer)
+                if wrongid:
+                    reply = bytes([reply[0] ^ 0x55, reply[1]]) + reply[2:]
+                sock.sendto(reply, peer)
 
     if tcp:
         sock.listen(8)
@@ -298,5 +301,46 @@ def run(t):
     t.ok("the first name server silent, the second is asked after DNS_TO") \
         if got and b"the-second" in got else \
         t.fail("the first name server silent, the second is asked after DNS_TO", "the TXT", got)
-    for x in (s, pending, mute, tcpns, second):
+
+    # an answer with another id is not the answer: neither relayed nor kept
+    liar, lport = fake_name_server(b"forged", wrongid=True)
+    guarded = t.free_port()
+    t.start("udploop_liar", f"""
+        log
+        timeouts 1 5 30 60 180 1800 2 60 15 5 5
+        nserver 127.0.0.1:{lport}
+        nscache 256
+        nsrecord local.test 10.1.2.3
+        auth iponly
+        allow *
+        dnspr -p{guarded}
+    """)
+    t.wait_udp(guarded, payload=dns_raw("local.test", 1))
+    t.eq(None, exchange(s, guarded, dns_raw("forged.test", 1), timeout=3),
+         "an answer with another transaction id is not relayed")
+    t.eq(None, exchange(s, guarded, dns_raw("forged.test", 1), timeout=3),
+         "nor kept in the name cache")
+    # a label with a '.' in it is another name than its text reads: answered,
+    # but not kept under that text
+    caching, cport2 = fake_name_server(b"x")
+    dotted = t.free_port()
+    t.start("udploop_dotted", f"""
+        log
+        nserver 127.0.0.1:{cport2}
+        nscache 256
+        nsrecord local.test 10.1.2.3
+        auth iponly
+        allow *
+        dnspr -p{dotted}
+    """)
+    t.wait_udp(dotted, payload=dns_raw("local.test", 1))
+    odd = struct.pack("!HHHHHH", 0x4C4C, 0x0100, 1, 0, 0, 0) + b"\x08evil.dot\x04test\x00" + struct.pack("!HH", 1, 1)
+    got = exchange(s, dotted, odd)
+    t.ok("a name with a '.' inside a label is answered") \
+        if got and bytes([10, 9, 8, 7]) in got else t.fail("a name with a '.' inside a label is answered", "10.9.8.7", got)
+    caching.close()
+    time.sleep(0.2)
+    t.eq(None, exchange(s, dotted, dns_raw("evil.dot.test", 1), timeout=3),
+         "and the name its text reads as was not cached from it")
+    for x in (s, pending, mute, tcpns, second, liar):
         x.close()