[<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.
