On Wed, Oct 25, 2017 at 04:54:01PM +0000, Jeremie Courreges-Anglas wrote:
> On Tue, Oct 24 2017, Jeremie Courreges-Anglas <[email protected]> wrote:
> > On Mon, Oct 23 2017, Jan Klemkow <[email protected]> wrote:
> >> On Sun, Oct 22, 2017 at 09:32:54PM +0000, Jeremie Courreges-Anglas wrote:
> >>> On Sat, Oct 21 2017, Jan Klemkow <[email protected]> wrote:
> >>> > On Fri, Oct 20, 2017 at 12:04:41PM +0000, Jeremie Courreges-Anglas
> >>> > wrote:
> >>> >> On Fri, Oct 20 2017, Sebastien Marie <[email protected]> wrote:
> >>> >> > On Thu, Oct 19, 2017 at 08:58:12PM +0200, Jan Klemkow wrote:
> >>> >> >> + char nfilename[PATH_MAX];
> >>> >> >> +
> >>> >> >> + snprintf(nfilename, sizeof nfilename, "%s/%s",
> >>> >> >> + getip(&client->ss), filename);
> >>> >> >
> >>> >> > - filename has PATH_MAX length
> >>> >> > - getip(&client->ss) could have NI_MAXHOST length
> >>> >>
> >>> >> INET6_ADDRSTRLEN since getip() calls getnameinfo(NI_NUMERICHOST), but
> >>> >> your point stands.
> >>> >>
> >>> >> > so nfilename could be larger than PATH_MAX (sizeof nfilename).
> >>> >> >
> >>> >> > I assume the return of snprintf() need to be checked. if truncation
> >>> >> > occured, a warning should be issued and nfilename discarded (just
> >>> >> > calling tftp_open(client, filename)) ?
> >>> >>
> >>> >> I think we should refuse the request if truncated.
> >>> >
> >>> > done
> >>> >
> >>> >> >> + if (access(nfilename, R_OK) == 0)
> >>> >> >> + tftp_open(client, nfilename);
> >>> >> >> + else
> >>> >> >> + tftp_open(client, filename);
> >>> >> >> + }
> >>> >>
> >>> >> Here we look up the same file in both the client-specific subdirectory
> >>> >> and the default directory. Should we instead look only in the
> >>> >> client-specific directory if the latter exist?
> >>> >
> >>> > Common files should be found in the default directory. But, host
> >>> > specific files could be overwritten if they exist in the subdirectory.
> >>>
> >>> I think it would be better to perform those access tests in
> >>> validate_access(); logic in a single place, and a less quirky handling
> >>> of SEEDPATH. Also the test done should probably depend on the type
> >>> (read, write) of the request. Retrying with the default directory may
> >>> make sense in read mode, but maybe not in write (and -c, create) mode?
> >>>
> >>> The updated diff below implements such semantics, but in
> >>> validate_access(). While here,
> >>> - improve diagnostic if both -i and -r are given; usage() doesn't show
> >>> the conflict
> >>> - also test snprintf return value against -1, as spotted by semarie@
> >>>
> >>> Maybe we should add a mention in the manpage that the client can
> >>> "escape" its client-specific directory? (ie /../192.0.2.1/file)
> >>>
> >>> The logic is more complicated but I hope it's for good.
> >>
> >> I successfully testes jca's diff in my setup and add two lines about
> >> directory escaping to the manpage.
> >
> > I don't think there is a need to expand on security and machines
> > changing their IP address, especially when you're using TFTP, an
> > insecure protocol. I just wanted to stress that no enforcement was
> > done.
> >
> > Here's an alternate take at documenting -i, addressing a few issues. It
> > moves the "no path enforcement" sentence to CAVEATS. I hope you agree
> > with this move.
>
> At least jmc@ thinks that the -i flag description is a better place.
>
> > While here:
> > - kill .Tn
> > - the content of the previous BUGS section doesn't look like a TFTP bug,
> > so CAVEATS looks more appropriate to me
>
> I've kept those changes (to be committed seperately).
>
> > Feedback & oks welcome.
>
> New diff after feedback from jmc@
I tested this diff. It looks fine for me.
Thanks,
Jan
> Index: tftpd.8
> ===================================================================
> RCS file: /d/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 25 Oct 2017 16:48:32 -0000
> @@ -37,16 +37,14 @@
> .Nd DARPA Trivial File Transfer Protocol daemon
> .Sh SYNOPSIS
> .Nm tftpd
> -.Op Fl 46cdv
> +.Op Fl 46cdiv
> .Op Fl l Ar address
> .Op Fl p Ar port
> .Op Fl r Ar socket
> .Ar directory
> .Sh DESCRIPTION
> .Nm
> -is a server which implements the
> -.Tn DARPA
> -Trivial File Transfer Protocol.
> +is a server which implements the DARPA Trivial File Transfer Protocol.
> .Pp
> The use of
> .Xr tftp 1
> @@ -100,6 +98,15 @@ If this option is specified,
> .Nm
> will run in the foreground and log
> the client IP, type of request, and filename to stderr.
> +.It Fl i
> +Look up the requested path in the subdirectory named after the
> +client's IP address.
> +For read requests, if the file is not found,
> +.Nm
> +falls back on the requested path.
> +Note that no attempt is made to limit the client to its subdirectory.
> +This option cannot be combined with
> +.Fl r .
> .It Fl l Ar address
> Listen on the specified address.
> By default
> @@ -126,6 +133,8 @@ before the TFTP request will continue.
> By default
> .Nm
> does not use filename rewriting.
> +This option cannot be combined with
> +.Fl i .
> .It Fl v
> Log the client IP, type of request, and filename.
> .It Ar directory
> @@ -151,6 +160,6 @@ and appeared in
> It was rewritten for
> .Ox 5.2
> as a persistent non-blocking daemon.
> -.Sh BUGS
> +.Sh CAVEATS
> Many TFTP clients will not transfer files over 16744448 octets
> .Pq 32767 blocks .
> Index: tftpd.c
> ===================================================================
> RCS file: /d/cvs/src/usr.sbin/tftpd/tftpd.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 tftpd.c
> --- tftpd.c 26 May 2017 17:38:46 -0000 1.39
> +++ tftpd.c 24 Oct 2017 17:37:46 -0000
> @@ -282,7 +282,7 @@ __dead void
> usage(void)
> {
> extern char *__progname;
> - fprintf(stderr, "usage: %s [-46cdv] [-l address] [-p port] [-r socket]"
> + fprintf(stderr, "usage: %s [-46cdiv] [-l address] [-p port] [-r socket]"
> " directory\n", __progname);
> exit(1);
> }
> @@ -290,6 +290,7 @@ usage(void)
> int cancreate = 0;
> int verbose = 0;
> int debug = 0;
> +int iflag = 0;
>
> int
> main(int argc, char *argv[])
> @@ -307,7 +308,7 @@ main(int argc, char *argv[])
> int family = AF_UNSPEC;
> int devnull = -1;
>
> - while ((c = getopt(argc, argv, "46cdl:p:r:v")) != -1) {
> + while ((c = getopt(argc, argv, "46cdil:p:r:v")) != -1) {
> switch (c) {
> case '4':
> family = AF_INET;
> @@ -321,6 +322,11 @@ main(int argc, char *argv[])
> case 'd':
> verbose = debug = 1;
> break;
> + case 'i':
> + if (rewrite != NULL)
> + errx(1, "options -i and -r are incompatible");
> + iflag = 1;
> + break;
> case 'l':
> addr = optarg;
> break;
> @@ -328,6 +334,8 @@ main(int argc, char *argv[])
> port = optarg;
> break;
> case 'r':
> + if (iflag == 1)
> + errx(1, "options -i and -r are incompatible");
> rewrite = optarg;
> break;
> case 'v':
> @@ -949,15 +957,16 @@ error:
> * given as we have no login directory.
> */
> int
> -validate_access(struct tftp_client *client, const char *filename)
> +validate_access(struct tftp_client *client, const char *requested)
> {
> int mode = client->opcode;
> struct opt_client *options = client->options;
> struct stat stbuf;
> int fd, wmode;
> - const char *errstr;
> + const char *errstr, *filename;
> + char rewritten[PATH_MAX];
>
> - if (strcmp(filename, SEEDPATH) == 0) {
> + if (strcmp(requested, SEEDPATH) == 0) {
> char *buf;
> if (mode != RRQ)
> return (EACCESS);
> @@ -971,15 +980,39 @@ validate_access(struct tftp_client *clie
> return (0);
> }
>
> + if (iflag) {
> + int ret;
> +
> + /*
> + * In -i mode, look in the directory named after the
> + * client address.
> + */
> + ret = snprintf(rewritten, sizeof(rewritten), "%s/%s",
> + getip(&client->ss), requested);
> + if (ret == -1 || ret >= sizeof(rewritten))
> + return (ENAMETOOLONG + 100);
> + filename = rewritten;
> + } else {
> +retryread:
> + filename = requested;
> + }
> +
> /*
> * We use a different permissions scheme if `cancreate' is
> * set.
> */
> wmode = O_TRUNC;
> if (stat(filename, &stbuf) < 0) {
> - if (!cancreate)
> + if (!cancreate) {
> + /*
> + * In -i mode, retry failed read requests from
> + * the root directory.
> + */
> + if (mode == RRQ && errno == ENOENT &&
> + filename == rewritten)
> + goto retryread;
> return (errno == ENOENT ? ENOTFOUND : EACCESS);
> - else {
> + } else {
> if ((errno == ENOENT) && (mode != RRQ))
> wmode |= O_CREAT;
> else
>
>
> --
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
