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