Re: tftp protocol

[jnemeth%victoria.tc.ca@localhost (John Nemeth)]

On May 31, 12:05pm, Patrick Welche wrote:
} On Thu, Jan 07, 2010 at 06:28:06AM -0800, John Nemeth wrote:
} > On May 29,  1:18pm, Patrick Welche wrote:
} >      Whilst I think the client should be fixed, this option is
} > non-contentious.  The only thing is I would check to see if any of the
} > other BSDs supports and use the same option letter.
} 
} Free/Open don't use 'p', and don't offer similar functionality.

     Good enough.

} >      The block counter should wrap as is done by both FreeBSD and
} > OpenBSD.  This is most likely necessary to support downloading large
} > files on certain devices (i.e. Cisco devices running IOS).  I need to
} > further investigate this, but will most likely be implementing this
} > change.
} 
} The attached patch does wrap to zero (successfully booted a vista image, and
} the wrap warning appeared in the logs.
} 
} 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   8 Jan 2010 17:16:46 -0000
} @@ -29,7 +29,7 @@
}  .\"
}  .\"  from: @(#)tftpd.8       8.1 (Berkeley) 6/4/93
}  .\"
} -.Dd June 11, 2003
} +.Dd November 13, 2009

     This will get changed to the actual commit date.

}  .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,15 @@
}  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 correctly transferred without client and server
} +supporting blocksize negotiation (RFCs 2347 and 2348). As a kludge,
} +.Nm
} +accepts a sequence of block numbers which wrap to zero after 65535.

     I'm not sure I'm crazy about the wording here, but I'm not sure
how I would fix it up.  I would probably make it a couple of sentences
and say something about legacy clients that don't support options.

} +.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   8 Jan 2010 17:16:47 -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)

     What's this -w option?

}               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();

     usage() should be on the next line to make the code easier to read.

} +                     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;
}       }
}  }
} @@ -918,20 +933,20 @@
}                               goto abort;
}  
}                       case ACK:
} -                             if (ap->th_block == 0) {
} +                             if (etftp && ap->th_block == 0) {
}                                       etftp = 0;
}                                       acklength = 0;
}                                       dp = r_init();
}                                       goto done;
}                               }
} -                             if (ap->th_block == block)
} +                             if (ap->th_block == (u_short)block)

     I would use uint16_t to make it clear what size is wanted.  If you
need a specific size, it's much better to specify it then make
assumptions about what size a type is.

}                                       goto done;
}                               if (debug)
}                                       syslog(LOG_DEBUG, "Resync ACK %u != %u",
}                                           (unsigned int)ap->th_block, block);
}                               /* Re-synchronize with the other side */
}                               (void) synchnet(peer, tftp_blksize);
} -                             if (ap->th_block == (block -1))
} +                             if (ap->th_block == (u_short)(block - 1))

     Same thing, use uint16_t.

}                                       goto send_data;
}                       default:
}                               syslog(LOG_INFO, "Received %s in sendfile\n",
} @@ -942,6 +957,9 @@
}  done:
}               if (debug)
}                       syslog(LOG_DEBUG, "Received ACK for block %u", block);
} +             if (block == UINT16_MAX && size == tftp_blksize)
} +                     syslog(LOG_WARNING,
} +                     "Block number wrapped (hint: increase block size)");

     Don't bother with the hint since the client is usually out of the
sysadmin's control.  Maybe also use LOG_DEBUG since this is normal
operation for the clients that do it.

}               block++;
}       } while (size == tftp_blksize || block == 1);
}  abort:
} @@ -980,6 +998,9 @@
}               }
}               if (debug)
}                       syslog(LOG_DEBUG, "Sending ACK for block %u\n", block);
} +             if (block == UINT16_MAX)
} +                     syslog(LOG_WARNING,
} +                     "Block number wrapped (hint: increase block size)");

     Where's the wrap?  block isn't being wrapped back to 0 nor are
comparisons being done against short/uint16_t.  It looks like this will
die at line 1011 when the block number from the client is compared
against 65536.  Have you tried uploading a large file?

}               block++;
}               (void) setjmp(timeoutbuf);
}  send_ack:

     The only other thing I would do is make wrapping an option in
order to satisfy the purists.  With the option, maybe also make it
possible to wrap to 1 instead of 0, since apparently there are some
clients that expect that.  However, I don't think that's of vital
importance for the first pass.

}-- End of excerpt from Patrick Welche