Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201123185633.GY534@brightrain.aerifal.cx>
Date: Mon, 23 Nov 2020 13:56:33 -0500
From: Rich Felker <dalias@...c.org>
To: Alexey Izbyshev <izbyshev@...ras.ru>
Cc: musl@...ts.openwall.com
Subject: Re: realpath without procfs -- should be ready for inclusion

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

View attachment "realpath-fixes.diff" of type "text/plain" (888 bytes)

Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.