Re: [PATCH] fs/open: add new RESOLVE_EMPTY_PATH flag for openat2

[Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com>]

On 1/12/22 17:51, Christian Brauner wrote:
> On Thu, Jan 13, 2022 at 01:34:19AM +1100, Aleksa Sarai wrote:
>> On 2022-01-12, Andrey Zhadchenko <andrey.zhadchenko@virtuozzo.com> wrote:
>>> If you have an opened O_PATH file, currently there is no way to re-open
>>> it with other flags with openat/openat2. As a workaround it is possible
>>> to open it via /proc/self/fd/<X>, however
>>> 1) You need to ensure that /proc exists
>>> 2) You cannot use O_NOFOLLOW flag
>>
>> There is also another issue -- you can mount on top of magic-links so if
>> you're a container runtime that has been tricked into creating bad
>> mounts of top of /proc/ subdirectories there's no way of detecting that
>> this has happened. (Though I think in the long-term we will need to
>> make it possible for unprivileged users to create a procfs mountfd if
>> they have hidepid=4,subset=pids set -- there are loads of things
>> containers need to touch in procfs which can be overmounted in malicious
>> ways.)
> 
> Yeah, though I see this as a less pressing issue for now. I'd rather
> postpone this and make userspace work a bit more. There are ways to
> design programs so you know that the procfs instance you're interacting
> with is the one you want to interact with without requiring unprivileged
> mounting outside of a userns+pidns+mountns pair. ;)
> 
>>
>>> Both problems may look insignificant, but they are sensitive for CRIU.
>>> First of all, procfs may not be mounted in the namespace where we are
>>> restoring the process. Secondly, if someone opens a file with O_NOFOLLOW
>>> flag, it is exposed in /proc/pid/fdinfo/<X>. So CRIU must also open the
>>> file with this flag during restore.
>>>
>>> This patch adds new constant RESOLVE_EMPTY_PATH for resolve field of
>>> struct open_how and changes getname() call to getname_flags() to avoid
>>> ENOENT for empty filenames.
>>
>> This is something I've wanted to implement for a while, but from memory
>> we need to add some other protections in place before enabling this.
>>
>> The main one is disallowing re-opening of a path when it was originally
>> opened with a different set of modes. [1] is the patch I originally
I looked at this patch. However I am not able to reproduce the problem.
For example, I can't open /proc/self/exe as RDWR with the following:
fd1 = open(/proc/self/exe, O_PATH)
fd2 = open(/proc/self/fd/3, O_RDWR) <- error
or open file with incorrect flags via O_PATH to O_PATH fd from proc
This is fixed or did I understand this problem wrong?
>> wrote as part of the openat2(2) (but I dropped it since I wasn't sure
>> whether it might break some systems in subtle ways -- though according
>> to my testing there wasn't an issue on any of my machines).
> 
> Oh this is the discussion we had around turning an opath fd into a say
> O_RDWR fd, I think.
> So yes, I think restricting fd reopening makes sense. However, going
> from an O_PATH fd to e.g. an fd with O_RDWR does make sense and needs to
> be the default anyway. So we would need to implement this as a denylist
> anyway. The default is that opath fds can be reopened with whatever and
> only if the opath creator has restricted reopening will it fail, i.e.
> it's similar to a denylist.
> 
> But this patch wouldn't prevent that or hinder the upgrade mask
> restriction afaict.

This issue is actually more complicated than I thought.

What do you think of the following:
1. Add new O_EMPTYPATH constant
2. When we open something with O_PATH, remember access flags (currently
we drop all flags in do_dentry_open() for O_PATH fds). This is similar
to Aleksa Sarai idea, but I do not think we should add some new fields,
because CRIU needs to be able to see it. Just leave access flags
untouched.
3. for openat(fd, "", O_EMPTYPATH | <access flags>) additionally check
access flags against the ones we remembered for O_PATH fd

This won't solve magiclinks problems but there at least will be API to
avoid procfs and which allow to add some restrictions.