Re: Add readonly flag to tftpd

[Stuart Henderson]

On 2016/01/26 02:06, Matthew Martin wrote:
> 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.

It's used to allow devices to write files (config, core dumps, etc)
to a tftp server given a known filename, but not allow just anybody
with network access to write random files to the server.

> 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.

OK I see some use for -R. I'm not sure about making it default, my
opinion could be swayed in either way with good arguments but if done
that the change needs to be communicated clearly (if I am relying on
it for a core dump for some network device, I don't want that to be
quietly disabled by an update). I am actively opposed to removing
the "allow only specific files to be written" feature.

Re: Add readonly flag to tftpd

[Matthew Martin]

On Mon, Jan 25, 2016 at 07:32:52AM +, Stuart Henderson wrote:
> On 2016/01/25 04:32, Jérémie Courrèges-Anglas wrote:
> > 
> > Hi Matthew,
> > 
> > Matthew Martin  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 -  1.5
+++ tftpd.8 26 Jan 2016 06:56:13 -
@@ -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 -  1.34
+++ tftpd.c 26 Jan 2016 06:56:13 -
@@ -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, ) < 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);
}
}

Re: Add readonly flag to tftpd

[Jérémie Courrèges-Anglas]

Stuart Henderson  writes:

> On 2016/01/26 02:06, Matthew Martin wrote:
>> 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.
>
> It's used to allow devices to write files (config, core dumps, etc)
> to a tftp server given a known filename, but not allow just anybody
> with network access to write random files to the server.
>
>> 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.

Tim Toady, I don't think that's the point here.

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

Maybe I'm dense but I still can't see what's the point.  tftpd *is*
read-only by default, only the admin can start tftpd, choose the
directory where it looks for files, and set the permissions on those
files.

It may not work like you thought it did, but it's been working like that
since ~30 years - as you mentioned.  Changing people's habits isn't
a bad thing per se, if it brings a clear benefit.

> OK I see some use for -R.

Preventing someone from shooting himself in the foot 'cause he messed up
permissions?

> I'm not sure about making it default, my
> opinion could be swayed in either way with good arguments but if done
> that the change needs to be communicated clearly (if I am relying on
> it for a core dump for some network device, I don't want that to be
> quietly disabled by an update). I am actively opposed to removing
> the "allow only specific files to be written" feature.

I fully agree.  -c is for "create", please don't change its semantics.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Add readonly flag to tftpd

[Matthew Martin]

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.

- Matthew Martin



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 -  1.5
+++ tftpd.8 24 Jan 2016 08:49:11 -
@@ -37,7 +37,7 @@
 .Nd DARPA Trivial File Transfer Protocol daemon
 .Sh SYNOPSIS
 .Nm tftpd
-.Op Fl 46cdv
+.Op Fl 46cdRv
 .Op Fl l Ar address
 .Op Fl p Ar port
 .Op Fl r Ar socket
@@ -113,6 +113,8 @@ listens on the port indicated in the
 .Ql tftp
 service description; see
 .Xr services 5 .
+.It Fl R
+Only allow read requests.
 .It Fl r Ar socket
 Issue filename rewrite requests to the specified UNIX domain socket.
 .Nm
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 -  1.34
+++ tftpd.c 24 Jan 2016 08:49:11 -
@@ -268,6 +268,7 @@ usage(void)
 }
 
 int  cancreate = 0;
+int  readonly = 0;
 int  verbose = 0;
 
 int
@@ -286,7 +287,7 @@ main(int argc, char *argv[])
char *port = "tftp";
int family = AF_UNSPEC;
 
