Re: tftpd(8): diff for ip path rewrite
On Wed, Oct 25, 2017 at 06:54:01PM +0200, Jeremie Courreges-Anglas wrote: > New diff after feedback from jmc@ OK bluhm@ > 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 - 1.5 > +++ tftpd.8 25 Oct 2017 16:48:32 - > @@ -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 1678 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 - 1.39 > +++ tftpd.c 24 Oct 2017 17:37:46 - > @@ -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) > intcancreate = 0; > intverbose = 0; > intdebug = 0; > +intiflag = 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
Re: tftpd(8): diff for ip path rewrite
On Wed, Oct 25, 2017 at 04:54:01PM +, Jeremie Courreges-Anglas wrote: > On Tue, Oct 24 2017, Jeremie Courreges-Anglas wrote: > > On Mon, Oct 23 2017, Jan Klemkow wrote: > >> On Sun, Oct 22, 2017 at 09:32:54PM +, Jeremie Courreges-Anglas wrote: > >>> On Sat, Oct 21 2017, Jan Klemkow wrote: > >>> > On Fri, Oct 20, 2017 at 12:04:41PM +, Jeremie Courreges-Anglas > >>> > wrote: > >>> >> On Fri, Oct 20 2017, Sebastien Marie 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 - 1.5 > +++ tftpd.8 25 Oct 2017 16:48:32 - > @@ -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 s
Re: tftpd(8): diff for ip path rewrite
On Tue, Oct 24 2017, Jeremie Courreges-Anglas wrote: > On Mon, Oct 23 2017, Jan Klemkow wrote: >> On Sun, Oct 22, 2017 at 09:32:54PM +, Jeremie Courreges-Anglas wrote: >>> On Sat, Oct 21 2017, Jan Klemkow wrote: >>> > On Fri, Oct 20, 2017 at 12:04:41PM +, Jeremie Courreges-Anglas wrote: >>> >> On Fri, Oct 20 2017, Sebastien Marie 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@ 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 - 1.5 +++ tftpd.8 25 Oct 2017 16:48:32 - @@ -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 daemo
Re: tftpd(8): diff for ip path rewrite
On Mon, Oct 23 2017, Jan Klemkow wrote: > On Sun, Oct 22, 2017 at 09:32:54PM +, Jeremie Courreges-Anglas wrote: >> On Sat, Oct 21 2017, Jan Klemkow wrote: >> > On Fri, Oct 20, 2017 at 12:04:41PM +, Jeremie Courreges-Anglas wrote: >> >> On Fri, Oct 20 2017, Sebastien Marie 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. 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 Feedback & oks welcome. 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 - 1.5 +++ tftpd.8 24 Oct 2017 18:39:15 - @@ -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,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 +If specified, +.Nm +prefixes looks 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. +This option cannot be combined with +.Fl r . +See also +.Sx CAVEATS .It Fl l Ar address Listen on the specified address. By default @@ -126,6 +137,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 +164,10 @@ 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 1678 octets .Pq 32767 blocks . +.Pp +In +.Fl i +mode, no attempt is made to limit the client to its subdirectory. Index: tftpd.c === RCS file: /d/cvs/src/usr.sbi
Re: tftpd(8): diff for ip path rewrite
On Sun, Oct 22, 2017 at 09:32:54PM +, Jeremie Courreges-Anglas wrote: > On Sat, Oct 21 2017, Jan Klemkow wrote: > > On Fri, Oct 20, 2017 at 12:04:41PM +, Jeremie Courreges-Anglas wrote: > >> On Fri, Oct 20 2017, Sebastien Marie 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 - 1.5 +++ tftpd.8 23 Oct 2017 18:03:41 - @@ -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 - 1.39 +++ tftpd.c 23 Oct 2017 17:56:03 - @@ -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
Re: tftpd(8): diff for ip path rewrite
I agree with this more. Also, the previous patch had + if (access(nfilename, R_OK) == 0) + tftp_open(client, nfilename); Which means if the directory is writeable by something else up the server side, you have TOCTOU. Never check if you can open, then open. Just open, and based upon that make a decision. Always convert a path to a fd, then decide. Never check a path, then uhm, check it again.
Re: tftpd(8): diff for ip path rewrite
On Sat, Oct 21 2017, Jan Klemkow wrote: > On Fri, Oct 20, 2017 at 12:04:41PM +, Jeremie Courreges-Anglas wrote: >> On Fri, Oct 20 2017, Sebastien Marie 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. 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 - 1.5 +++ tftpd.8 22 Oct 2017 21:25:32 - @@ -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,16 @@ 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 can not be combined with +.Fl r . .It Fl l Ar address Listen on the specified address. By default @@ -126,6 +136,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: /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 - 1.39 +++ tftpd.c 22 Oct 2017 21:25:32 - @@ -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[])
Re: tftpd(8): diff for ip path rewrite
On Sat, Oct 21, 2017 at 10:10:39PM +0200, Jan Klemkow wrote: > > Common files should be found in the default directory. But, host > specific files could be overwritten if they exist in the subdirectory. > > The diff below should address all comments. > > 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 - 1.39 > +++ tftpd.c 21 Oct 2017 19:49:04 - > @@ -903,7 +911,20 @@ again: > > if (rwmap != NULL) > rewrite_map(client, filename); > - else > + else if (iflag) { > + char nfilename[PATH_MAX]; > + > + if (snprintf(nfilename, sizeof(nfilename), "%s/%s", > + getip(&client->ss), filename) > PATH_MAX - 1) { > + ecode = EBADOP; > + goto error; > + } snprintf(3) could return -1 on error. the snprintf(3) man page document the proper secure idiom as: int ret = snprintf(nfilename, sizeof(nfilename), "%s/%s", getip(&client->ss), filename); if (ret == -1 || ret >= sizeof(nfilename)) { ecode = EBADOP; goto error; } regarding the error EBADOP (Illegal TFTP operation), I am unsure if it is the proper error code for this case. ENOTFOUND (File not found) or EACCESS (Access violation) seems more indicated to me. but I am not familiar with tftpd protocol and expectations regarding these error codes. thanks. -- Sebastien Marie
Re: tftpd(8): diff for ip path rewrite
On Fri, Oct 20, 2017 at 12:04:41PM +, Jeremie Courreges-Anglas wrote: > On Fri, Oct 20 2017, Sebastien Marie 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. The diff below should address all comments. 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 - 1.5 +++ tftpd.8 21 Oct 2017 18:41:15 - @@ -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,16 @@ 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 can not be combined with +.Fl r . .It Fl l Ar address Listen on the specified address. By default @@ -126,6 +136,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 - 1.39 +++ tftpd.c 21 Oct 2017 19:49:04 - @@ -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) + usage(); + 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) + usage(); rewrite = optarg; break; case 'v': @@ -903,7 +911,20 @@ again: if (rwmap != NULL) rewrite_map(client, filename); - else + else if (iflag) { + char nfilename[PATH_MAX]; + + if (snprintf(nfilename, sizeof(nfilename), "%s/%s", + getip(&client->ss), filename) > PATH_MAX - 1) { + ecode = EBADOP; + goto error; + } + + if (access(nfilename, R_OK) == 0) + tftp_open(client, nfilename); +
Re: tftpd(8): diff for ip path rewrite
On Thu, Oct 19 2017, Stuart Henderson wrote: > On 2017/10/19 16:22, Theo de Raadt wrote: >> I am always worried by non-intuitive magic behaviour. >> >> It may serve some obvious purposes, but for someone else it is going >> to break things. >> >> I worry. > > The IP/filename -> filename fallback method seems good enough, but > I agree with Theo. > > I think it would be safer behind a flag rather than on by default. Same here. -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: tftpd(8): diff for ip path rewrite
On Fri, Oct 20 2017, Sebastien Marie wrote: > On Thu, Oct 19, 2017 at 08:58:12PM +0200, Jan Klemkow wrote: >> >> 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 - 1.39 >> +++ tftpd.c 19 Oct 2017 18:27:24 - >> @@ -903,8 +903,17 @@ again: >> >> if (rwmap != NULL) >> rewrite_map(client, filename); >> -else >> -tftp_open(client, filename); >> +else { >> +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. >> + >> +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? >> >> return; >> >> > > thanks -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: tftpd(8): diff for ip path rewrite
On Thu, Oct 19, 2017 at 08:58:12PM +0200, Jan Klemkow wrote: > > 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 - 1.39 > +++ tftpd.c 19 Oct 2017 18:27:24 - > @@ -903,8 +903,17 @@ again: > > if (rwmap != NULL) > rewrite_map(client, filename); > - else > - tftp_open(client, filename); > + else { > + 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 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)) ? > + > + if (access(nfilename, R_OK) == 0) > + tftp_open(client, nfilename); > + else > + tftp_open(client, filename); > + } > > return; > > thanks -- Sebastien Marie
Re: tftpd(8): diff for ip path rewrite
On 2017/10/19 16:22, Theo de Raadt wrote: > I am always worried by non-intuitive magic behaviour. > > It may serve some obvious purposes, but for someone else it is going > to break things. > > I worry. The IP/filename -> filename fallback method seems good enough, but I agree with Theo. I think it would be safer behind a flag rather than on by default.
Re: tftpd(8): diff for ip path rewrite
I am always worried by non-intuitive magic behaviour. It may serve some obvious purposes, but for someone else it is going to break things. I worry. > bluhm@ suggested, that this should be the default behavior. Thus, the > ftpd(8) checks if a subdirectory with the client's ip address exists and > contains the requested file. It not, it uses the original path as > default. I implemented it in this diff: > > 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 - 1.5 > +++ tftpd.8 19 Oct 2017 18:41:07 - > @@ -78,6 +78,14 @@ and therefore this path will be ignored > .Ox > network bootloaders access this path to harvest entropy during > kernel load. > +Also, > +.Nm > +always 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. > .Pp > The options are as follows: > .Bl -tag -width Ds > 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 - 1.39 > +++ tftpd.c 19 Oct 2017 18:27:24 - > @@ -903,8 +903,17 @@ again: > > if (rwmap != NULL) > rewrite_map(client, filename); > - else > - tftp_open(client, filename); > + else { > + char nfilename[PATH_MAX]; > + > + snprintf(nfilename, sizeof nfilename, "%s/%s", > + getip(&client->ss), filename); > + > + if (access(nfilename, R_OK) == 0) > + tftp_open(client, nfilename); > + else > + tftp_open(client, filename); > + } > > return; > >
Re: tftpd(8): diff for ip path rewrite
On Thu, Oct 19, 2017 at 09:36:50AM +, Jeremie Courreges-Anglas wrote: > On Wed, Oct 18 2017, Jan Klemkow wrote: > > On Wed, Oct 18, 2017 at 08:37:48PM +, Jason McIntyre wrote: > >> On Wed, Oct 18, 2017 at 10:25:13PM +0200, Jan Klemkow wrote: > >> > This diff adds an option for client IP address path prefixes to the > >> > tftpd(8). First, I used the -r rewrite socket for this, but... > >> > > >> > If you use the rewrite socket feature, the tftpd(8) will exit with an > >> > error when the rewrite socket is closed. A reopen of the socket is not > >> > possible, if its outside of the chroot directory. And a privilege > >> > separated tftpd(8) is a bit overkill for a stable per client path > >> > rewrite feature. This story led me to this change here. > > I think it makes sense to support this feature without the need for an > additional unix service. > > >> > Any suggestions or objections are welcome. :-) > > Do we want to provide a fallback directory so that you don't need to > restart tftpd without -i to support unknown clients? bluhm@ suggested, that this should be the default behavior. Thus, the ftpd(8) checks if a subdirectory with the client's ip address exists and contains the requested file. It not, it uses the original path as default. I implemented it in this diff: 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 - 1.5 +++ tftpd.8 19 Oct 2017 18:41:07 - @@ -78,6 +78,14 @@ and therefore this path will be ignored .Ox network bootloaders access this path to harvest entropy during kernel load. +Also, +.Nm +always 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. .Pp The options are as follows: .Bl -tag -width Ds 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 - 1.39 +++ tftpd.c 19 Oct 2017 18:27:24 - @@ -903,8 +903,17 @@ again: if (rwmap != NULL) rewrite_map(client, filename); - else - tftp_open(client, filename); + else { + char nfilename[PATH_MAX]; + + snprintf(nfilename, sizeof nfilename, "%s/%s", + getip(&client->ss), filename); + + if (access(nfilename, R_OK) == 0) + tftp_open(client, nfilename); + else + tftp_open(client, filename); + } return;
Re: tftpd(8): diff for ip path rewrite
On Thu, Oct 19, 2017 at 11:36:50AM +0200, Jeremie Courreges-Anglas wrote: > On Wed, Oct 18 2017, Jan Klemkow wrote: > > On Wed, Oct 18, 2017 at 08:37:48PM +, Jason McIntyre wrote: > >> On Wed, Oct 18, 2017 at 10:25:13PM +0200, Jan Klemkow wrote: > >> > This diff adds an option for client IP address path prefixes to the > >> > tftpd(8). First, I used the -r rewrite socket for this, but... > >> > > >> > If you use the rewrite socket feature, the tftpd(8) will exit with an > >> > error when the rewrite socket is closed. A reopen of the socket is not > >> > possible, if its outside of the chroot directory. And a privilege > >> > separated tftpd(8) is a bit overkill for a stable per client path > >> > rewrite feature. This story led me to this change here. > > I think it makes sense to support this feature without the need for an > additional unix service. Guys, wouldn't be a way to re-used httpd lua patterns here? If a rewrite daemon should be trusted, it should listen on < 1024 ports, and wouldn't most people let such rewrite daemon run under root user? Jiri
Re: tftpd(8): diff for ip path rewrite
On Wed, Oct 18 2017, Jan Klemkow wrote: > On Wed, Oct 18, 2017 at 08:37:48PM +, Jason McIntyre wrote: >> On Wed, Oct 18, 2017 at 10:25:13PM +0200, Jan Klemkow wrote: >> > This diff adds an option for client IP address path prefixes to the >> > tftpd(8). First, I used the -r rewrite socket for this, but... >> > >> > If you use the rewrite socket feature, the tftpd(8) will exit with an >> > error when the rewrite socket is closed. A reopen of the socket is not >> > possible, if its outside of the chroot directory. And a privilege >> > separated tftpd(8) is a bit overkill for a stable per client path >> > rewrite feature. This story led me to this change here. I think it makes sense to support this feature without the need for an additional unix service. >> > Any suggestions or objections are welcome. :-) Do we want to provide a fallback directory so that you don't need to restart tftpd without -i to support unknown clients? >> evening. some comments inline: > > Thanks. Fixed diff: > > 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 - 1.5 > +++ tftpd.8 18 Oct 2017 21:12:52 - > @@ -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,11 @@ 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 > +Use the client's IP address as a subdirectory prefix for all requested > +filenames. > +This option can not be combined with > +.Fl r . > .It Fl l Ar address > Listen on the specified address. > By default > @@ -126,6 +131,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 - 1.39 > +++ tftpd.c 18 Oct 2017 21:16:25 - > @@ -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) > intcancreate = 0; > intverbose = 0; > intdebug = 0; > +intiflag = 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) > + usage(); > + iflag = 1; > + break; > case 'l': > addr = optarg; > break; > @@ -328,6 +334,8 @@ main(int argc, char *argv[]) > port = optarg; > break; > case 'r': > + if (iflag) > + usage(); > rewrite = optarg; > break; > case 'v': > @@ -903,7 +911,13 @@ again: > > if (rwmap != NULL) > rewrite_map(client, filename); > - else > + else if (iflag) { > + char nfilename[PATH_MAX]; > + > + snprintf(nfilename, sizeof nfilename, "%s/%s", > + getip(&client->ss), filename); > + tftp_open(client, nfilename); > + } else > tftp_open(client, filename); > > return; > -- jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
Re: tftpd(8): diff for ip path rewrite
On Wed, Oct 18, 2017 at 08:37:48PM +, Jason McIntyre wrote: > On Wed, Oct 18, 2017 at 10:25:13PM +0200, Jan Klemkow wrote: > > This diff adds an option for client IP address path prefixes to the > > tftpd(8). First, I used the -r rewrite socket for this, but... > > > > If you use the rewrite socket feature, the tftpd(8) will exit with an > > error when the rewrite socket is closed. A reopen of the socket is not > > possible, if its outside of the chroot directory. And a privilege > > separated tftpd(8) is a bit overkill for a stable per client path > > rewrite feature. This story led me to this change here. > > > > Any suggestions or objections are welcome. :-) > > evening. some comments inline: Thanks. Fixed diff: 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 - 1.5 +++ tftpd.8 18 Oct 2017 21:12:52 - @@ -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,11 @@ 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 +Use the client's IP address as a subdirectory prefix for all requested +filenames. +This option can not be combined with +.Fl r . .It Fl l Ar address Listen on the specified address. By default @@ -126,6 +131,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 - 1.39 +++ tftpd.c 18 Oct 2017 21:16:25 - @@ -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) + usage(); + iflag = 1; + break; case 'l': addr = optarg; break; @@ -328,6 +334,8 @@ main(int argc, char *argv[]) port = optarg; break; case 'r': + if (iflag) + usage(); rewrite = optarg; break; case 'v': @@ -903,7 +911,13 @@ again: if (rwmap != NULL) rewrite_map(client, filename); - else + else if (iflag) { + char nfilename[PATH_MAX]; + + snprintf(nfilename, sizeof nfilename, "%s/%s", + getip(&client->ss), filename); + tftp_open(client, nfilename); + } else tftp_open(client, filename); return;
tftpd(8): diff for ip path rewrite
Hi, This diff adds an option for client IP address path prefixes to the tftpd(8). First, I used the -r rewrite socket for this, but... If you use the rewrite socket feature, the tftpd(8) will exit with an error when the rewrite socket is closed. A reopen of the socket is not possible, if its outside of the chroot directory. And a privilege separated tftpd(8) is a bit overkill for a stable per client path rewrite feature. This story led me to this change here. Any suggestions or objections are welcome. :-) Bye, 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 - 1.5 +++ tftpd.8 18 Oct 2017 19:58:40 - @@ -40,7 +40,7 @@ .Op Fl 46cdv .Op Fl l Ar address .Op Fl p Ar port -.Op Fl r Ar socket +.Op Fl i | Fl r Ar socket .Ar directory .Sh DESCRIPTION .Nm @@ -113,6 +113,12 @@ listens on the port indicated in the .Ql tftp service description; see .Xr services 5 . +.It Fl i +.Nm +will use the clients IP address as subdirectory prefix for all requested +filenames. +This option can not be combined with +.Fl r . .It Fl r Ar socket Issue filename rewrite requests to the specified UNIX domain socket. .Nm @@ -126,6 +132,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 - 1.39 +++ tftpd.c 18 Oct 2017 19:59:40 - @@ -282,14 +282,15 @@ __dead void usage(void) { extern char *__progname; - fprintf(stderr, "usage: %s [-46cdv] [-l address] [-p port] [-r socket]" - " directory\n", __progname); + fprintf(stderr, "usage: %s [-46cdv] [-l address] [-p port]" + " [-i | -r socket] directory\n", __progname); exit(1); } 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) + usage(); + iflag = 1; + break; case 'l': addr = optarg; break; @@ -328,6 +334,8 @@ main(int argc, char *argv[]) port = optarg; break; case 'r': + if (iflag) + usage(); rewrite = optarg; break; case 'v': @@ -903,7 +911,13 @@ again: if (rwmap != NULL) rewrite_map(client, filename); - else + else if (iflag) { + char nfilename[PATH_MAX]; + + snprintf(nfilename, sizeof nfilename, "%s/%s", + getip(&client->ss), filename); + tftp_open(client, nfilename); + } else tftp_open(client, filename); return;