Re: tftp protocol

[Patrick Welche <prlw1%cam.ac.uk@localhost>]

The tftpd discussion was a while ago now. In brief, I had a strange
"opcode 768" error when tftp'ing a large file. This was because
the block number of the transfer overflowed its 16bit integer. This
wasn't obvious to me because the error was obscure. I propose in
the attached patch bailing out and printing a sensible error message
on overflow. It also wasn't obvious because our tcpdump doesn't
know about tftp options, and I attached a patch already in this
thread, which will become redundant once the in-tree tcpdump is
upgraded as tcpdump has learnt about tftp options in the meantime.

The other part was how to deal with clients which use a different path
separator to unix, and I proposed a -p option to tftpd (enclosed once
more). (I prefer this to the one Manuel passed on which swapped all \
for / during connection phase rather than just the filename.)

The number of blocks mentioned in the BUGS section was pointed out to
be incorrect, and that is amended in the attached patch (hopefully
correctly this time, but correct me if I'm wrong!)


We could reopen the argument on whether bailing with an error is better
than choosing to overflow to 0 (with the "block 0" is special for option
acknowledgement problem) or to 1 (I haven't actually seen a client do this).
Neither of those options is "right".

BTW that (u_short) patch I had sent in this thread is wrong, as it
effectively enabled overflow to 0 but didn't treat the not-first block 0 in
the same way as other blocks. I think the patch Manuel passed on did this
correctly.

Thoughts?

Cheers,

Patrick

Index: tftpd.8
===================================================================
RCS file: /cvsroot/src/libexec/tftpd/tftpd.8,v
retrieving revision 1.21
diff -u -r1.21 tftpd.8
--- tftpd.8     7 Aug 2003 09:46:53 -0000       1.21
+++ tftpd.8     5 Jan 2010 16:14:30 -0000
@@ -29,7 +29,7 @@
 .\"
 .\"    from: @(#)tftpd.8       8.1 (Berkeley) 6/4/93
 .\"
-.Dd June 11, 2003
+.Dd November 13, 2009
 .Dt TFTPD 8
 .Os
 .Sh NAME
@@ -43,6 +43,7 @@
 .Op Fl g Ar group
 .Op Fl l
 .Op Fl n
+.Op Fl p Ar path separator
 .Op Fl s Ar directory
 .Op Fl u Ar user
 .Op Ar directory ...
@@ -107,6 +108,11 @@
 .It Fl n
 Suppresses negative acknowledgement of requests for nonexistent
 relative filenames.
+.It Fl p Ar path separator
+All occurances of the single character
+.Ar path separator
+in the requested filename are replaced with
+.Sq / .
 .It Fl s Ar directory
 .Nm
 will
@@ -193,11 +199,13 @@
 and first appeared in
 .Nx 2.0 .
 .Sh BUGS
-Files larger than 33488896 octets (65535 blocks) cannot be transferred
-without client and server supporting blocksize negotiation (RFCs
-2347 and 2348).
-.Pp
-Many tftp clients will not transfer files over 16744448 octets (32767 blocks).
+Files larger than 33,553,919 octets (65535 blocks, last one <512
+octets) cannot be transferred without client and server supporting
+blocksize negotiation (RFCs 2347 and 2348).
+.Pp
+Many tftp clients will not transfer files over 16,776,703 octets
+(32767 blocks), as they incorrectly count the block number using
+a signed rather than unsigned 16-bit integer.
 .Sh SECURITY CONSIDERATIONS
 You are
 .Em strongly
Index: tftpd.c
===================================================================
RCS file: /cvsroot/src/libexec/tftpd/tftpd.c,v
retrieving revision 1.32
diff -u -r1.32 tftpd.c
--- tftpd.c     16 Mar 2009 01:56:21 -0000      1.32
+++ tftpd.c     5 Jan 2010 16:14:30 -0000
@@ -107,6 +107,7 @@
 static int     suppress_naks;
 static int     logging;
 static int     secure;
+static char    pathsep = '\0';
 static char    *securedir;
 
 struct formats;
@@ -141,7 +142,7 @@
 {
 
        syslog(LOG_ERR,
-    "Usage: %s [-dln] [-u user] [-g group] [-s directory] [directory ...]",
+    "Usage: %s [-dln] [-u user] [-g group] [-s directory] [-p pathsep] 
[directory ...]",
                    getprogname());
        exit(1);
 }
@@ -170,7 +171,7 @@
        curuid = getuid();
        curgid = getgid();
 
-       while ((ch = getopt(argc, argv, "dg:lns:u:")) != -1)
+       while ((ch = getopt(argc, argv, "dg:lnp:s:u:w:")) != -1)
                switch (ch) {
                case 'd':
                        debug++;
@@ -188,6 +189,11 @@
                        suppress_naks = 1;
                        break;
 
+               case 'p':
+                       pathsep = optarg[0];
+                       if (optarg[1] != '\0' || optarg[0] == '\0') usage();
+                       break;
+
                case 's':
                        secure = 1;
                        securedir = optarg;
@@ -662,6 +668,15 @@
                        exit(1);
                }
        }
+       /*
+        * Globally replace the path separator given in the -p option
+        * with / to cope with clients expecting a non-unix path separator.
+        */
+       if (pathsep != '\0') {
+               for (cp = filename; *cp != '\0'; ++cp) {
+                       if (*cp == pathsep) *cp = '/';
+               }
+       }
        ecode = (*pf->f_validate)(&filename, tp->th_opcode);
        if (logging) {
                syslog(LOG_INFO, "%s: %s request for %s: %s",
@@ -853,7 +868,7 @@
        case OACK:
                return "OACK";
        default:
-               (void)snprintf(obuf, sizeof(obuf), "*code %d*", code);
+               (void)snprintf(obuf, sizeof(obuf), "*code 0x%x*", code);
                return obuf;
        }
 }
@@ -882,6 +897,11 @@
        }
 
        do {
+               if (block > UINT16_MAX) {
+                       syslog(LOG_ERR, "tftpd: block number wrapped (hint: 
increase block size)");
+                       nak(EBADOP);
+                       goto abort;
+               }
                if (block > 0) {
                        size = readit(file, &dp, tftp_blksize, pf->f_convert);
                        if (size < 0) {