Hi Theo,
> ?: could be used here:
>
> if (unveil("/", rflag ? "r" : "rwc") == -1)
> err(1, "unveil /");
Right, I find it a bit verbose too.
>> - /* 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.
Well, I'm at least sure about dropping the directory traversal.
The reason I also dropped O_NOFOLLOW is because that alone doesn't cut
it and more code is needed to check every path in between. Should I give
this a try, and is there any existing code I can take a look at?
All changes (except the / unveil) are done to the -d option which was
added around 7 years ago. Original rmt isn't touched and doesn't care
about directory traversal and symlinks at all. I could of course just
use this then, but I really don't want to for obvious reasons...
>> /* 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.
I just thought I was being overly clever here and there's no harm in
doing it. But it's fine for me to just fix the indent and keep removing
extra leading slashes only when stripping a valid directory prefix.
Regards,
Andre
