>	there are spec non-conformance in 4.4BSD tftpd/tftp.
>	if we strictly follow RFC1350, we should allow IP address pairs to be
>	switched - we should validate connection based on udp src/dst port
>	number only.
>
>	4.4BSD tftpd calls connect(2), which forbids us to change IP address
>	pairs.  NetBSD tftpd is worse because it calls bind(2) with
>	non-wildcard address as well.  4.4BSD tftp trusts server too much
>	(always copies port number got from recvfrom).
>
>	question - are there any other implementation that chokes, if we
>	correct it?

	here's a proposed patch.

itojun


Index: libexec/tftpd/tftpd.c
===================================================================
RCS file: /cvsroot/basesrc/libexec/tftpd/tftpd.c,v
retrieving revision 1.21
diff -u -r1.21 tftpd.c
--- libexec/tftpd/tftpd.c	2000/11/21 13:50:25	1.21
+++ libexec/tftpd/tftpd.c	2000/11/21 15:22:23
@@ -93,6 +93,7 @@
 char	ackbuf[PKTSIZE];
 struct	sockaddr_storage from;
 int	fromlen;
+struct	sockaddr_storage client;	/* valid peer port number */
 
 /*
  * Null-terminated directory prefix list for absolute pathname requests and
@@ -117,6 +118,7 @@
 static const char *errtomsg __P((int));
 static void nak __P((int));
 static char *verifyhost __P((struct sockaddr *));
+static int cmpport __P((struct sockaddr *, struct sockaddr *));
 static void usage __P((void));
 void timer __P((int));
 void sendfile __P((struct formats *));
@@ -158,7 +160,6 @@
 	int ch, on;
 	int fd = 0;
 	struct sockaddr_storage me;
-	int len;
 	char *tgtuser, *tgtgroup, *ep;
 	uid_t curuid, tgtuid;
 	gid_t curgid, tgtgid;
@@ -344,29 +345,6 @@
 		}
 	}
 
-	/*
-	 * remember what address this was sent to, so we can respond on the
-	 * same interface
-	 */
-	len = sizeof(me);
-	if (getsockname(fd, (struct sockaddr *)&me, &len) == 0) {
-		switch (me.ss_family) {
-		case AF_INET:
-			((struct sockaddr_in *)&me)->sin_port = 0;
-			break;
-		case AF_INET6:
-			((struct sockaddr_in6 *)&me)->sin6_port = 0;
-			break;
-		default:
-			/* unsupported */
-			break;
-		}
-	} else {
-		memset(&me, 0, sizeof(me));
-		me.ss_family = from.ss_family;
-		me.ss_len = from.ss_len;
-	}
-
 	alarm(0);
 	close(fd);
 	close(1);
@@ -375,14 +353,22 @@
 		syslog(LOG_ERR, "socket: %m");
 		exit(1);
 	}
+	/*
+	 * allocate port number for my side.
+	 * we shouldn't bind(2) nor connect(2) with explicit address.  if we
+	 * do, we would unintentionally fix IP address pairs.
+	 */
+	memset(&me, 0, sizeof(me));
+	me.ss_family = from.ss_family;
+	me.ss_len = from.ss_len;
 	if (bind(peer, (struct sockaddr *)&me, me.ss_len) < 0) {
 		syslog(LOG_ERR, "bind: %m");
 		exit(1);
 	}
-	if (connect(peer, (struct sockaddr *)&from, from.ss_len) < 0) {
-		syslog(LOG_ERR, "connect: %m");
-		exit(1);
-	}
+	/*
+	 * keep peer's port number, for future validation
+	 */
+	client = from;
 	tp = (struct tftphdr *)buf;
 	tp->th_opcode = ntohs(tp->th_opcode);
 	if (tp->th_opcode == RRQ || tp->th_opcode == WRQ)
@@ -610,19 +596,27 @@
 		(void)setjmp(timeoutbuf);
 
 send_data:
-		if (send(peer, dp, size + 4, 0) != size + 4) {
+		if (sendto(peer, dp, size + 4, 0, (struct sockaddr *)&from,
+		    fromlen) != size + 4) {
 			syslog(LOG_ERR, "tftpd: write: %m");
 			goto abort;
 		}
 		read_ahead(file, pf->f_convert);
 		for ( ; ; ) {
 			alarm(rexmtval);        /* read the ack */
-			n = recv(peer, ackbuf, sizeof (ackbuf), 0);
+			fromlen = sizeof(from);
+			n = recvfrom(peer, ackbuf, sizeof (ackbuf), 0,
+			    (struct sockaddr *)&from, &fromlen);
 			alarm(0);
 			if (n < 0) {
 				syslog(LOG_ERR, "tftpd: read: %m");
 				goto abort;
 			}
+			if (!cmpport((struct sockaddr *)&from,
+			    (struct sockaddr *)&client)) {
+				syslog(LOG_ERR, "tftpd: client port mismatch");
+				goto abort;
+			}
 			ap->th_opcode = ntohs((u_short)ap->th_opcode);
 			ap->th_block = ntohs((u_short)ap->th_block);
 
@@ -675,19 +669,27 @@
 		block++;
 		(void) setjmp(timeoutbuf);
 send_ack:
-		if (send(peer, ackbuf, 4, 0) != 4) {
+		if (sendto(peer, ackbuf, 4, 0, (struct sockaddr *)&from,
+		    fromlen) != 4) {
 			syslog(LOG_ERR, "tftpd: write: %m");
 			goto abort;
 		}
 		write_behind(file, pf->f_convert);
 		for ( ; ; ) {
 			alarm(rexmtval);
-			n = recv(peer, dp, PKTSIZE, 0);
+			fromlen = sizeof(from);
+			n = recvfrom(peer, dp, PKTSIZE, 0,
+			    (struct sockaddr *)&from, &fromlen);
 			alarm(0);
 			if (n < 0) {            /* really? */
 				syslog(LOG_ERR, "tftpd: read: %m");
 				goto abort;
 			}
+			if (!cmpport((struct sockaddr *)&from,
+			    (struct sockaddr *)&client)) {
+				syslog(LOG_ERR, "tftpd: client port mismatch");
+				goto abort;
+			}
 			dp->th_opcode = ntohs((u_short)dp->th_opcode);
 			dp->th_block = ntohs((u_short)dp->th_block);
 			if (dp->th_opcode == ERROR)
@@ -715,16 +717,25 @@
 
 	ap->th_opcode = htons((u_short)ACK);    /* send the "final" ack */
 	ap->th_block = htons((u_short)(block));
-	(void) send(peer, ackbuf, 4, 0);
+	(void) sendto(peer, ackbuf, 4, 0, (struct sockaddr *)&from, fromlen);
 
 	signal(SIGALRM, justquit);      /* just quit on timeout */
 	alarm(rexmtval);
-	n = recv(peer, buf, sizeof (buf), 0); /* normally times out and quits */
+	fromlen = sizeof(from);
+	/* normally times out and quits */
+	n = recvfrom(peer, buf, sizeof (buf), 0, (struct sockaddr *)&from,
+	    &fromlen);
 	alarm(0);
+	if (!cmpport((struct sockaddr *)&from, (struct sockaddr *)&client)) {
+		syslog(LOG_ERR, "tftpd: client port mismatch");
+		goto abort;
+	}
 	if (n >= 4 &&                   /* if read some data */
 	    dp->th_opcode == DATA &&    /* and got a data block */
 	    block == dp->th_block) {	/* then my last ack was lost */
-		(void) send(peer, ackbuf, 4, 0);     /* resend final ack */
+		/* resend final ack */
+		(void) sendto(peer, ackbuf, 4, 0, (struct sockaddr *)&from,
+		    fromlen);
 	}
 abort:
 	return;
@@ -791,7 +802,8 @@
 	}
 	length = strlen(tp->th_msg);
 	msglen = &tp->th_msg[length + 1] - buf;
-	if (send(peer, buf, msglen, 0) != msglen)
+	if (sendto(peer, buf, msglen, 0, (struct sockaddr *)&from,
+	    fromlen) != msglen)
 		syslog(LOG_ERR, "nak: %m");
 }
 
@@ -804,4 +816,21 @@
 	if (getnameinfo(fromp, fromp->sa_len, hbuf, sizeof(hbuf), NULL, 0, 0))
 		strlcpy(hbuf, "?", sizeof(hbuf));
 	return hbuf;
+}
+
+static int
+cmpport(sa, sb)
+	struct sockaddr *sa;
+	struct sockaddr *sb;
+{
+	char a[NI_MAXSERV], b[NI_MAXSERV];
+
+	if (getnameinfo(sa, sa->sa_len, NULL, 0, a, sizeof(a), NI_NUMERICSERV))
+		return 0;
+	if (getnameinfo(sb, sb->sa_len, NULL, 0, b, sizeof(b), NI_NUMERICSERV))
+		return 0;
+	if (strcmp(a, b) != 0)
+		return 0;
+
+	return 1;
 }
Index: usr.bin/tftp/tftp.c
===================================================================
RCS file: /cvsroot/basesrc/usr.bin/tftp/tftp.c,v
retrieving revision 1.14
diff -u -r1.14 tftp.c
--- usr.bin/tftp/tftp.c	2000/11/21 14:58:21	1.14
+++ usr.bin/tftp/tftp.c	2000/11/21 15:22:25
@@ -62,6 +62,7 @@
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
+#include <netdb.h>
 
 #include "extern.h"
 #include "tftpsubs.h"
@@ -86,6 +87,7 @@
 static void stopclock __P((void));
 static void timer __P((int));
 static void tpacket __P((const char *, struct tftphdr *, int));
+static int cmpport __P((struct sockaddr *, struct sockaddr *));
 
 /*
  * Send the requested file.
@@ -106,6 +108,7 @@
 	int fromlen;
 	FILE *file;
 	struct sockaddr_storage peer;
+	struct sockaddr_storage serv;	/* valid server port number */
 
 	startclock();		/* start stat's clock */
 	dp = r_init();		/* reset fillbuf/read-ahead code */
@@ -115,6 +118,7 @@
 	block = 0;
 	amount = 0;
 	memcpy(&peer, &peeraddr, peeraddr.ss_len);
+	memset(&serv, 0, sizeof(serv));
 
 	signal(SIGALRM, timer);
 	do {
@@ -154,19 +158,14 @@
 				warn("recvfrom");
 				goto abort;
 			}
-			switch (peer.ss_family) {	/* added */
-			case AF_INET:
-				((struct sockaddr_in *)&peer)->sin_port =
-				    ((struct sockaddr_in *)&from)->sin_port;
-				break;
-			case AF_INET6:
-				((struct sockaddr_in6 *)&peer)->sin6_port =
-				    ((struct sockaddr_in6 *)&from)->sin6_port;
-				break;
-			default:
-				/* unsupported */
-				break;
+			if (!serv.ss_family)
+				serv = from;
+			else if (!cmpport((struct sockaddr *)&serv,
+			    (struct sockaddr *)&from)) {
+				warn("server port mismatch");
+				goto abort;
 			}
+			peer = from;
 			if (trace)
 				tpacket("received", ap, n);
 			/* should verify packet came from server */
@@ -227,6 +226,7 @@
 	FILE *file;
 	volatile int convert;		/* true if converting crlf -> lf */
 	struct sockaddr_storage peer;
+	struct sockaddr_storage serv;	/* valid server port number */
 
 	startclock();
 	dp = w_init();
@@ -237,6 +237,7 @@
 	firsttrip = 1;
 	amount = 0;
 	memcpy(&peer, &peeraddr, peeraddr.ss_len);
+	memset(&serv, 0, sizeof(serv));
 
 	signal(SIGALRM, timer);
 	do {
@@ -273,19 +274,14 @@
 				warn("recvfrom");
 				goto abort;
 			}
-			switch (peer.ss_family) {	/* added */
-			case AF_INET:
-				((struct sockaddr_in *)&peer)->sin_port =
-				    ((struct sockaddr_in *)&from)->sin_port;
-				break;
-			case AF_INET6:
-				((struct sockaddr_in6 *)&peer)->sin6_port =
-				    ((struct sockaddr_in6 *)&from)->sin6_port;
-				break;
-			default:
-				/* unsupported */
-				break;
+			if (!serv.ss_family)
+				serv = from;
+			else if (!cmpport((struct sockaddr *)&serv,
+			    (struct sockaddr *)&from)) {
+				warn("server port mismatch");
+				goto abort;
 			}
+			peer = from;
 			if (trace)
 				tpacket("received", dp, n);
 			/* should verify client address */
@@ -499,4 +495,21 @@
 		longjmp(toplevel, -1);
 	}
 	longjmp(timeoutbuf, 1);
+}
+
+static int
+cmpport(sa, sb)
+	struct sockaddr *sa;
+	struct sockaddr *sb;
+{
+	char a[NI_MAXSERV], b[NI_MAXSERV];
+
+	if (getnameinfo(sa, sa->sa_len, NULL, 0, a, sizeof(a), NI_NUMERICSERV))
+		return 0;
+	if (getnameinfo(sb, sb->sa_len, NULL, 0, b, sizeof(b), NI_NUMERICSERV))
+		return 0;
+	if (strcmp(a, b) != 0)
+		return 0;
+
+	return 1;
 }