-   while ((c = getopt(argc, argv, "46cdl:p:r:v")) != -1) {
+   while ((c = getopt(argc, argv, "46cdl:p:Rr:v")) != -1) {
switch (c) {
case '4':
family = AF_INET;
@@ -296,6 +297,7 @@ main(int argc, char *argv[])
break;
case 'c':
cancreate = 1;
+   readonly = 0;
break;
case 'd':
verbose = debug = 1;
@@ -306,6 +308,10 @@ main(int argc, char *argv[])
case 'p':
port = optarg;
break;
+   case 'R':
+   readonly = 1;
+   cancreate = 0;
+   break;
case 'r':
rewrite = optarg;
break;
@@ -358,8 +364,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 (readonly) {
+   if (pledge("stdio rpath dns inet", NULL) == -1)
+   err(1, "pledge");
+   } else {
+   if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) == 
-1)
+   err(1, "pledge");
+   }
 
event_init();
 
@@ -966,7 +977,7 @@ validate_access(struct tftp_client *clie
if ((stbuf.st_mode & (S_IRUSR >> 6)) == 0)
return (EACCESS);
} else {
-   if ((stbuf.st_mode & (S_IWUSR >> 6)) == 0)
+   if (readonly || (stbuf.st_mode & (S_IWUSR >> 6)) == 0)
return (EACCESS);
}
}

Re: Add readonly flag to tftpd

[Stuart Henderson]

On 2016/01/25 04:32, Jérémie Courrèges-Anglas wrote:
> 
> Hi Matthew,
> 
> Matthew Martin  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?

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.

Re: Add readonly flag to tftpd

[Matthew Martin]

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.
> 
> - Matthew Martin

This time not forgetting usage(). (Thanks Jason.)


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 -  1.5
+++ tftpd.8 24 Jan 2016 18:09:07 -
@@ -37,7 +37,7 @@
 .Nd DARPA Trivial File Transfer Protocol daemon
 .Sh SYNOPSIS
 .Nm tftpd
-.Op Fl 46cdv
+.Op Fl 46cdRv
 .Op Fl l Ar address
 .Op Fl p Ar port
 .Op Fl r Ar socket
@@ -113,6 +113,8 @@ listens on the port indicated in the
 .Ql tftp
 service description; see
 .Xr services 5 .
+.It Fl R
+Only allow read requests.
 .It Fl r Ar socket
 Issue filename rewrite requests to the specified UNIX domain socket.
 .Nm
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 -  1.34
+++ tftpd.c 24 Jan 2016 18:09:07 -
@@ -262,12 +262,13 @@ __dead void
 usage(void)
 {
extern char *__progname;
-   fprintf(stderr, "usage: %s [-46cdv] [-l address] [-p port] [-r socket]"
+   fprintf(stderr, "usage: %s [-46cdRv] [-l address] [-p port] [-r socket]"
" directory\n", __progname);
exit(1);
 }
 
 int  cancreate = 0;
+int  readonly = 0;
 int  verbose = 0;
 
 int
@@ -286,7 +287,7 @@ main(int argc, char *argv[])
char *port = "tftp";
int family = AF_UNSPEC;
 
-   while ((c = getopt(argc, argv, "46cdl:p:r:v")) != -1) {
+   while ((c = getopt(argc, argv, "46cdl:p:Rr:v")) != -1) {
switch (c) {
case '4':
family = AF_INET;
@@ -296,6 +297,7 @@ main(int argc, char *argv[])
break;
case 'c':
cancreate = 1;
+   readonly = 0;
break;
case 'd':
verbose = debug = 1;
@@ -306,6 +308,10 @@ main(int argc, char *argv[])
case 'p':
port = optarg;
break;
+   case 'R':
+   readonly = 1;
+   cancreate = 0;
+   break;
case 'r':
rewrite = optarg;
break;
@@ -358,8 +364,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 (readonly) {
+   if (pledge("stdio rpath dns inet", NULL) == -1)
+   err(1, "pledge");
+   } else {
+   if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) == 
-1)
+   err(1, "pledge");
+   }
 
event_init();
 
@@ -966,7 +977,7 @@ validate_access(struct tftp_client *clie
if ((stbuf.st_mode & (S_IRUSR >> 6)) == 0)
return (EACCESS);
} else {
-   if ((stbuf.st_mode & (S_IWUSR >> 6)) == 0)
+   if (readonly || (stbuf.st_mode & (S_IWUSR >> 6)) == 0)
return (EACCESS);
}
}