Re: bin/43164 (tftpd: file upload broken)

[Hubert Feyrer <hubert%feyrer.de@localhost>]

Updated patch (V2):

Currently, file uploads require files to exist. 
Allow file upload without that restriction if option -w is used.
Document with hint at proper security precautions.

Patch applies to netbsd-4 and netbsd-5 branch as of 20100414.


 - Hubert Feyrer <hubert%feyrer.de@localhost>


Index: tftpd.c
===================================================================
RCS file: /cvsroot/src/libexec/tftpd/tftpd.c,v
retrieving revision 1.31
diff -u -r1.31 tftpd.c
--- tftpd.c     21 Jul 2008 13:25:47 -0000      1.31
+++ tftpd.c     16 Apr 2010 20:35:11 -0000
@@ -108,6 +108,7 @@
 static int     logging;
 static int     secure;
 static char    *securedir;
+static int     unrestricted_writes;    /* uploaded files don't have to exist */

 struct formats;

@@ -170,7 +171,7 @@
        curuid = getuid();
        curgid = getgid();

-       while ((ch = getopt(argc, argv, "dg:lns:u:")) != -1)
+       while ((ch = getopt(argc, argv, "dg:lns:u:w")) != -1)
                switch (ch) {
                case 'd':
                        debug++;
@@ -197,6 +198,10 @@
                        tgtuser = optarg;
                        break;

+               case 'w':
+                       unrestricted_writes = 1;
+                       break;
+
                default:
                        usage();
                        break;
@@ -722,6 +727,8 @@
        static char      pathname[MAXPATHLEN];
        char            *filename;
        int              fd;
+       int              creat=0;
+       int              trunc=0;

        filename = *filep;

@@ -787,21 +794,46 @@
                                return (EACCESS);
                        *filep = filename = pathname;
                } else {
+                       int stat_rc;
+
                        /*
                         * If there's no directory list, take our cue from the
                         * absolute file request check above (*filename == '/'),
                         * and allow access to anything.
                         */
-                       if (stat(filename, &stbuf) < 0)
-                               return (errno == ENOENT ? ENOTFOUND : EACCESS);
-                       if (!S_ISREG(stbuf.st_mode))
-                               return (ENOTFOUND);
+                       stat_rc = stat(filename, &stbuf);
                        if (mode == RRQ) {
+                               /* Read request */
+                               if (stat_rc < 0)
+                                       return (errno == ENOENT ? ENOTFOUND : 
EACCESS);
+                               if (!S_ISREG(stbuf.st_mode))
+                                       return (ENOTFOUND);
                                if ((stbuf.st_mode & S_IROTH) == 0)
                                        return (EACCESS);
                        } else {
-                               if ((stbuf.st_mode & S_IWOTH) == 0)
-                                       return (EACCESS);
+                               /* Write request */
+                               if (stat_rc < 0) {
+                                       /* Can't stat */
+                                       if (errno == EACCES) {
+                                               /* Permission denied */
+                                               return EACCESS;
+                                       } else {
+                                               /* Not there */
+                                               if (unrestricted_writes) {
+                                                       /* need to creat new 
file! */
+                                                       creat = O_CREAT;
+                                               } else {
+                                                       /* Permission denied */
+                                                       return EACCESS;
+                                               }
+                                       }
+                               } else {
+                                       /* Can stat */
+                                       if ((stbuf.st_mode & S_IWOTH) == 0) {
+                                               return (EACCESS);
+                                       }
+                                       trunc = O_TRUNC;
+                               }
                        }
                        *filep = filename;
                }
@@ -810,9 +842,10 @@
        if (tftp_opt_tsize && mode == RRQ)
                tftp_tsize = (unsigned long) stbuf.st_size;

-       fd = open(filename, mode == RRQ ? O_RDONLY : O_WRONLY | O_TRUNC);
-       if (fd < 0)
+       fd = open(filename, mode == RRQ ? O_RDONLY : O_WRONLY | trunc | creat, 
0644); /*644=debatable... maybe 646? or 004? or 006, so the file can be 
overwritten? */
+       if (fd < 0) {
                return (errno + 100);
+       }
        file = fdopen(fd, (mode == RRQ)? "r":"w");
        if (file == NULL) {
                close(fd);
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     16 Apr 2010 20:35:11 -0000
@@ -29,7 +29,7 @@
 .\"
 .\"       from: @(#)tftpd.8       8.1 (Berkeley) 6/4/93
 .\"
-.Dd June 11, 2003
+.Dd April 16, 2010
 .Dt TFTPD 8
 .Os
 .Sh NAME
@@ -45,6 +45,7 @@
 .Op Fl n
 .Op Fl s Ar directory
 .Op Fl u Ar user
+.Op Fl w
 .Op Ar directory ...
 .Sh DESCRIPTION
 .Nm
@@ -68,7 +69,10 @@
 will allow only publicly readable files to be accessed.
 Filenames beginning in ``\|\fB.\|.\fP\|/'' or
 containing ``/\|\fB.\|.\fP\|/'' are not allowed.
-Files may be written to only if they already exist and are publicly writable.
+Unless option
+.Fl w
+is used,
+files may be written to only if they already exist and are publicly writable.
 .Pp
 Note that this extends the concept of
 .Qq public
@@ -141,6 +145,9 @@
 isn't also given, change the gid to that of
 .Ar user
 as well.
+.Fl w
+Allow unrestricted writing of new files, with no need for
+a prior existance.
 .El
 .Sh SEE ALSO
 .Xr tftp 1 ,
@@ -220,3 +227,9 @@
 sort of file-access restrictions in place.
 The exact methods are specific to each site and therefore
 difficult to document here.
+.Pp
+If unrestricted file upload is enabled via the
+.Fl w
+option, care should be taken that this can be used
+to fill up disk space in an uncontrolled manner
+if this is used in an insecure environment.