On Sat, Oct 21 2017, Jan Klemkow <j.klem...@wemelug.de> wrote:
> On Fri, Oct 20, 2017 at 12:04:41PM +0000, Jeremie Courreges-Anglas wrote:
>> On Fri, Oct 20 2017, Sebastien Marie <sema...@online.fr> wrote:
>> > On Thu, Oct 19, 2017 at 08:58:12PM +0200, Jan Klemkow wrote:
>> >> + char nfilename[PATH_MAX];
>> >> +
>> >> + snprintf(nfilename, sizeof nfilename, "%s/%s",
>> >> + getip(&client->ss), filename);
>> >
>> > - filename has PATH_MAX length
>> > - getip(&client->ss) could have NI_MAXHOST length
>>
>> INET6_ADDRSTRLEN since getip() calls getnameinfo(NI_NUMERICHOST), but
>> your point stands.
>>
>> > so nfilename could be larger than PATH_MAX (sizeof nfilename).
>> >
>> > I assume the return of snprintf() need to be checked. if truncation
>> > occured, a warning should be issued and nfilename discarded (just
>> > calling tftp_open(client, filename)) ?
>>
>> I think we should refuse the request if truncated.
>
> done
>
>> >> + if (access(nfilename, R_OK) == 0)
>> >> + tftp_open(client, nfilename);
>> >> + else
>> >> + tftp_open(client, filename);
>> >> + }
>>
>> Here we look up the same file in both the client-specific subdirectory
>> and the default directory. Should we instead look only in the
>> client-specific directory if the latter exist?
>
> Common files should be found in the default directory. But, host
> specific files could be overwritten if they exist in the subdirectory.
I think it would be better to perform those access tests in
validate_access(); logic in a single place, and a less quirky handling
of SEEDPATH. Also the test done should probably depend on the type
(read, write) of the request. Retrying with the default directory may
make sense in read mode, but maybe not in write (and -c, create) mode?
The updated diff below implements such semantics, but in
validate_access(). While here,
- improve diagnostic if both -i and -r are given; usage() doesn't show
the conflict
- also test snprintf return value against -1, as spotted by semarie@
Maybe we should add a mention in the manpage that the client can
"escape" its client-specific directory? (ie /../192.0.2.1/file)
The logic is more complicated but I hope it's for good.
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 22 Oct 2017 21:25:32 -0000
@@ -37,7 +37,7 @@
.Nd DARPA Trivial File Transfer Protocol daemon
.Sh SYNOPSIS
.Nm tftpd
-.Op Fl 46cdv
+.Op Fl 46cdiv
.Op Fl l Ar address
.Op Fl p Ar port
.Op Fl r Ar socket
@@ -100,6 +100,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 -0000 1.39
+++ tftpd.c 22 Oct 2017 21:25:32 -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
