Call for testers of restricted rmt(8)

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Call for testers of restricted rmt(8)

Alexander Hall
I'm going wide with this diff I've been pushing for quite some time now.

Is *anyone* but me using rdump(8) + rmt(8)?

*If you are currently using rdump/rrestore + rmt, I urge you to test
this diff to make sure it causes no regression. It shouldn't, but
you've been warned.

So, anyway, this diff allows running a restricted rmt(8), in my case
for remote dumps over ssh, a.k.a rdump(8).

For restricting rmt(8) when dumping/restoring to/from a remote machine:

  -d <directory>   confines rmt to operate within a single directory.
  -r               enforces read-only mode.
  -w               enforces write-only mode.

With this, rmt could be used with the following (simplified)
.ssh/authorized_keys entries

  command="/etc/rmt -wd /dumps/host/foo" ssh-ed25519 ...dumpkey...
  command="/etc/rmt -rd /dumps/host/foo" ssh-ed25519 ...restorekey...

This has the major advantage that a remote user cannot ever destroy or
manipulate former backups. A bit more detail is in the man page.

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 9 Sep 2015 22:57:41 -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 9 Sep 2015 22:57:41 -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(device, 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);
 }

Reply | Threaded
Open this post in threaded view
|

Re: Call for testers of restricted rmt(8)

Sebastien Marie-2
On Thu, Sep 10, 2015 at 12:58:52AM +0200, Alexander Hall wrote:
> I'm going wide with this diff I've been pushing for quite some time now.
>
> Is *anyone* but me using rdump(8) + rmt(8)?

I use dump(8) for doing remote backup, but I don't use rmt(8), due to
plaintext storage on remote side.

> *If you are currently using rdump/rrestore + rmt, I urge you to test
> this diff to make sure it causes no regression. It shouldn't, but
> you've been warned.
>
> So, anyway, this diff allows running a restricted rmt(8), in my case
> for remote dumps over ssh, a.k.a rdump(8).
>
> For restricting rmt(8) when dumping/restoring to/from a remote machine:
>
>   -d <directory>   confines rmt to operate within a single directory.
>   -r               enforces read-only mode.
>   -w               enforces write-only mode.
>
> With this, rmt could be used with the following (simplified)
> .ssh/authorized_keys entries
>
>   command="/etc/rmt -wd /dumps/host/foo" ssh-ed25519 ...dumpkey...
>   command="/etc/rmt -rd /dumps/host/foo" ssh-ed25519 ...restorekey...
>
> This has the major advantage that a remote user cannot ever destroy or
> manipulate former backups. A bit more detail is in the man page.
>
> OK?
>

I will try to get time soon to review it a bit.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: Call for testers of restricted rmt(8)

Alexander Hall


On September 11, 2015 6:27:26 AM GMT+02:00, Sebastien Marie <[hidden email]> wrote:
>On Thu, Sep 10, 2015 at 12:58:52AM +0200, Alexander Hall wrote:
>> I'm going wide with this diff I've been pushing for quite some time
>now.
>>
>> Is *anyone* but me using rdump(8) + rmt(8)?
>
>I use dump(8) for doing remote backup, but I don't use rmt(8), due to
>plaintext storage on remote side.

I don't understand. What's "plaintext storage"? :-)

>
>> *If you are currently using rdump/rrestore + rmt, I urge you to test
>> this diff to make sure it causes no regression. It shouldn't, but
>> you've been warned.
>>
>> So, anyway, this diff allows running a restricted rmt(8), in my case
>> for remote dumps over ssh, a.k.a rdump(8).
>>
>> For restricting rmt(8) when dumping/restoring to/from a remote
>machine:
>>
>>   -d <directory>   confines rmt to operate within a single directory.
>>   -r               enforces read-only mode.
>>   -w               enforces write-only mode.
>>
>> With this, rmt could be used with the following (simplified)
>> .ssh/authorized_keys entries
>>
>>   command="/etc/rmt -wd /dumps/host/foo" ssh-ed25519 ...dumpkey...
>>   command="/etc/rmt -rd /dumps/host/foo" ssh-ed25519 ...restorekey...
>>
>> This has the major advantage that a remote user cannot ever destroy
>or
>> manipulate former backups. A bit more detail is in the man page.
>>
>> OK?
>>
>
>I will try to get time soon to review it a bit.

