On Mon, Jan 25, 2016 at 07:32:52AM +0000, Stuart Henderson wrote:
> On 2016/01/25 04:32, Jérémie Courrèges-Anglas wrote:
> > 
> > Hi Matthew,
> > 
> > Matthew Martin <phy1...@gmail.com> writes:
> > 
> > > On Sun, Jan 24, 2016 at 03:05:28AM -0600, Matthew Martin wrote:
> > >> Add a -R flag to tftpd for a read only mode. This allows for a tighter
> > >> pledge than currently possible because by default existing files can be
> > >> overwritten (but no new files created). Perhaps read only should be the
> > >> default since it is surprising that tftp can overwrite by default.
> > 
> > Files have to be world-writable to be overwritten ; thus except for the
> > tighter pledge request, I don't see the benefit.  What use case do you
> > have in mind?

The patch was motivated by the tighter pledge request. The same argument
about permissions could be made about the -c flag (just set the
directory o-w). More on use cases below.

> I don't see why it would be surprising that tftpd can write files, it's
> expected behaviour from a tftpd and the manual talks about writing to
> files in a reasonably prominent place on the first page.

It'd well documented, and the behavior of allowing files to be
overwritten but not created goes back to the original commit some 33
years ago. However both the original commit and the commit that
introduced the -c flag don't mention the rationale for allowing users
to overwrite but not create files. 

I'd bet the most common use case for tftpd is to serve PXE files or
similar where the files being served should not be modified. I'm failing
to find a use case where files need to be overwritten, but new files
must not be created. Even if such a case did exist, setting the files to
be overwritten as world-writable and the directory to just
world-readable would accomplish the same result.

Patch below provides a reasonable default of read only tftpd, with -c
allowing writing and creating.




Index: tftpd.8
===================================================================
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.8,v
retrieving revision 1.5
diff -u -p -r1.5 tftpd.8
--- tftpd.8     18 Jul 2015 05:32:56 -0000      1.5
+++ tftpd.8     26 Jan 2016 06:56:13 -0000
@@ -54,8 +54,7 @@ does not require an account or password 
 Due to the lack of authentication information,
 .Nm
 will allow only publicly readable files to be accessed.
-Files may be written only if they already exist and are publicly writable,
-unless the
+Files may be written only if the
 .Fl c
 flag is specified
 .Pq see below .
@@ -90,8 +89,7 @@ Forces
 .Nm
 to use IPv6 addresses only.
 .It Fl c
-Allow new files to be created;
-otherwise uploaded files must already exist.
+Allow existing files to be edited and new files to be created.
 Files are created with default permissions
 allowing anyone to read or write to them.
 .It Fl d
Index: tftpd.c
===================================================================
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.34
diff -u -p -r1.34 tftpd.c
--- tftpd.c     14 Dec 2015 16:34:55 -0000      1.34
+++ tftpd.c     26 Jan 2016 06:56:13 -0000
@@ -358,8 +358,13 @@ main(int argc, char *argv[])
            setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid))
                errx(1, "can't drop privileges");
 
-       if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) == -1)
-               err(1, "pledge");
+       if (cancreate) {
+               if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) == 
-1)
+                       err(1, "pledge");
+       } else {
+               if (pledge("stdio rpath dns inet", NULL) == -1)
+                       err(1, "pledge");
+       }
 
        event_init();
 
@@ -953,20 +958,16 @@ validate_access(struct tftp_client *clie
         */
        wmode = O_TRUNC;
        if (stat(filename, &stbuf) < 0) {
-               if (!cancreate)
-                       return (errno == ENOENT ? ENOTFOUND : EACCESS);
-               else {
-                       if ((errno == ENOENT) && (mode != RRQ))
-                               wmode |= O_CREAT;
-                       else
-                               return (EACCESS);
-               }
+               if (cancreate && (errno == ENOENT) && (mode != RRQ))
+                       wmode |= O_CREAT;
+               else
+                       return (EACCESS);
        } else {
                if (mode == RRQ) {
                        if ((stbuf.st_mode & (S_IRUSR >> 6)) == 0)
                                return (EACCESS);
                } else {
-                       if ((stbuf.st_mode & (S_IWUSR >> 6)) == 0)
+                       if (!cancreate || (stbuf.st_mode & (S_IWUSR >> 6)) == 0)
                                return (EACCESS);
                }
        }

Reply via email to