Subject: bin/13326: tftp/tftpd uses fixed address pair, which they shouldn't
To: None <gnats-bugs@gnats.netbsd.org>
From: None <itojun@itojun.org>
List: netbsd-bugs
Date: 06/28/2001 02:09:57
>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 10:12:00 PDT 2001
>Closed-Date:
>Last-Modified:
>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:
>Unformatted: