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

[Alexey Izbyshev <izbyshev@ispras.ru>]

On 2020-11-23 06:19, 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.

Probably I misunderstand what you mean, but when I tried a 
straightforward test on a 5.4-based kernel (with procfs-based musl, of 
course), I failed:

$ mkdir -p chr/proc
$ cat test.c
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
   puts(realpath(argv[1], NULL));
}
$ musl-gcc -static -std=gnu99 test.c -o chr/a.out
$ unshare -Urm
# mount --rbind /proc chr/proc
# chroot chr ./a.out ./a.out
/a.out

> 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.
> 
I see how this can be the case if a bind mount was created on top of 
something *after* musl opened the fd in realpath(), but in presence of 
concurrent FS modifications the new implementation can return a stale 
result too.

> 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.
> 
If a path that is not meaningful in the context of the current process 
can be returned by the kernel in absence of concurrent FS modifications, 
it is disturbing. The kernel code for open() does more-or-less the same 
as the new realpath() implementation, and it's not clear to me what goes 
wrong when the kernel walks the parent chain back to rebuild the path 
(and AFAIR, the kernel does remember the chain of mount points that it 
traversed while resolving a path, and it certainly does account for 
chroot).

> 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.
> 
Ugh, yes, pre-O_PATH kernels are a problem. Apart from what you 
described, you can't open a non-readable file in a readable directory 
without O_PATH, and you'll have trouble with Unix domain sockets as 
well. But at least this class of problems will go away with time.

> 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.
> 
Thanks for the detailed response. While I was aware about problems with 
procfs-based path resolution for long-lived fds (i.e. when FS tree 
changes after the file was opened), I'm a bit baffled that, according to 
your description, things can go wrong even in realpath() use case.

Alexey