From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 21120 invoked from network); 23 Nov 2020 03:19:51 -0000 Received: from mother.openwall.net (195.42.179.200) by inbox.vuxu.org with ESMTPUTF8; 23 Nov 2020 03:19:51 -0000 Received: (qmail 19689 invoked by uid 550); 23 Nov 2020 03:19:49 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 19665 invoked from network); 23 Nov 2020 03:19:48 -0000 Date: Sun, 22 Nov 2020 22:19:33 -0500 From: Rich Felker To: Alexey Izbyshev Cc: musl@lists.openwall.com Message-ID: <20201123031932.GS534@brightrain.aerifal.cx> References: <20201122225619.GR534@brightrain.aerifal.cx> <97dd3cf7c69673e5962e9ccd46ea5131@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <97dd3cf7c69673e5962e9ccd46ea5131@ispras.ru> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] realpath without procfs -- should be ready for inclusion 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 > >#include > >#include > >#include > >#include > > > >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. Rich