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

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

The following reply was made to PR bin/43164; it has been noted by GNATS.

From: Hubert Feyrer <hubert%feyrer.de@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost
Subject: Re: bin/43164 (tftpd: file upload broken)
Date: Fri, 16 Apr 2010 22:41:54 +0200 (CEST)

 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.