On Tue, Oct 27, 2020 at 03:00:36PM -0700, Junio C Hamano wrote:
> > + /*
> > + * Now we have a path in "target" that only go down into the tree.
> > + * Disallow any interior "../", like "foo/../bar". These might be
> > + * OK, but we cannot know unless we know whether "foo" is itself a
> > + * symlink. So err on the side of caution.
> > + */
> > + while (*target) {
> > + const char *v;
> > + if (skip_prefix(target, "..", &v) && (!*v || is_dir_sep(*v)))
> > + return 1;
> > + target++;
> > + }
Just reading over the patch again, I suspect this is overly eager to
find ".." that is not bordered by a directory separator on the left-hand
side (e.g., I think it will match "foo../").
> > +int safe_symlink(const char *target, const char *linkpath)
> > +{
> > + if (!allow_external_symlinks &&
> > + symlink_leaves_repo(target, linkpath)) {
> > + errno = EPERM;
> > + return -1;
> > + }
> > +
> > + return symlink(target, linkpath);
> > +}
>
> OK. This is only about blocking creation of new symbolic links that
> goes outside the working tree. It obviously is a good thing to do.
>
> We have some "symlink safety" in various parts of the system [*1*],
> and I wonder if we can somehow consolidate the support to a more
> central place.
Note that it is overly conservative, as described in the comment quoted
at the top of this email. It's possible that other code might be able to
be more accurate because it can see "/foo/" in the middle of a symlink
target and actually _look_ at "foo". Does it exist, is it a symlink
itself, etc.
Whereas here we're taking a purely textual look at the target. We have
to, I think, because we don't know if or when that "foo" will get
updated. And maybe that same restriction applies to other parts of the
system, or maybe not.
At any rate, I don't really have plans to push this particular topic
forward, at least not in the near term. I was mostly sharing it in case
anybody found it useful. If somebody wants to run with it, please be my
guest.
-Peff
