On Tue, Sep 15, 2015 at 10:06:22PM +0200, Alexander Hall wrote:
> On 09/12/15 09:13, Sebastien Marie wrote:
> > First, some generals remarks:
> >
> > - The debug feature (not documented) defeat the `-r' flag purpose.
>
> How do you mean it defeats it? The purpose of the '-r' flag is to stop
> a user from creating and/or writing to files. Obviously said user may
> not dictate the rmt arguments himself in that case.
>
If the user not dictate the rmt arguments, it would be ok. Else the user
could choose a file to overwrite, and he controls pieces of written
string.
> >
> > I think the code should be either:
> > - enclosed in #ifdef DEBUG (prefered way)
> > - not permitted if `rflag' or `wflag' are setted
>
> I was tempted to rip that undocumented feature out entirely. But that's
> another diff, so I left it as-is. The only regression the diff
> introduces is if you are currently using a debug log filename with a
> leading dash. I won't care about that.
>
agree
> > - Adding tame(2) may be a good way to enforce the policyi, but it should
> > be added later (when other userland tools gains it).
>
> Sure, later.
>
> > Else, just some comments inline.
> >
> > On Thu, Sep 10, 2015 at 12:58:52AM +0200, Alexander Hall wrote:
> >>
> >> Index: rmt.c
> >> ===================================================================
> >> RCS file: /cvs/src/usr.sbin/rmt/rmt.c,v
> >> retrieving revision 1.15
> >> diff -u -p -r1.15 rmt.c
> >> --- rmt.c 16 Jan 2015 06:40:20 -0000 1.15
> >> +++ rmt.c 9 Sep 2015 22:57:41 -0000
>
> >> @@ -93,10 +132,66 @@ top:
> >> getstring(device, sizeof(device));
> >> getstring(mode, sizeof(mode));
> >> DEBUG2("rmtd: O %s %s\n", device, mode);
> >> - tape = open(device, atoi(mode),
> >> - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH);
> >> +
> >> + devp = device;
> >> + f = atoi(mode);
> >
> > atoi(3) is a bit fragile.
>
> Yeah, but the code is full of it already, so I left it consistent and
> potentially for another diff to fix.
agree
> >> + m = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH;
> >> + acc = f & O_ACCMODE;
> >> + if (dir) {
> >> + /* Strip away valid directory prefix. */
> >> + if (strncmp(dir, devp, dirlen) == 0 &&
> >> + (devp[dirlen - 1] == '/' ||
> >> + devp[dirlen] == '/')) {
> >> + devp += dirlen;
> >> + while (*devp == '/')
> >> + devp++;
> >> + }
> >
> > the "strip away valid directory prefix" code is a bit complex. If I
> > understand your purpose, you deal with possible trailing '/' in `dir' in
> > the same code than removing the prefix from `devp'.
>
> Yes, and also potential extra slashes in the attempted filename
> ("rmt -d /some/path" vs the filename "/some/path//filename")
>
> While maybe somewhat complex, do you think it's wrong?
>
currently, you manage extra slashes only on filename:
rmt -d /some/path// vs the filename /some/path/filename
:)
But I don't think it is wrong. And even if it is, the strchr call after
would forbid any '/' in devp.
> > maybe you should:
> >
> > 1. canonalize `dir' (by removing possible trailing '/') (should be
> > done in if (dir) { } block before).
> >
> >>> if (dir[dirlen] == '/') {
> >>> dir[dirlen] = '\0';
> >>> dirlen --;
> >>> }
>
> > 2. remove `dir' prefix from `devp'
> >
> >>> if (strncmp(dir, devp, dirlen) == 0 && devp[dirlen] == '/')
> >>> devp += dirlen + 1;
>
> FWIW, this does not handle multiple slashes in the attempted filename.
right. but it wouldn't be difficult to add :)
>
> >> + } else {
> >> + acc = O_RDONLY;
> >> + }
> >> + /* Create readonly file */
> >> + m = S_IRUSR|S_IRGRP|S_IROTH;
> >> + }
> >> + /* Apply new access mode. */
> >> + f = (f & ~O_ACCMODE) | acc;
> >> +
> >> + tape = open(device, f, m);
> >
> > we have checked if `devp' is safe, so we should use `devp' instead of
> > `device'.
>
> I was thinking it didn't matter much, but yes, we're less likely to be
> bitten by race conditions that way.
>
> That said, this is the only thing I feel compelled to change. If you
> feel strongly otherwise, please convince me.
>
> New diff below. OK?
>
I am OK with the diff. I still have some concern about the "strip away
valid directory prefix" code, but I could do with.
> /Alexander
>
> Index: rmt.8
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rmt/rmt.8,v
> retrieving revision 1.12
> diff -u -p -r1.12 rmt.8
> --- rmt.8 23 Jul 2011 15:40:13 -0000 1.12
> +++ rmt.8 15 Sep 2015 20:03:27 -0000
> @@ -36,18 +36,38 @@
> .Nm rmt
> .Nd remote magtape protocol module
> .Sh SYNOPSIS
> -.Nm rmt
> +.Nm
> +.Op Fl r | w
> +.Op Fl d Ar directory
> .Sh DESCRIPTION
> .Nm
> is a program used by the remote dump and restore programs
> -in manipulating a magnetic tape drive through an interprocess
> -communication connection.
> +through an interprocess communication connection.
> +Traditionally it is used for manipulating a magnetic tape drive but it may
> +be used for regular file access as well.
> .Nm
> is normally started up with an
> .Xr rcmd 3
> or
> .Xr rcmdsh 3
> call.
> +.Pp
> +The options are as follows:
> +.Bl -tag -width Ds
> +.It Fl d Ar directory
> +Confine file access to
> +.Ar directory .
> +Forward slashes in filenames are disallowed and symlinks are not followed.
> +.It Fl r
> +Read-only mode, suitable for use with
> +.Xr rrestore 8 .
> +.It Fl w
> +File write mode, suitable for use with
> +.Xr rdump 8
> +for dumping to regular files.
> +Creates missing files and refuses to open existing ones.
> +The file permission bits are set to readonly.
> +.El
> .Pp
> The
> .Nm
> Index: rmt.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rmt/rmt.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 rmt.c
> --- rmt.c 16 Jan 2015 06:40:20 -0000 1.15
> +++ rmt.c 15 Sep 2015 20:03:27 -0000
> @@ -41,6 +41,7 @@
> #include <unistd.h>
> #include <stdio.h>
> #include <stdlib.h>
> +#include <err.h>
> #include <errno.h>
> #include <string.h>
> #include <limits.h>
> @@ -52,6 +53,7 @@ int maxrecsize = -1;
>
> #define STRSIZE 64
> char device[PATH_MAX];
> +char lastdevice[PATH_MAX] = "";
> char count[STRSIZE], mode[STRSIZE], pos[STRSIZE], op[STRSIZE];
>
> char resp[BUFSIZ];
> @@ -61,9 +63,10 @@ FILE *debug;
> #define DEBUG1(f,a) if (debug) fprintf(debug, f, a)
> #define DEBUG2(f,a1,a2) if (debug) fprintf(debug, f, a1, a2)
>
> -char *checkbuf(char *, int);
> -void getstring(char *, int);
> -void error(int);
> +char *checkbuf(char *, int);
> +void getstring(char *, int);
> +void error(int);
> +__dead void usage(void);
>
> int
> main(int argc, char *argv[])
> @@ -72,14 +75,50 @@ main(int argc, char *argv[])
> int rval;
> char c;
> int n, i, cc;
> + int ch, rflag = 0, wflag = 0;
> + int f, acc;
> + mode_t m;
> + char *dir = NULL;
> + char *devp;
> + size_t dirlen;
> +
> + while ((ch = getopt(argc, argv, "d:rw")) != -1) {
> + switch (ch) {
> + case 'd':
> + dir = optarg;
> + if (*dir != '/')
> + errx(1, "directory must be absolute");
> + break;
> + case 'r':
> + rflag = 1;
> + break;
> + case 'w':
> + wflag = 1;
> + break;
> + default:
> + usage();
> + /* NOTREACHED */
> + }
> + }
> + argc -= optind;
> + argv += optind;
> +
> + if (rflag && wflag)
> + usage();
>
> - argc--, argv++;
> if (argc > 0) {
> debug = fopen(*argv, "w");
> if (debug == 0)
> - exit(1);
> + err(1, "cannot open debug file");
> (void) setbuf(debug, (char *)0);
> }
> +
> + if (dir) {
> + if (chdir(dir) != 0)
> + err(1, "chdir");
> + dirlen = strlen(dir);
> + }
> +
> top:
> errno = 0;
> rval = 0;
> @@ -93,10 +132,66 @@ top:
> getstring(device, sizeof(device));
> getstring(mode, sizeof(mode));
> DEBUG2("rmtd: O %s %s\n", device, mode);
> - tape = open(device, atoi(mode),
> - S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH);
> +
> + devp = device;
> + f = atoi(mode);
> + m = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH;
> + acc = f & O_ACCMODE;
> + if (dir) {
> + /* Strip away valid directory prefix. */
> + if (strncmp(dir, devp, dirlen) == 0 &&
> + (devp[dirlen - 1] == '/' ||
> + devp[dirlen] == '/')) {
> + devp += dirlen;
> + while (*devp == '/')
> + devp++;
> + }
> + /* Don't allow directory traversal. */
> + if (strchr(devp, '/')) {
> + errno = EACCES;
> + goto ioerror;
> + }
> + f |= O_NOFOLLOW;
> + }
> + if (rflag) {
> + /*
> + * Only allow readonly open and ignore file
> + * creation requests.
> + */
> + if (acc != O_RDONLY) {
> + errno = EPERM;
> + goto ioerror;
> + }
> + f &= ~O_CREAT;
> + } else if (wflag) {
> + /*
> + * Require, and force creation of, a nonexistant file,
> + * unless we are reopening the last opened file again,
> + * in which case it is opened read-only.
> + */
> + if (strcmp(devp, lastdevice) != 0) {
> + /*
> + * Disallow read-only open since that would
> + * only result in an empty file.
> + */
> + if (acc == O_RDONLY) {
> + errno = EPERM;
> + goto ioerror;
> + }
> + f |= O_CREAT | O_EXCL;
> + } else {
> + acc = O_RDONLY;
> + }
> + /* Create readonly file */
> + m = S_IRUSR|S_IRGRP|S_IROTH;
> + }
> + /* Apply new access mode. */
> + f = (f & ~O_ACCMODE) | acc;
> +
> + tape = open(devp, f, m);
> if (tape == -1)
> goto ioerror;
> + (void)strlcpy(lastdevice, devp, sizeof(lastdevice));
> goto respond;
>
> case 'C':
> @@ -224,4 +319,14 @@ error(int num)
> DEBUG2("rmtd: E %d (%s)\n", num, strerror(num));
> (void) snprintf(resp, sizeof (resp), "E%d\n%s\n", num, strerror(num));
> (void) write(STDOUT_FILENO, resp, strlen(resp));
> +}
> +
> +__dead void
> +usage(void)
> +{
> + extern char *__progname;
> +
> + (void)fprintf(stderr, "usage: %s [-r | -w] [-d directory]\n",
> + __progname);
> + exit(1);
> }
--
Sebastien Marie
