Re: tftpd(8): diff for ip path rewrite

[Alexander Bluhm]

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(>ss), requested);
> + if (ret 

Re: tftpd(8): diff for ip path rewrite

[Jan Klemkow]

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(>ss), filename);
> >>> >> >
> >>> >> > - filename has PATH_MAX length
> >>> >> > - getip(>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 

Re: tftpd(8): diff for ip path rewrite

[Jeremie Courreges-Anglas]

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(>ss), filename);
>>> >> >
>>> >> > - filename has PATH_MAX length
>>> >> > - getip(>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
 

Re: tftpd(8): diff for ip path rewrite

[Jeremie Courreges-Anglas]

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(>ss), filename);
>> >> >
>> >> > - filename has PATH_MAX length
>> >> > - getip(>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

Re: tftpd(8): diff for ip path rewrite

[Jan Klemkow]

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(>ss), filename);
> >> >
> >> > - filename has PATH_MAX length
> >> > - getip(>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':
   

Re: tftpd(8): diff for ip path rewrite

[Theo de Raadt]

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

[Jeremie Courreges-Anglas]

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(>ss), filename);
>> >
>> > - filename has PATH_MAX length
>> > - getip(>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 @@ 

Re: tftpd(8): diff for ip path rewrite

[Sebastien Marie]

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(>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(>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

[Jan Klemkow]

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(>ss), filename);
> >
> > - filename has PATH_MAX length
> > - getip(>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(>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

[Jeremie Courreges-Anglas]

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

[Jeremie Courreges-Anglas]

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(>ss), filename);
>
> - filename has PATH_MAX length
> - getip(>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

[Sebastien Marie]

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(>ss), filename);

- filename has PATH_MAX length
- getip(>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

[Stuart Henderson]

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

[Theo de Raadt]

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(>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

[Jan Klemkow]

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(>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

[Jeremie Courreges-Anglas]

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(>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

[Jan Klemkow]

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(>ss), filename);
+   tftp_open(client, nfilename);
+   } else
tftp_open(client, filename);
 
return;