All of lore.kernel.org
 help / color / mirror / Atom feed
From: Max Reitz <mreitz@redhat.com>
To: Miklos Szeredi <mszeredi@redhat.com>
Cc: virtio-fs-list <virtio-fs@redhat.com>
Subject: Re: [Virtio-fs] Securing file handles
Date: Mon, 8 Mar 2021 12:29:29 +0100	[thread overview]
Message-ID: <2d94bc7c-e999-44b1-7813-6269cbfcf54e@redhat.com> (raw)
In-Reply-To: <CAOssrKcY7o_LOabLiy+86LFZ1aQdqq8EOaM=qjRqHKGUqRup6w@mail.gmail.com>

On 08.03.21 10:54, Miklos Szeredi wrote:
> Hi,
> 
> Thanks for the good summary.
> 
> Another aspect is what the file handle will be used for:
> 
>   a) allowing server to close O_PATH descriptors any time because they
> can be reconstructed using the file handle
> 
>   b) allowing NFS export on client, or just name_to_handle_at(2)
> open_by_handle_at(2).
> 
> The requirements are slightly different, since file handles used for
> (a) do not have to persist after a guest reboot (since the VFS cache
> referencing those handles is gone).  While (b) requires persistence
> after a reboot.

I’m not even sure we need file handles in the guest for (a).  For 
example, we could just store the file handle for each node in the table 
virtiofsd keeps.  Or perhaps we don’t even need that, because we can 
always reopen nodes with something like

   openat(self.parent.fd.get(), self.name)

(which may recurse quite a bit).  The problem with that would be “what 
happens when a directory is moved”, but well, what does happen then?  Is 
that change propagated to the guest today?  Or will things break?  If 
so, how will they break?

> Yet another issue is global CAP_DAC_READ_SEARCH required by the server
> for file handle decode.
> 
> Taking this into account, I think the final solution has to be in the
> host kernel.   E.g. it seems okay to allow user namespace owner to
> decode file handles on filesystems it actually owns.

What do you mean by “owns”?  That the process’s user is the owner of the 
FS root?

> That would not
> generally help us, though, since virtiofs will want to export root
> owned fs as well.
> 
> Addition of a MAC header to the file handle by name_to_handle_at(2)
> could solve some or all of the above problems.  The question is where
> the key comes from and what the security implications are.
> 
> A per-process (e.g. associated with task->files, generated by the
> kernel on demand and discarded on process exit) key would suffice to
> replace O_PATH descriptors.  In this case the only difference between
> keeping the O_PATH fd open and
> 
>    name_to_handle_at(opathfd, &handle);
>    close(opathfd);
>    opathfd=open_by_handle_at(&handle);
> 
> would be that the resulting fd might point to a disconnected dentry
> and hence would result in incomplete path information under
> /proc/self/fd/.  Need to think hard about whether this has any
> security implications for unprivileged users.
> 
> Adding key management would solve the other aspects, but would also
> possibly open up holes for accessing arbitrary files, so this would
> need to be done carefully.

I have a bad feeling in my stomach when thinking about adding such 
things to the kernel.

I’d feel better about doing something like I noted in my reply to 
Sergio, i.e. add a helper process that does this for other processes. 
This process could do key management and return file handles with MACs, 
and then allow opening them later, passing FDs back.  Only this process 
would need the capability then.  Would that be worse than adding such a 
thing to the kernel?

> Adding subtree checking to the kernel is also a possibility (i.e.
> limit the opened fd to the subtree of the bind mount).   It would have
> the advantage of not resulting in disconnected dentries, but
> disadvantage of not working if the file was moved across directories.

To me it looks like NFS now considers that route to have been a mistake, 
so I’d have a bad feeling about it...

Max


  reply	other threads:[~2021-03-08 11:29 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-05 16:22 [Virtio-fs] Securing file handles Max Reitz
2021-03-08  9:06 ` Sergio Lopez
2021-03-08 10:52   ` Max Reitz
2021-03-08 14:15     ` Sergio Lopez
2021-03-08 15:01   ` Stefan Hajnoczi
2021-03-08  9:54 ` Miklos Szeredi
2021-03-08 11:29   ` Max Reitz [this message]
2021-03-08 12:30     ` Miklos Szeredi
2021-03-08 13:39       ` Max Reitz
2021-03-08 14:50         ` Miklos Szeredi
2021-03-16 17:28           ` Max Reitz
2021-03-17 13:19             ` Vivek Goyal
2021-03-17 15:13               ` Miklos Szeredi
2021-03-08 22:03   ` Vivek Goyal
2021-03-08 11:44 ` Dr. David Alan Gilbert

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=2d94bc7c-e999-44b1-7813-6269cbfcf54e@redhat.com \
    --to=mreitz@redhat.com \
    --cc=mszeredi@redhat.com \
    --cc=virtio-fs@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.