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.
>
> 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.
> - 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.
>> + 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?
> 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.
>> + } 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;
>> + }
>
> does when using -w, O_RDWR should be permitted ? I would expect
> O_WRONLY to be the only allowed mode.
>
>> + f |= O_CREAT | O_EXCL;
>
> anyway, O_CREAT | O_EXCL would mitigate the use of O_RDWR for reading
> existing file...
Yeah, I considered that being fine. -r and -w is not strictly read-only
and write-only in every low-level sense, but at a slightly higher level.
>> + } 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?
/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);
}
