On Sun, Dec 06, 2020 at 05:59:45PM -0800, Yang Zhong wrote:
> >   err = got_path_skip_common_ancestor(&child,
> >       got_worktree_get_root_path(worktree), abspath_in_worktree);
> >   if (err)
> >         goto done;
> >   if (openat(worktree_fd, child, ...) == -1) {
> >         err = got_error_from_errno2("openat", abspath_in_worktree);
> >         free(child);
> >         goto done;
> >   }
> >   free(child);
> >
> > We can derive relative paths from absolute ones in this way.
> > Any function which has access to struct got_worktree and an absolute path
> > that points inside this worktree should be able to use openat() to open the
> > corresponding file. Provided an fd and worktree root abspath are both stored
> > in struct got_worktree. Similarly for got_repository and the tempdir.
> 
> I definitely see the benefit of this solution. I'm concerned that adding
> this extra step to every open() call will add too much noise, but I'll try
> this solution in a section of code first, to be sure. Likely, some places
> will still make more sense with relative paths (such as in the first patch
> I wrote).

Yes, I agree this approach may not always be the best choice. Your PoC
patch shows that quite a few calls to open() exist, and perhaps not all
of them can be handled the same way. We will have to look at actual code
changes to make an informed decision on a case-by-case basis.

Some code paths, such as the work tree status crawl, already have relative
paths available. In the work tree's case, they are read from the file index
where paths are stored relative to the work tree's root directory.
 
> > (I also hope that capsicum will be OK with multiple path components in the
> > path argument of openat(), like this: openat(open("/one"), "two/three");
> > If not, an fd for every intermediate directory would be required, which
> > would be very awkward to handle.)
> 
> Yes, capsicum is fine with this. It only requires that the path have no ".."s
> in it.

That seems fine, then.

This means paths obtained directly from user input are not safe to use.
But I suppose such input will already be converted to absolute paths by
realpath(3) before capsicum is entered. Converting absolute paths obtained
from realpath(3) to relative ones should ensure that ".." never occurs under
normal conditions.