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
