Re: [musl] realpath without procfs -- should be ready for inclusion

[Rich Felker <dalias@libc.org>]

On Mon, Nov 23, 2020 at 01:56:33PM -0500, Rich Felker wrote:
> On Sun, Nov 22, 2020 at 10:19:33PM -0500, Rich Felker wrote:
> > On Mon, Nov 23, 2020 at 05:03:25AM +0300, Alexey Izbyshev wrote:
> > > On 2020-11-23 01:56, Rich Felker wrote:
> > > >I originally considered keeping the procfs based version and only
> > > >using the new one as a fallback, but I discovered there are cases
> > > >(involving chroot, namespaces, etc.) where the answer from procfs is
> > > >wrong and validating it requires basically the same procedure as
> > > >implementing it manually (walking and performing readlink on each path
> > > >component).
> > > >
> > > Pity that the simple and fast procfs-based implementation goes away.
> > > Do you have any specific example of a wrong answer from procfs at
> > > hand, or at least a more specific direction to look at than just
> > > "chroot/namespaces"?
> > 
> > Assuming you even have procfs, the name read from readlink on procfs
> > will be relative to the real root of the mount namespace, not the
> > chroot. If the mount namespace has bind mounts over top of anything,
> > it can also give you a pathname that's no longer valid because
> > something is mounted over it.
> > 
> > There may be other ways this arises too. At the very least, you need
> > to do fstat/stat to match them up like we do now; otherwise you can
> > get wildly wrong results. But even if the stat matches up, it's still
> > possible that the resulting pathname is an absolute pathname outside
> > the chroot or behind the bind mount or whatever, but is also valid but
> > involving symlink traversal in when processed from in the current
> > process context. This means you return a result that does not satisfy
> > the contract to be symlink-free.
> > 
> > There's also a matter I didn't mention that the current code is wrong
> > in an unsafe way on per-O_PATH kernels. Other places we mitigate that
> > by using O_NOFOLLOW and O_NOCTTY to avoid *most* of the possible
> > unwanted side effects if opening an actual file on a kernel that
> > doesn't have O_PATH, but on realpath we specifically can't use
> > O_NOFOLLOW, and this makes it susceptible to tricking root (or any
> > user with read access) into opening device nodes, in ways that might
> > have side effects.
> > 
> > So, there are a lot of bad things about the current implementation.
> > Even the minor mitigations present now for some of them (the stat
> > check) along with the overhead (open/close) makes it questionable
> > whether it's faster for lots of inputs. For deep paths the new one is
> > probably slower, but for typical ones it's not as clear and I didn't
> > measure.
> > 
> > 
> > > 
> > > >#define _GNU_SOURCE
> > > >#include <stdlib.h>
> > > >#include <limits.h>
> > > >#include <errno.h>
> > > >#include <unistd.h>
> > > >#include <string.h>
> > > >
> > > >static inline int at_dotdot(const char *end, size_t len)
> > > >{
> > > >   if (len<2) return 0;
> > > >   if (len>2 && end[-3]!='/') return 0;
> > > >   return end[-1]=='.' && end[-2]=='.';
> > > >}
> > > >
> > > >char *realpath(const char *restrict filename, char *restrict resolved)
> > > >{
> > > >   char stack[PATH_MAX];
> > > >   char buf[resolved ? 1 : PATH_MAX];
> > > >   char *output = resolved ? resolved : buf;
> > > >   size_t p, q, l, cnt=0;
> > > >
> > > >   l = strnlen(filename, sizeof stack + 1);
> > > 
> > > Why + 1?
> > 
> > I was thinking it was to ensure that the largest possible result is
> > sufficient to detect ENAMETOOLONG condition, but even == PATH_MAX is
> > sufficient for that since PATH_MAX is a limit including null
> > termination. So I think the +1 can be removed.
> > 
> > > >   if (!l) {
> > > >       errno = ENOENT;
> > > >       return 0;
> > > >   }
> > > >   if (l >= sizeof stack) goto toolong;
> > > >   p = sizeof stack - l - 1;
> > > >   q = 0;
> > > >   memcpy(stack+p, filename, l+1);
> > > >
> > > >   while (stack[p]) {
> > > >       int up = 0;
> > > >       if (stack[p] == '/') {
> > > >           q=0;
> > > >           output[q++] = '/';
> > > >           p++;
> > > >           /* Initial // is special. */
> > > >           if (stack[p] == '/' && stack[p+1] != '/') {
> > > >               output[q++] = '/';
> > > >           }
> > > >           while (stack[p] == '/') p++;
> > > >           continue;
> > > >       }
> > > >       char *z = __strchrnul(stack+p, '/');
> > > >       l = z-(stack+p);
> > > >       if (l==1 && stack[p]=='.') {
> > > >           p += l;
> > > >           while (stack[p] == '/') p++;
> > > >           continue;
> > > >       }
> > > >       if (at_dotdot(stack+p+l, l)) {
> > > >           if (q && !at_dotdot(output+q, q)) {
> > > >               while(q && output[q-1]!='/') q--;
> > > >               if (q>1 && (q>2 || output[0]!='/')) q--;
> > > >               p += l;
> > > >               while (stack[p] == '/') p++;
> > > >               continue;
> > > >           }
> > > >           up = 1;
> > > >       }
> > > >       if (q && output[q-1] != '/') {
> > > >           if (!p) goto toolong;
> > > >           stack[--p] = '/';
> > > >           l++;
> > > >       }
> > > >       if (q+l >= PATH_MAX) goto toolong;
> > > >       memcpy(output+q, stack+p, l);
> > > >       output[q+l] = 0;
> > > >       p += l;
> > > >       if (up) goto notlink;
> > > >       ssize_t k = readlink(output, stack, p);
> > > >       if (k==-1) {
> > > >           if (errno == EINVAL) {
> > > >notlink:
> > > >               q += l;
> > > >               while (stack[p] == '/') p++;
> > > >               continue;
> > > >           }
> > > >           return 0;
> > > >       }
> > > >       if (k==p) goto toolong;
> > > >       if (++cnt == SYMLOOP_MAX) {
> > > >           errno = ELOOP;
> > > >           return 0;
> > > >       }
> > > >       p -= k;
> > > >       memmove(stack+p, stack, k);
> > > 
> > > This isn't always correct if the symlink resolves to "/" because
> > > stack[p] might be '/', so two slashes will be preserved in the
> > > output. For example, "root/home" resolves to "//home" (where "root"
> > > -> "/").
> > 
> > Thanks for catching that. I propose:
> > 
> > 	if (stack[k-1]=='/') p++;
> > 
> > And that raises the point that k==0 should be handled, even though
> > Linux doesn't let you create such links, since in theory they could
> > come from an existing fs or from non-Linux kernels (see
> > https://lwn.net/Articles/551224/ for coverage of the topic). I propose
> > erroring out with ENOENT in this case right after the k==p check above
> > (since k==0 due to p==0 would bee toolong not ENOENT if it can
> > happen).
> > 
> > > >   output[q] = 0;
> > > >
> > > >   if (output[0] != '/') {
> > > >       if (!getcwd(stack, sizeof stack)) return 0;
> > > >       l = strlen(stack);
> > > >       /* Cancel any initial .. components. */
> > > >       p = 0;
> > > >       while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
> > > 
> > > This doesn't check that output+p+2 is the end of a path element,
> > > which is the prerequisite for at_dotdot(). So, for example, "..."
> > > resolves incorrectly.
> > 
> > Thanks again. I don't see a really good way to reuse at_dotdot here,
> > do you? Probably just [0]=='.' && [1]=='.' && (![2] || [2]=='/') is
> > the cleanest.
> 
> And here are the proposed fixes as a patch. This should be good now.
> Perhaps should add some comments.
> 
> Rich

> --- realpath8.c	2020-11-22 17:52:17.586481571 -0500
> +++ realpath9.c	2020-11-23 13:55:06.808458893 -0500
> @@ -19,7 +19,7 @@
>  	char *output = resolved ? resolved : buf;
>  	size_t p, q, l, cnt=0;
>  
> -	l = strnlen(filename, sizeof stack + 1);
> +	l = strnlen(filename, sizeof stack);
>  	if (!l) {
>  		errno = ENOENT;
>  		return 0;
> @@ -80,11 +80,16 @@
>  			return 0;
>  		}
>  		if (k==p) goto toolong;
> +		if (!k) {
> +			errno = ENOENT;
> +			return 0;
> +		}
>  		if (++cnt == SYMLOOP_MAX) {
>  			errno = ELOOP;
>  			return 0;
>  		}
>  		p -= k;
> +		if (stack[k-1]=='/') p++;
>  		memmove(stack+p, stack, k);

This is wrong and needs further consideration.

>  	}
>  
> @@ -95,7 +100,8 @@
>  		l = strlen(stack);
>  		/* Cancel any initial .. components. */
>  		p = 0;
> -		while (q-p>=2 && at_dotdot(output+p+2, p+2)) {
> +		while (output[p]=='.' && output[p+1]=='.'
> +		  && (!output[p+2] || output[p+2]=='/')) {
>  			while(l>1 && stack[l-1]!='/') l--;
>  			if (l>1) l--;
>  			p += 2;

OK, I have a better improvement for this: counting the number of
levels of .. as they're built at the head of output. Then it's just
while (nup--) here, and the condition for canceling .. in the first
loop no longer needs any string inspection; it's just (q>3*nup).

Rich