Re: tftpd: add -R for read-only mode/reduced pledges
On Tue, Oct 04, 2022 at 09:43:40AM +, Klemens Nanni wrote:
> On Tue, Oct 04, 2022 at 09:24:04AM +, Klemens Nanni wrote:
> > On Mon, Oct 03, 2022 at 06:43:26PM -0600, Theo de Raadt wrote:
> > > David Gwynne wrote:
> > >
> > > > On Sun, Oct 02, 2022 at 06:32:04PM +, Klemens Nanni wrote:
> > > > > diskless(8) just needs tftpd(8) to deliver files, none of the possibly
> > > > > untrusted clients are supposed to ever write anything.
> > > > >
> > > > > Either way, even when run without -c, a single file writable by _tftpd
> > > > > might be enough for a malicious client to fill up the server's disk.
> > > > >
> > > > > A proper read-only mode ("stdio rpath dns inet") seems much safer.
> > > >
> > > > agreed. i'm ok with this diff, but it's worth asking if we can make the
> > > > default read-only and ask people to opt in for write (and create) before
> > > > this specific diff goes in. ie, read-only be default, '-w' to enable
> > > > write mode, '-c' to enable write+create?
> > >
> > > we were read-only believers a long time ago, and it seems the world has
> > > caught up to our way of thinking so yes maybe it is time to make it an
> > > option you must specify.
> >
> > I like the idea, then -c should logically imply -w.
> >
> > Feedback? OK?
>
> Now with the important manual bits explaining the new defaul.t
ok by me.
>
> Index: tftpd.8
> ===
> RCS file: /cvs/src/usr.sbin/tftpd/tftpd.8,v
> retrieving revision 1.8
> diff -u -p -r1.8 tftpd.8
> --- tftpd.8 4 Mar 2019 01:06:03 - 1.8
> +++ tftpd.8 4 Oct 2022 09:42:16 -
> @@ -37,7 +37,7 @@
> .Nd Trivial File Transfer Protocol daemon
> .Sh SYNOPSIS
> .Nm tftpd
> -.Op Fl 46cdiv
> +.Op Fl 46cdivw
> .Op Fl l Ar address
> .Op Fl p Ar port
> .Op Fl r Ar socket
> @@ -53,11 +53,13 @@ does not require an account or password
> Due to the lack of authentication information,
> .Nm
> will allow only publicly readable files to be accessed.
> +By default files may only be read, unless the
> +.Fl w
> +option is specified.
> Files may be written only if they already exist and are publicly writable,
> unless the
> .Fl c
> -flag is specified
> -.Pq see below .
> +flag is specified.
> Note that this extends the concept of
> .Dq public
> to include
> @@ -93,6 +95,9 @@ Allow new files to be created;
> otherwise uploaded files must already exist.
> Files are created with default permissions
> allowing anyone to read or write to them.
> +.Pp
> +This option implies
> +.Fl w .
> .It Fl d
> Do not daemonize.
> If this option is specified,
> @@ -145,6 +150,8 @@ to
> on startup;
> the remote host is not expected to pass the directory
> as part of the file name to transfer.
> +.It Fl w
> +Allow files to be written to.
> .El
> .Sh SEE ALSO
> .Xr tftp 1 ,
> Index: tftpd.c
> ===
> RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 tftpd.c
> --- tftpd.c 4 Oct 2022 07:05:28 - 1.48
> +++ tftpd.c 4 Oct 2022 09:42:26 -
> @@ -283,12 +283,13 @@ __dead void
> usage(void)
> {
> extern char *__progname;
> - fprintf(stderr, "usage: %s [-46cdiv] [-l address] [-p port] [-r socket]"
> + fprintf(stderr, "usage: %s [-46cdivw] [-l address] [-p port] [-r
> socket]"
> " directory\n", __progname);
> exit(1);
> }
>
> intcancreate = 0;
> +intcanwrite = 0;
> intverbose = 0;
> intdebug = 0;
> intiflag = 0;
> @@ -309,7 +310,7 @@ main(int argc, char *argv[])
> int family = AF_UNSPEC;
> int devnull = -1;
>
> - while ((c = getopt(argc, argv, "46cdil:p:r:v")) != -1) {
> + while ((c = getopt(argc, argv, "46cdil:p:r:vw")) != -1) {
> switch (c) {
> case '4':
> family = AF_INET;
> @@ -318,7 +319,7 @@ main(int argc, char *argv[])
> family = AF_INET6;
> break;
> case 'c':
> - cancreate = 1;
> + canwrite = cancreate = 1;
> break;
> case 'd':
> verbose = debug = 1;
> @@ -342,6 +343,9 @@ main(int argc, char *argv[])
> case 'v':
> verbose = 1;
> break;
> + case 'w':
> + canwrite = 1;
> + break;
> default:
> usage();
> /* NOTREACHED */
> @@ -394,9 +398,12 @@ main(int argc, char *argv[])
> if (cancreate) {
> if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) ==
> -1)
> lerr(1, "pledge");
> - } else {
> + } else if (canwrite) {
> if (pledge("stdio rpath wpath fattr dns inet", NULL) == -1)
> lerr(1, "pledge");
Re: tftpd: add -R for read-only mode/reduced pledges
On Tue, Oct 04, 2022 at 09:24:04AM +, Klemens Nanni wrote:
> On Mon, Oct 03, 2022 at 06:43:26PM -0600, Theo de Raadt wrote:
> > David Gwynne wrote:
> >
> > > On Sun, Oct 02, 2022 at 06:32:04PM +, Klemens Nanni wrote:
> > > > diskless(8) just needs tftpd(8) to deliver files, none of the possibly
> > > > untrusted clients are supposed to ever write anything.
> > > >
> > > > Either way, even when run without -c, a single file writable by _tftpd
> > > > might be enough for a malicious client to fill up the server's disk.
> > > >
> > > > A proper read-only mode ("stdio rpath dns inet") seems much safer.
> > >
> > > agreed. i'm ok with this diff, but it's worth asking if we can make the
> > > default read-only and ask people to opt in for write (and create) before
> > > this specific diff goes in. ie, read-only be default, '-w' to enable
> > > write mode, '-c' to enable write+create?
> >
> > we were read-only believers a long time ago, and it seems the world has
> > caught up to our way of thinking so yes maybe it is time to make it an
> > option you must specify.
>
> I like the idea, then -c should logically imply -w.
>
> Feedback? OK?
Now with the important manual bits explaining the new defaul.t
Index: tftpd.8
===
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.8,v
retrieving revision 1.8
diff -u -p -r1.8 tftpd.8
--- tftpd.8 4 Mar 2019 01:06:03 - 1.8
+++ tftpd.8 4 Oct 2022 09:42:16 -
@@ -37,7 +37,7 @@
.Nd Trivial File Transfer Protocol daemon
.Sh SYNOPSIS
.Nm tftpd
-.Op Fl 46cdiv
+.Op Fl 46cdivw
.Op Fl l Ar address
.Op Fl p Ar port
.Op Fl r Ar socket
@@ -53,11 +53,13 @@ does not require an account or password
Due to the lack of authentication information,
.Nm
will allow only publicly readable files to be accessed.
+By default files may only be read, unless the
+.Fl w
+option is specified.
Files may be written only if they already exist and are publicly writable,
unless the
.Fl c
-flag is specified
-.Pq see below .
+flag is specified.
Note that this extends the concept of
.Dq public
to include
@@ -93,6 +95,9 @@ Allow new files to be created;
otherwise uploaded files must already exist.
Files are created with default permissions
allowing anyone to read or write to them.
+.Pp
+This option implies
+.Fl w .
.It Fl d
Do not daemonize.
If this option is specified,
@@ -145,6 +150,8 @@ to
on startup;
the remote host is not expected to pass the directory
as part of the file name to transfer.
+.It Fl w
+Allow files to be written to.
.El
.Sh SEE ALSO
.Xr tftp 1 ,
Index: tftpd.c
===
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.48
diff -u -p -r1.48 tftpd.c
--- tftpd.c 4 Oct 2022 07:05:28 - 1.48
+++ tftpd.c 4 Oct 2022 09:42:26 -
@@ -283,12 +283,13 @@ __dead void
usage(void)
{
extern char *__progname;
- fprintf(stderr, "usage: %s [-46cdiv] [-l address] [-p port] [-r socket]"
+ fprintf(stderr, "usage: %s [-46cdivw] [-l address] [-p port] [-r
socket]"
" directory\n", __progname);
exit(1);
}
int cancreate = 0;
+int canwrite = 0;
int verbose = 0;
int debug = 0;
int iflag = 0;
@@ -309,7 +310,7 @@ main(int argc, char *argv[])
int family = AF_UNSPEC;
int devnull = -1;
- while ((c = getopt(argc, argv, "46cdil:p:r:v")) != -1) {
+ while ((c = getopt(argc, argv, "46cdil:p:r:vw")) != -1) {
switch (c) {
case '4':
family = AF_INET;
@@ -318,7 +319,7 @@ main(int argc, char *argv[])
family = AF_INET6;
break;
case 'c':
- cancreate = 1;
+ canwrite = cancreate = 1;
break;
case 'd':
verbose = debug = 1;
@@ -342,6 +343,9 @@ main(int argc, char *argv[])
case 'v':
verbose = 1;
break;
+ case 'w':
+ canwrite = 1;
+ break;
default:
usage();
/* NOTREACHED */
@@ -394,9 +398,12 @@ main(int argc, char *argv[])
if (cancreate) {
if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) ==
-1)
lerr(1, "pledge");
- } else {
+ } else if (canwrite) {
if (pledge("stdio rpath wpath fattr dns inet", NULL) == -1)
lerr(1, "pledge");
+ } else {
+ if (pledge("stdio rpath dns inet", NULL) == -1)
+ lerr(1, "pledge");
}
event_init();
@@ -969,6 +976,9 @@ validate_access(struct tftp_client *clie
int fd, wmode;
Re: tftpd: add -R for read-only mode/reduced pledges
On Tue, 04 Oct 2022 09:24:04 -, Klemens Nanni wrote: > > we were read-only believers a long time ago, and it seems the world has > > caught up to our way of thinking so yes maybe it is time to make it an > > option you must specify. > > I like the idea, then -c should logically imply -w. Looks great to me. Most tftp usage these days is for reading so read-only by default makes sense. - todd
Re: tftpd: add -R for read-only mode/reduced pledges
On 2022/10/04 10:36, David Gwynne wrote:
> On Sun, Oct 02, 2022 at 06:32:04PM +, Klemens Nanni wrote:
> > diskless(8) just needs tftpd(8) to deliver files, none of the possibly
> > untrusted clients are supposed to ever write anything.
> >
> > Either way, even when run without -c, a single file writable by _tftpd
> > might be enough for a malicious client to fill up the server's disk.
> >
> > A proper read-only mode ("stdio rpath dns inet") seems much safer.
>
> agreed. i'm ok with this diff, but it's worth asking if we can make the
> default read-only and ask people to opt in for write (and create) before
> this specific diff goes in. ie, read-only be default, '-w' to enable
> write mode, '-c' to enable write+create?
yes please!
Re: tftpd: add -R for read-only mode/reduced pledges
On Mon, Oct 03, 2022 at 06:43:26PM -0600, Theo de Raadt wrote:
> David Gwynne wrote:
>
> > On Sun, Oct 02, 2022 at 06:32:04PM +, Klemens Nanni wrote:
> > > diskless(8) just needs tftpd(8) to deliver files, none of the possibly
> > > untrusted clients are supposed to ever write anything.
> > >
> > > Either way, even when run without -c, a single file writable by _tftpd
> > > might be enough for a malicious client to fill up the server's disk.
> > >
> > > A proper read-only mode ("stdio rpath dns inet") seems much safer.
> >
> > agreed. i'm ok with this diff, but it's worth asking if we can make the
> > default read-only and ask people to opt in for write (and create) before
> > this specific diff goes in. ie, read-only be default, '-w' to enable
> > write mode, '-c' to enable write+create?
>
> we were read-only believers a long time ago, and it seems the world has
> caught up to our way of thinking so yes maybe it is time to make it an
> option you must specify.
I like the idea, then -c should logically imply -w.
Feedback? OK?
Index: tftpd.8
===
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.8,v
retrieving revision 1.8
diff -u -p -r1.8 tftpd.8
--- tftpd.8 4 Mar 2019 01:06:03 - 1.8
+++ tftpd.8 4 Oct 2022 09:23:17 -
@@ -37,7 +37,7 @@
.Nd Trivial File Transfer Protocol daemon
.Sh SYNOPSIS
.Nm tftpd
-.Op Fl 46cdiv
+.Op Fl 46cdivw
.Op Fl l Ar address
.Op Fl p Ar port
.Op Fl r Ar socket
@@ -93,6 +93,9 @@ Allow new files to be created;
otherwise uploaded files must already exist.
Files are created with default permissions
allowing anyone to read or write to them.
+.Pp
+This option implies
+.Fl w .
.It Fl d
Do not daemonize.
If this option is specified,
@@ -145,6 +148,8 @@ to
on startup;
the remote host is not expected to pass the directory
as part of the file name to transfer.
+.It Fl w
+Allow files to be written to.
.El
.Sh SEE ALSO
.Xr tftp 1 ,
Index: tftpd.c
===
RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
retrieving revision 1.48
diff -u -p -r1.48 tftpd.c
--- tftpd.c 4 Oct 2022 07:05:28 - 1.48
+++ tftpd.c 4 Oct 2022 09:23:18 -
@@ -283,12 +283,13 @@ __dead void
usage(void)
{
extern char *__progname;
- fprintf(stderr, "usage: %s [-46cdiv] [-l address] [-p port] [-r socket]"
+ fprintf(stderr, "usage: %s [-46cdivw] [-l address] [-p port] [-r
socket]"
" directory\n", __progname);
exit(1);
}
int cancreate = 0;
+int canwrite = 0;
int verbose = 0;
int debug = 0;
int iflag = 0;
@@ -309,7 +310,7 @@ main(int argc, char *argv[])
int family = AF_UNSPEC;
int devnull = -1;
- while ((c = getopt(argc, argv, "46cdil:p:r:v")) != -1) {
+ while ((c = getopt(argc, argv, "46cdil:p:r:vw")) != -1) {
switch (c) {
case '4':
family = AF_INET;
@@ -318,7 +319,7 @@ main(int argc, char *argv[])
family = AF_INET6;
break;
case 'c':
- cancreate = 1;
+ canwrite = cancreate = 1;
break;
case 'd':
verbose = debug = 1;
@@ -342,6 +343,9 @@ main(int argc, char *argv[])
case 'v':
verbose = 1;
break;
+ case 'w':
+ canwrite = 1;
+ break;
default:
usage();
/* NOTREACHED */
@@ -394,9 +398,12 @@ main(int argc, char *argv[])
if (cancreate) {
if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) ==
-1)
lerr(1, "pledge");
- } else {
+ } else if (canwrite) {
if (pledge("stdio rpath wpath fattr dns inet", NULL) == -1)
lerr(1, "pledge");
+ } else {
+ if (pledge("stdio rpath dns inet", NULL) == -1)
+ lerr(1, "pledge");
}
event_init();
@@ -969,6 +976,9 @@ validate_access(struct tftp_client *clie
int fd, wmode;
const char *errstr, *filename;
char rewritten[PATH_MAX];
+
+ if (!canwrite && mode != RRQ)
+ return (EACCESS);
if (strcmp(requested, SEEDPATH) == 0) {
char *buf;
Re: tftpd: add -R for read-only mode/reduced pledges
David Gwynne wrote:
> On Sun, Oct 02, 2022 at 06:32:04PM +, Klemens Nanni wrote:
> > diskless(8) just needs tftpd(8) to deliver files, none of the possibly
> > untrusted clients are supposed to ever write anything.
> >
> > Either way, even when run without -c, a single file writable by _tftpd
> > might be enough for a malicious client to fill up the server's disk.
> >
> > A proper read-only mode ("stdio rpath dns inet") seems much safer.
>
> agreed. i'm ok with this diff, but it's worth asking if we can make the
> default read-only and ask people to opt in for write (and create) before
> this specific diff goes in. ie, read-only be default, '-w' to enable
> write mode, '-c' to enable write+create?
we were read-only believers a long time ago, and it seems the world has
caught up to our way of thinking so yes maybe it is time to make it an
option you must specify.
Re: tftpd: add -R for read-only mode/reduced pledges
On Sun, Oct 02, 2022 at 06:32:04PM +, Klemens Nanni wrote:
> diskless(8) just needs tftpd(8) to deliver files, none of the possibly
> untrusted clients are supposed to ever write anything.
>
> Either way, even when run without -c, a single file writable by _tftpd
> might be enough for a malicious client to fill up the server's disk.
>
> A proper read-only mode ("stdio rpath dns inet") seems much safer.
agreed. i'm ok with this diff, but it's worth asking if we can make the
default read-only and ask people to opt in for write (and create) before
this specific diff goes in. ie, read-only be default, '-w' to enable
write mode, '-c' to enable write+create?
>
> diskless(8) setup and manual testing runs fine with this, clients get
> proper error responses (just like trying to write /etc/random.seed) and
> the server keeps running without any pledge violations:
>
> $ doas ./obj/tfpd -dvR ./_tftpd
>
> $ touch file
> $ echo put file | tftp localhost
> tftp> Error code 2: Access violation
>
> tftpd: 127.0.0.1: write request for 'file'
> tftpd: 127.0.0.1: nak: Access violation
>
>
> tftpd(8) is nicely written such that all file access goes through a
> single validation function, so adding read-only logic seems trivial.
>
> Did I miss anything?
> Feedback? OK?
>
> This applies cleanly to -current but conflicts with the -c/cpath diff
> on tech@, so one needs rebasing after the other lands.
>
> Index: tftpd.8
> ===
> RCS file: /cvs/src/usr.sbin/tftpd/tftpd.8,v
> retrieving revision 1.8
> diff -u -p -r1.8 tftpd.8
> --- tftpd.8 4 Mar 2019 01:06:03 - 1.8
> +++ tftpd.8 1 Oct 2022 17:30:27 -
> @@ -37,7 +37,7 @@
> .Nd Trivial File Transfer Protocol daemon
> .Sh SYNOPSIS
> .Nm tftpd
> -.Op Fl 46cdiv
> +.Op Fl 46cdivR
> .Op Fl l Ar address
> .Op Fl p Ar port
> .Op Fl r Ar socket
> @@ -56,6 +56,8 @@ will allow only publicly readable files
> Files may be written only if they already exist and are publicly writable,
> unless the
> .Fl c
> +or
> +.Fl R
> flag is specified
> .Pq see below .
> Note that this extends the concept of
> @@ -145,6 +147,8 @@ to
> on startup;
> the remote host is not expected to pass the directory
> as part of the file name to transfer.
> +.Fl R
> +Prevent creating or writing to files.
> .El
> .Sh SEE ALSO
> .Xr tftp 1 ,
> Index: tftpd.c
> ===
> RCS file: /cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 tftpd.c
> --- tftpd.c 24 Oct 2021 21:24:19 - 1.47
> +++ tftpd.c 2 Oct 2022 18:12:25 -
> @@ -289,6 +289,7 @@ usage(void)
> }
>
> intcancreate = 0;
> +intreadonly = 0;
> intverbose = 0;
> intdebug = 0;
> intiflag = 0;
> @@ -309,7 +310,7 @@ main(int argc, char *argv[])
> int family = AF_UNSPEC;
> int devnull = -1;
>
> - while ((c = getopt(argc, argv, "46cdil:p:r:v")) != -1) {
> + while ((c = getopt(argc, argv, "46cdil:p:r:vR")) != -1) {
> switch (c) {
> case '4':
> family = AF_INET;
> @@ -342,6 +343,9 @@ main(int argc, char *argv[])
> case 'v':
> verbose = 1;
> break;
> + case 'R':
> + readonly = 1;
> + break;
> default:
> usage();
> /* NOTREACHED */
> @@ -351,6 +355,9 @@ main(int argc, char *argv[])
> argc -= optind;
> argv += optind;
>
> + if (cancreate && readonly)
> + errx(1, "options -c and -R are incompatible");
> +
> if (argc != 1)
> usage();
>
> @@ -391,8 +398,13 @@ main(int argc, char *argv[])
> if (!debug && rdaemon(devnull) == -1)
> err(1, "unable to daemonize");
>
> - if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) == -1)
> - lerr(1, "pledge");
> + if (readonly) {
> + if (pledge("stdio rpath dns inet", NULL) == -1)
> + lerr(1, "pledge");
> + } else {
> + if (pledge("stdio rpath wpath cpath fattr dns inet", NULL) ==
> -1)
> + lerr(1, "pledge");
> + }
>
> event_init();
>
> @@ -964,6 +976,9 @@ validate_access(struct tftp_client *clie
> int fd, wmode;
> const char *errstr, *filename;
> char rewritten[PATH_MAX];
> +
> + if (readonly && mode != RRQ)
> + return (EACCESS);
>
> if (strcmp(requested, SEEDPATH) == 0) {
> char *buf;
>
