On Tue, Oct 04, 2022 at 09:43:40AM +0000, Klemens Nanni wrote:
> On Tue, Oct 04, 2022 at 09:24:04AM +0000, Klemens Nanni wrote:
> > On Mon, Oct 03, 2022 at 06:43:26PM -0600, Theo de Raadt wrote:
> > > David Gwynne <da...@gwynne.id.au> wrote:
> > >
> > > > On Sun, Oct 02, 2022 at 06:32:04PM +0000, 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 -0000 1.8
> +++ tftpd.8 4 Oct 2022 09:42:16 -0000
> @@ -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 -0000 1.48
> +++ tftpd.c 4 Oct 2022 09:42:26 -0000
> @@ -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;
