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 -0000 1.5
+++ tftpd.8 24 Jan 2016 08:49:11 -0000
@@ -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 -0000 1.34
+++ tftpd.c 24 Jan 2016 08:49:11 -0000
@@ -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);
}
}

[Matthew Martin]

Post by 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

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 -0000 1.5
+++ tftpd.8 24 Jan 2016 18:09:07 -0000
@@ -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 -0000 1.34
+++ tftpd.c 24 Jan 2016 18:09:07 -0000
@@ -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);
}
}

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

Hi Matthew,

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

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?

[Stuart Henderson]

Post by Jérémie Courrèges-Anglas
Hi Matthew,

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

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.

[Matthew Martin]

Post by Stuart Henderson

Post by Jérémie Courrèges-Anglas
Hi Matthew,

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

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.

Post by Stuart Henderson
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);
}
}

[Stuart Henderson]

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

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

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

Post by Stuart Henderson

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

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

Post by Stuart Henderson

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

Post by Stuart Henderson
OK I see some use for -R.

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

Post by Stuart Henderson
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.