Andre Stoebe <[email protected]> wrote:
> So I added unveil and removed the limitation on forward slashes as well
> as symbolic links when using the -d option. I believe (please correct me
> if I'm wrong) these limitations were added to prevent breaking out of
> the directory while keeping the code simple. With unveil this isn't
> needed anymore.
Yes... but maybe no.
> --- usr.sbin/rmt/rmt.c
> +++ usr.sbin/rmt/rmt.c
> @@ -83,7 +83,7 @@ main(int argc, char *argv[])
> char *devp;
> size_t dirlen;
>
> - if (pledge("stdio rpath wpath cpath inet", NULL) == -1)
> + if (pledge("stdio rpath wpath cpath inet unveil", NULL) == -1)
> err(1, "pledge");
>
> while ((ch = getopt(argc, argv, "d:rw")) != -1) {
> @@ -118,10 +118,27 @@ main(int argc, char *argv[])
> }
>
> if (dir) {
> + if (rflag) {
> + if (unveil(dir, "r") == -1)
> + err(1, "unveil %s", dir);
> + } else {
> + if (unveil(dir, "rwc") == -1)
> + err(1, "unveil %s", dir);
> + }
> if (chdir(dir) != 0)
> err(1, "chdir");
> dirlen = strlen(dir);
> + } else {
> + if (rflag) {
> + if (unveil("/", "r") == -1)
> + err(1, "unveil /");
> + } else {
> + if (unveil("/", "rwc") == -1)
> + err(1, "unveil /");
> + }
?: could be used here:
if (unveil("/", rflag ? "r" : "rwc") == -1)
err(1, "unveil /");
> - /* Don't allow directory traversal. */
> - if (strchr(devp, '/')) {
> - errno = EACCES;
> - goto ioerror;
> - }
> - f |= O_NOFOLLOW;
Are you sure about these two changes? The directory stored into on the
remote end can also be manipulated by another process on that machine, which
could change the layout on the fly, including placing symbolic links. The
unveil won't allow traversal outside the top-level directory, but placement
inside
the directory could still become confused. rmt has been super-strict for those
cases for about 20 years without any downside discovered, but makes it less
strict.
> /* Strip away valid directory prefix. */
> if (strncmp(dir, devp, dirlen) == 0 &&
> - (devp[dirlen - 1] == '/' ||
> - devp[dirlen] == '/')) {
> - devp += dirlen;
> - while (*devp == '/')
> + (devp[dirlen - 1] == '/' || devp[dirlen] == '/'))
> + devp += dirlen;
> + while (*devp == '/')
> devp++;
> - }
Same question here.
