On Sun, Oct 22, 2017 at 09:32:54PM +0000, Jeremie Courreges-Anglas wrote:
> On Sat, Oct 21 2017, Jan Klemkow <j.klem...@wemelug.de> wrote:
> > On Fri, Oct 20, 2017 at 12:04:41PM +0000, Jeremie Courreges-Anglas wrote:
> >> On Fri, Oct 20 2017, Sebastien Marie <sema...@online.fr> 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.
Thanks,
Jan
Index: tftpd.8
===================================================================
RCS file: /mount/openbsd/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 23 Oct 2017 18:03:41 -0000
@@ -37,7 +37,7 @@
.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
@@ -100,6 +100,19 @@ 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
+.Nm
+tries to rewrite the requested filename with a prefix of
+the client's IP address.
+If the rewritten path exists
+.Nm
+will serve this file.
+If not, it will serve the original filename.
+This option is no security feature.
+Clients are able to escape thier specific directory by
+paths like /../ or by changing their IP address.
+This option can not be combined with
+.Fl r .
.It Fl l Ar address
Listen on the specified address.
By default
@@ -126,6 +139,8 @@ before the TFTP request will continue.
By default
.Nm
does not use filename rewriting.
+This option can not be combined with
+.Fl i .
.It Fl v
Log the client IP, type of request, and filename.
.It Ar directory
Index: tftpd.c
===================================================================
RCS file: /mount/openbsd/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 23 Oct 2017 17:56:03 -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