Nice. I'll hold back a bit, but theo already pretty much told me to commit it as I'm probably pretty much the last person on earth using rmt. :-)

/Alexander

Reply | Threaded
Open this post in threaded view
|

Re: Call for testers of restricted rmt(8)

Sebastien Marie-2
On Fri, Sep 11, 2015 at 05:03:54PM +0200, Alexander Hall wrote:
>
> >>
> >> Is *anyone* but me using rdump(8) + rmt(8)?
> >
> >I use dump(8) for doing remote backup, but I don't use rmt(8), due to
> >plaintext storage on remote side.
>
> I don't understand. What's "plaintext storage"? :-)
>

when using dump(8) with rmt(8), the remote connection is done using ssh.
So the network traffic is encrypted. But rmt will save the dump "as it"
on remote side, so without encryption.

it is what I mean by "plaintext storage".

my concerns is simple:
  - I am the unique admin on my laptop.
  - I want to save dumps of it, on a remote location where I am not admin.
  - I don't want my data to be readable by third-party on the remote side.

Currently, I use the (somehow simplified) following command:
dump -f- | gzip -1 | openssl enc | ssh 'cat > dumpfile'

which ensure the `dumpfile' is encrypted on local side, and saved
(encrypted) on the remote side.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: Call for testers of restricted rmt(8)

Sebastien Marie-2
In reply to this post by Alexander Hall
First, some generals remarks:

- The debug feature (not documented) defeat the `-r' flag purpose.

  I think the code should be either:
    - enclosed in #ifdef DEBUG (prefered way)
    - not permitted if `rflag' or `wflag' are setted

- Adding tame(2) may be a good way to enforce the policyi, but it should
  be added later (when other userland tools gains it).


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
> @@ -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);

atoi(3) is a bit fragile.

> + 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'.

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;

> + /* 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;
> + }

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

> + } 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'.

>   if (tape == -1)
>   goto ioerror;
> + (void)strlcpy(lastdevice, devp, sizeof(lastdevice));
>   goto respond;
>  
>   case 'C':

--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: Call for testers of restricted rmt(8)

Alexander Hall
In reply to this post by Sebastien Marie-2


On 09/11/15 19:33, Sebastien Marie wrote:

> On Fri, Sep 11, 2015 at 05:03:54PM +0200, Alexander Hall wrote:
>>
>>>>
>>>> Is *anyone* but me using rdump(8) + rmt(8)?
>>>
>>> I use dump(8) for doing remote backup, but I don't use rmt(8), due to
>>> plaintext storage on remote side.
>>
>> I don't understand. What's "plaintext storage"? :-)
>>
>
> when using dump(8) with rmt(8), the remote connection is done using ssh.
> So the network traffic is encrypted. But rmt will save the dump "as it"
> on remote side, so without encryption.
>
> it is what I mean by "plaintext storage".
>
> my concerns is simple:
>    - I am the unique admin on my laptop.
>    - I want to save dumps of it, on a remote location where I am not admin.
>    - I don't want my data to be readable by third-party on the remote side.
>
> Currently, I use the (somehow simplified) following command:
> dump -f- | gzip -1 | openssl enc | ssh 'cat > dumpfile'
>
> which ensure the `dumpfile' is encrypted on local side, and saved
> (encrypted) on the remote side.

Yeah, those are valid concerns, and while I think that's common
practice, be warned that if Murphy is around and you loose the tail of
the piped data, dump(8) might still consider this a complete dump once
it has written all its data to the pipe, causing inconsistency between
actually dumped data and /etc/dumpdates...

I've spent some time skimming the code and wondering if it would improve
the situation if we allowed -f "| /path/to/some_command" or -p 'gzip |
openssl enc'.

Ideas about how and if this would be reasonable (or not) are gladly
accepted.

/Alexander

Reply | Threaded
Open this post in threaded view
|

Re: Call for testers of restricted rmt(8)

Alexander Hall
In reply to this post by Sebastien Marie-2
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);
 }

Reply | Threaded
Open this post in threaded view
|

Re: Call for testers of restricted rmt(8)

Sebastien Marie-2
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