NetBSD Problem Report #13326

Received: (qmail 9658 invoked from network); 27 Jun 2001 17:11:44 -0000
Message-Id: <20010627170957.559B27BB@starfruit.itojun.org>
Date: Thu, 28 Jun 2001 02:09:57 +0900 (JST)
From: itojun@itojun.org
Reply-To: itojun@itojun.org
To: gnats-bugs@gnats.netbsd.org
Subject: tftp/tftpd uses fixed address pair, which they shouldn't
X-Send-Pr-Version: 3.95

>Number:         13326
>Category:       bin
>Synopsis:       tftp/tftpd uses fixed address pair, which they shouldn't
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    bin-bug-people
>State:          open
>Class:          change-request
>Submitter-Id:   net
>Arrival-Date:   Wed Jun 27 17:12:00 +0000 2001
>Closed-Date:    
>Last-Modified:  Wed Jun 27 17:51:01 +0000 2001
>Originator:     Jun-ichiro itojun Hagino
>Release:        all past releases including 4.[34]BSD
>Organization:
	itojun.org
>Environment:
System: NetBSD starfruit.itojun.org 1.5W NetBSD 1.5W (STARFRUIT) #508: Thu Jun 28 00:51:45 JST 2001 itojun@starfruit.itojun.org:/usr/home/itojun/NetBSD/src/sys/arch/i386/compile/STARFRUIT i386
Architecture: i386
Machine: i386
>Description:
	http://orange.kame.net/dev/query-pr.cgi?pr=254

	on the discussion, it is claimed that tftp/tftpd should not use
	fixed (src, srcport, dst, dstport) pair (or 4-tuple) - it should
	use (srcport, dstport) and should not check the address portion.

	from what i see from RFC1350, it seems that there's no such requirement
	to use fixed IP address pairs.  therefore, from standard standpoint
	it is more correct to check (srcport, dstport) only.

	not sure if it is safe to incorporate the patch - since 4.4BSD
	tftp/tftpd has widely been deployed, we may encounter peers which
	expect us to use fixed address pairs (= may break interoperability,
	by peers' bug).

	comments are welcome.
>How-To-Repeat:
>Fix:
	the diff was taken on kame repository, against netbsd tree somewhere
	between 1.5 and 1.5.1.  so it may not apply clean.

Index: libexec/tftpd/tftpd.8
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/libexec/tftpd/tftpd.8,v
retrieving revision 1.1.1.3
retrieving revision 1.5
diff -u -r1.1.1.3 -r1.5
Index: libexec/tftpd/tftpd.c
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/libexec/tftpd/tftpd.c,v
retrieving revision 1.1.1.4
retrieving revision 1.8
diff -u -r1.1.1.4 -r1.8
--- libexec/tftpd/tftpd.c	2001/06/27 16:59:38	1.1.1.4
+++ libexec/tftpd/tftpd.c	2001/06/27 17:02:17	1.8
@@ -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/main.c
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/usr.bin/tftp/main.c,v
retrieving revision 1.1.1.2
retrieving revision 1.5
diff -u -r1.1.1.2 -r1.5
--- usr.bin/tftp/main.c	2000/11/21 15:54:14	1.1.1.2
+++ usr.bin/tftp/main.c	2001/03/02 11:05:31	1.5
@@ -1,4 +1,4 @@
-/*	$NetBSD: main.c,v 1.12 1999/07/12 20:50:54 itojun Exp $	*/
+/*	$NetBSD: main.c,v 1.13 2000/11/21 14:28:54 itojun Exp $	*/

 /*
  * Copyright (c) 1983, 1993
@@ -40,7 +40,7 @@
 #if 0
 static char sccsid[] = "@(#)main.c	8.1 (Berkeley) 6/6/93";
 #else
-__RCSID("$NetBSD: main.c,v 1.12 1999/07/12 20:50:54 itojun Exp $");
+__RCSID("$NetBSD: main.c,v 1.13 2000/11/21 14:28:54 itojun Exp $");
 #endif
 #endif /* not lint */

@@ -201,6 +201,8 @@
 	}

 	for (res = res0; res; res = res->ai_next) {
+		if (res->ai_addrlen > sizeof(peeraddr))
+			continue;
 		f = socket(res->ai_family, res->ai_socktype, res->ai_protocol);
 		if (f < 0) {
 			cause = "socket";
@@ -223,6 +225,7 @@
 	if (f < 0)
 		warn("%s", cause);
 	else {
+		/* res->ai_addr <= sizeof(peeraddr) is guaranteed */
 		memcpy(&peeraddr, res->ai_addr, res->ai_addrlen);
 		if (res->ai_canonname) {
 			(void) strncpy(hostname, res->ai_canonname,
@@ -254,7 +257,7 @@
 		printf("usage: %s host-name [port]\n", argv[0]);
 		return;
 	}
-	if (argc == 3)
+	if (argc == 2)
 		setpeer0(argv[1], NULL);
 	else
 		setpeer0(argv[1], argv[2]);
Index: usr.bin/tftp/tftp.1
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/usr.bin/tftp/tftp.1,v
retrieving revision 1.1.1.2
retrieving revision 1.3
diff -u -r1.1.1.2 -r1.3
Index: usr.bin/tftp/tftp.c
===================================================================
RCS file: /cvsroot/kame/kame/netbsd/usr.bin/tftp/tftp.c,v
retrieving revision 1.1.1.3
retrieving revision 1.7
diff -u -r1.1.1.3 -r1.7
--- usr.bin/tftp/tftp.c	2000/11/21 15:54:15	1.1.1.3
+++ usr.bin/tftp/tftp.c	2000/11/24 06:18:19	1.7
@@ -1,4 +1,4 @@
-/*	$NetBSD: tftp.c,v 1.11 2000/01/21 17:08:36 mycroft Exp $	*/
+/*	$NetBSD: tftp.c,v 1.14 2000/11/21 14:58:21 itojun Exp $	*/

 /*
  * Copyright (c) 1983, 1993
@@ -38,7 +38,7 @@
 #if 0
 static char sccsid[] = "@(#)tftp.c	8.1 (Berkeley) 6/6/93";
 #else
-__RCSID("$NetBSD: tftp.c,v 1.11 2000/01/21 17:08:36 mycroft Exp $");
+__RCSID("$NetBSD: tftp.c,v 1.14 2000/11/21 14:58:21 itojun Exp $");
 #endif
 #endif /* not lint */

@@ -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.
@@ -99,12 +101,14 @@
 	struct tftphdr *ap;	   /* data and ack packets */
 	struct tftphdr *dp;
 	int n;
-	volatile int block, size, convert;
+	volatile unsigned int block;
+	volatile int size, convert;
 	volatile unsigned long amount;
 	struct sockaddr_storage from;
 	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 */
@@ -114,6 +118,7 @@
 	block = 0;
 	amount = 0;
 	memcpy(&peer, &peeraddr, peeraddr.ss_len);
+	memset(&serv, 0, sizeof(serv));

 	signal(SIGALRM, timer);
 	do {
@@ -153,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 */
@@ -218,13 +218,15 @@
 	struct tftphdr *ap;
 	struct tftphdr *dp;
 	int n;
-	volatile int block, size, firsttrip;
+	volatile unsigned int block;
+	volatile int size, firsttrip;
 	volatile unsigned long amount;
 	struct sockaddr_storage from;
 	int fromlen;
 	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();
@@ -235,6 +237,7 @@
 	firsttrip = 1;
 	amount = 0;
 	memcpy(&peer, &peeraddr, peeraddr.ss_len);
+	memset(&serv, 0, sizeof(serv));

 	signal(SIGALRM, timer);
 	do {
@@ -271,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 */
@@ -385,23 +383,26 @@
 	const struct errmsg *pe;
 	struct tftphdr *tp;
 	int length;
+	size_t msglen;

 	tp = (struct tftphdr *)ackbuf;
 	tp->th_opcode = htons((u_short)ERROR);
+	msglen = sizeof(ackbuf) - (&tp->th_msg[0] - ackbuf);
 	for (pe = errmsgs; pe->e_code >= 0; pe++)
 		if (pe->e_code == error)
 			break;
 	if (pe->e_code < 0) {
 		tp->th_code = EUNDEF;
-		strcpy(tp->th_msg, strerror(error - 100));
+		strlcpy(tp->th_msg, strerror(error - 100), msglen);
 	} else {
 		tp->th_code = htons((u_short)error);
-		strcpy(tp->th_msg, pe->e_msg);
+		strlcpy(tp->th_msg, pe->e_msg, msglen);
 	}
-	length = strlen(pe->e_msg) + 4;
+	length = strlen(tp->th_msg);
+	msglen = &tp->th_msg[length + 1] - ackbuf;
 	if (trace)
-		tpacket("sent", tp, length);
-	if (sendto(f, ackbuf, length, 0, peer, peer->sa_len) != length)
+		tpacket("sent", tp, (int)msglen);
+	if (sendto(f, ackbuf, msglen, 0, peer, peer->sa_len) != length)
 		warn("nak");
 }

@@ -494,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;
 }
>Release-Note:
>Audit-Trail:

From: David Brownlee <abs@formula1.com>
To: <itojun@itojun.org>
Cc: <gnats-bugs@gnats.netbsd.org>
Subject: Re: bin/13326: tftp/tftpd uses fixed address pair, which they
 shouldn't
Date: Fri, 27 Jun 2008 18:41:58 +0100 (BST)

 On Thu, 28 Jun 2001 itojun@itojun.org wrote:

 > 	on the discussion, it is claimed that tftp/tftpd should not use
 > 	fixed (src, srcport, dst, dstport) pair (or 4-tuple) - it should
 > 	use (srcport, dstport) and should not check the address portion.

 	Would this lose the ability to bind tftpd to specific interfaces?

 -- 
 		David/absolute		abs@formula1.com




From: Jun-ichiro itojun Hagino <itojun@iijlab.net>
To: David Brownlee <abs@formula1.com>
Cc: gnats-bugs@gnats.netbsd.org
Subject: Re: bin/13326: tftp/tftpd uses fixed address pair, which they shouldn't 
Date: Thu, 28 Jun 2001 02:40:49 +0900

 >> 	on the discussion, it is claimed that tftp/tftpd should not use
 >> 	fixed (src, srcport, dst, dstport) pair (or 4-tuple) - it should
 >> 	use (srcport, dstport) and should not check the address portion.
 >	Would this lose the ability to bind tftpd to specific interfaces?

 	from what i found from the KAME PR he claims that it is a bug.
 	not sure if he is right or not.

 itojun

From: David Brownlee <abs@formula1.com>
To: Jun-ichiro itojun Hagino <itojun@iijlab.net>
Cc: <gnats-bugs@gnats.netbsd.org>
Subject: Re: bin/13326: tftp/tftpd uses fixed address pair, which they
 shouldn't 
Date: Fri, 27 Jun 2008 18:53:13 +0100 (BST)

 On Thu, 28 Jun 2001, Jun-ichiro itojun Hagino wrote:

 >
 > >> 	on the discussion, it is claimed that tftp/tftpd should not use
 > >> 	fixed (src, srcport, dst, dstport) pair (or 4-tuple) - it should
 > >> 	use (srcport, dstport) and should not check the address portion.
 > >	Would this lose the ability to bind tftpd to specific interfaces?
 >
 > 	from what i found from the KAME PR he claims that it is a bug.
 > 	not sure if he is right or not.

 	I can see utility both ways - maybe it should accept any src/dst
 	combination by default, but have an option to enforce checking?

 -- 
 		David/absolute		abs@formula1.com



>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2007 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.