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?
>
> 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 -0000 1.8
> +++ tftpd.8 1 Oct 2022 17:30:27 -0000
> @@ -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 -0000 1.47
> +++ tftpd.c 2 Oct 2022 18:12:25 -0000
> @@ -289,6 +289,7 @@ usage(void)
> }
>
> int cancreate = 0;
> +int readonly = 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: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;
>
