[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201123205259.GZ534@brightrain.aerifal.cx>
Date: Mon, 23 Nov 2020 15:53:00 -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 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
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.
