[RFC,v1,0/6] proc: Add allowlist for procfs files

  The patch expands subset= option. If the proc is mounted with the
subset=allowlist option, the /proc/allowlist file will appear. This file
contains the filenames and directories that are allowed for this
mountpoint. By default, /proc/allowlist contains only its own name.
Changing the allowlist is possible as long as it is present in the
allowlist itself.

This allowlist is applied in lookup/readdir so files that will create
modules after mounting will not be visible.

Compared to the previous patches [1][2], I switched to a special virtual
file from listing filenames in the mount options.

[1] https://lore.kernel.org/lkml/20200604200413.587896-1-gladkov.alexey@gmail.com/
[2] https://lore.kernel.org/lkml/YZvuN0Wqmn7XB4dX@localhost.localdomain/

Signed-off-by: Alexey Gladkov <legion@kernel.org>

---

Alexey Gladkov (6):
  proc: Fix separator for subset option
  proc: Add allowlist to control access to procfs files
  proc: Check that subset= option has been set
  proc: Allow to use the allowlist filter in userns
  proc: Validate incoming allowlist
  doc: proc: Add description of subset=allowlist

 Documentation/filesystems/proc.rst |  10 +
 fs/proc/Kconfig                    |  10 +
 fs/proc/Makefile                   |   1 +
 fs/proc/generic.c                  |  15 +-
 fs/proc/inode.c                    |  16 +-
 fs/proc/internal.h                 |  33 ++++
 fs/proc/proc_allowlist.c           | 300 +++++++++++++++++++++++++++++
 fs/proc/root.c                     |  36 +++-
 include/linux/proc_fs.h            |  18 +-
 9 files changed, 420 insertions(+), 19 deletions(-)
 create mode 100644 fs/proc/proc_allowlist.c
  

On Wed, 25 Jan 2023 16:28:47 +0100 Alexey Gladkov <legion@kernel.org> wrote:

> The patch expands subset= option. If the proc is mounted with the
> subset=allowlist option, the /proc/allowlist file will appear. This file
> contains the filenames and directories that are allowed for this
> mountpoint. By default, /proc/allowlist contains only its own name.
> Changing the allowlist is possible as long as it is present in the
> allowlist itself.
> 
> This allowlist is applied in lookup/readdir so files that will create
> modules after mounting will not be visible.
> 
> Compared to the previous patches [1][2], I switched to a special virtual
> file from listing filenames in the mount options.
> 

Changlog doesn't explain why you think Linux needs this feature.  The
[2/6] changelog hints that containers might be involved.  IOW, please
fully describe the requirement and use-case(s).

Also, please describe why /proc/allowlist is made available via a mount
option, rather than being permanently present.

And why add to subset=, instead of a separate mount option.

Does /proc/allowlist work in subdirectories?  Like, permit presence of
/proc/sys/vm/compact_memory?

I think the whole thing is misnamed, really.  "allowlist" implies
access permissions.  Some of the test here uses "visibility" and other
places use "presence", which are better.  "presentlist" and
/proc/presentlist might be better.  But why not simply /proc/contents?

Please run these patches through checkpatch and consider the result.
  

On Wed, Jan 25, 2023 at 03:36:28PM -0800, Andrew Morton wrote:
> On Wed, 25 Jan 2023 16:28:47 +0100 Alexey Gladkov <legion@kernel.org> wrote:
> 
> > The patch expands subset= option. If the proc is mounted with the
> > subset=allowlist option, the /proc/allowlist file will appear. This file
> > contains the filenames and directories that are allowed for this
> > mountpoint. By default, /proc/allowlist contains only its own name.
> > Changing the allowlist is possible as long as it is present in the
> > allowlist itself.
> > 
> > This allowlist is applied in lookup/readdir so files that will create
> > modules after mounting will not be visible.
> > 
> > Compared to the previous patches [1][2], I switched to a special virtual
> > file from listing filenames in the mount options.
> > 
> 
> Changlog doesn't explain why you think Linux needs this feature.  The
> [2/6] changelog hints that containers might be involved.  IOW, please
> fully describe the requirement and use-case(s).
> 
> Also, please describe why /proc/allowlist is made available via a mount
> option, rather than being permanently present.
> 
> And why add to subset=, instead of a separate mount option.
> 
> Does /proc/allowlist work in subdirectories?  Like, permit presence of
> /proc/sys/vm/compact_memory?
> 
> I think the whole thing is misnamed, really.  "allowlist" implies
> access permissions.  Some of the test here uses "visibility" and other
> places use "presence", which are better.  "presentlist" and
> /proc/presentlist might be better.  But why not simply /proc/contents?

Currently, a lot of container runtimes - even if they mount a new procfs
instance - overmount various procfs files and directories to ensure that
they're hidden from the container workload. (The motivations for this
are mixed and usually it's only needed for containers that run with the
same privilege level as the host.)

The consequence of overmounting is that we need to refuse mounting
procfs again somewhere else otherwise the procfs instance might reveal
files and directories that were supposed to be hidden.

So this patchset moves the ability to hide entries into the kernel
through an allowlist. This way you can hide files and directories while
being able to mount procfs again because it will inherit the same
allowlist.

I get the motivation. The question is whether this belongs into the
kernel at all. I'm unfortunately not convinced.

This adds a lot of string parsing to procfs and I think we would also
need to decide what a reasonable maximum limit for such allowlists would
be. The data structure likely shouldn't be a linked list but at least an
rbtree especially if the size isn't limited.

But fundamentally I think it moves something that should be and
currently is a userspace policy into the kernel which I think is wrong.
Sure you can't predict what files show up in procfs over time but then
subset=pid is already your friend - even if not as fine-grained.

If this where another simple subset style mount option that allowlists a
bunch of well-known global proc files then sure. But making this
dynamically configurable from userspace doesn't make sense to me. I
mean, users could write /gobble/dy/gook into /proc/allowlist or use it
to stash secrets or hashes or whatever as we have no way of figuring out
whether the entry they allowlist does or will actually ever exist.

In general, such flexibility belongs into userspace imho.

Frankly, if that is really required it would almost make more sense to
be able to attach a new bpf program type to procfs that would allow to
filter procfs entries. Then the filter could be done purely in
userspace. If signed bpf lands one could then even ship signed programs
that are attachable by userns root.
  

On Wed, Jan 25, 2023 at 03:36:28PM -0800, Andrew Morton wrote:
> On Wed, 25 Jan 2023 16:28:47 +0100 Alexey Gladkov <legion@kernel.org> wrote:
> 
> > The patch expands subset= option. If the proc is mounted with the
> > subset=allowlist option, the /proc/allowlist file will appear. This file
> > contains the filenames and directories that are allowed for this
> > mountpoint. By default, /proc/allowlist contains only its own name.
> > Changing the allowlist is possible as long as it is present in the
> > allowlist itself.
> > 
> > This allowlist is applied in lookup/readdir so files that will create
> > modules after mounting will not be visible.
> > 
> > Compared to the previous patches [1][2], I switched to a special virtual
> > file from listing filenames in the mount options.
> > 
> 
> Changlog doesn't explain why you think Linux needs this feature.  The
> [2/6] changelog hints that containers might be involved.  IOW, please
> fully describe the requirement and use-case(s).

Ok. I will.

Basically, as Christian described, the motivation is to give
containerization programs (docker, podman, etc.) a way to control the
content in procfs.

Now container tools use a list of dangerous files that they hide with
overmount. But procfs is not a static filesystem and using a bad list to
hide dangerous files can't be the solution.

I believe that a container should define a list of files that it considers
useful within the container, and not try to hide what it considers
unwanted.

> Also, please describe why /proc/allowlist is made available via a mount
> option, rather than being permanently present.

Like subset=pid, this file is needed to change the visibility of files in
the procfs mountpoint.

> And why add to subset=, instead of a separate mount option.
> 
> Does /proc/allowlist work in subdirectories?  Like, permit presence of
> /proc/sys/vm/compact_memory?

Yes. But /proc/allowlist is limited in size to 128K.

> I think the whole thing is misnamed, really.  "allowlist" implies
> access permissions.  Some of the test here uses "visibility" and other
> places use "presence", which are better.  "presentlist" and
> /proc/presentlist might be better.  But why not simply /proc/contents?

I don't hold on to the name allowlist at all :) present list is perfect
for me. The /proc/contents is confusing to me. 

> Please run these patches through checkpatch and consider the result.

Ok. I will.
  

On Thu, Jan 26, 2023 at 11:16:07AM +0100, Christian Brauner wrote:
> On Wed, Jan 25, 2023 at 03:36:28PM -0800, Andrew Morton wrote:
> > On Wed, 25 Jan 2023 16:28:47 +0100 Alexey Gladkov <legion@kernel.org> wrote:
> > 
> > > The patch expands subset= option. If the proc is mounted with the
> > > subset=allowlist option, the /proc/allowlist file will appear. This file
> > > contains the filenames and directories that are allowed for this
> > > mountpoint. By default, /proc/allowlist contains only its own name.
> > > Changing the allowlist is possible as long as it is present in the
> > > allowlist itself.
> > > 
> > > This allowlist is applied in lookup/readdir so files that will create
> > > modules after mounting will not be visible.
> > > 
> > > Compared to the previous patches [1][2], I switched to a special virtual
> > > file from listing filenames in the mount options.
> > > 
> > 
> > Changlog doesn't explain why you think Linux needs this feature.  The
> > [2/6] changelog hints that containers might be involved.  IOW, please
> > fully describe the requirement and use-case(s).
> > 
> > Also, please describe why /proc/allowlist is made available via a mount
> > option, rather than being permanently present.
> > 
> > And why add to subset=, instead of a separate mount option.
> > 
> > Does /proc/allowlist work in subdirectories?  Like, permit presence of
> > /proc/sys/vm/compact_memory?
> > 
> > I think the whole thing is misnamed, really.  "allowlist" implies
> > access permissions.  Some of the test here uses "visibility" and other
> > places use "presence", which are better.  "presentlist" and
> > /proc/presentlist might be better.  But why not simply /proc/contents?
> 
> Currently, a lot of container runtimes - even if they mount a new procfs
> instance - overmount various procfs files and directories to ensure that
> they're hidden from the container workload. (The motivations for this
> are mixed and usually it's only needed for containers that run with the
> same privilege level as the host.)
> 
> The consequence of overmounting is that we need to refuse mounting
> procfs again somewhere else otherwise the procfs instance might reveal
> files and directories that were supposed to be hidden.
> 
> So this patchset moves the ability to hide entries into the kernel
> through an allowlist. This way you can hide files and directories while
> being able to mount procfs again because it will inherit the same
> allowlist.
> 
> I get the motivation. The question is whether this belongs into the
> kernel at all. I'm unfortunately not convinced.
> 
> This adds a lot of string parsing to procfs and I think we would also
> need to decide what a reasonable maximum limit for such allowlists would
> be.> The data structure likely shouldn't be a linked list but at least an
> rbtree especially if the size isn't limited.

There is a limit. So far I've limited the file size to 128k. I think this
is a reasonable limit.

> But fundamentally I think it moves something that should be and
> currently is a userspace policy into the kernel which I think is wrong.

We don't have mechanisms to implement this userspace policy. overmount is
not a solution but plugging holes in the absence of other ways to control
the visibility of files in procfs.

> Sure you can't predict what files show up in procfs over time but then
> subset=pid is already your friend - even if not as fine-grained.
> 
> If this where another simple subset style mount option that allowlists a
> bunch of well-known global proc files then sure. But making this
> dynamically configurable from userspace doesn't make sense to me. I
> mean, users could write /gobble/dy/gook into /proc/allowlist or use it
> to stash secrets or hashes or whatever as we have no way of figuring out
> whether the entry they allowlist does or will actually ever exist.

BTW I only allow printable data to be written to the file.

We can make this file write-only and then writing any extraneous data
there will not make sense.

> In general, such flexibility belongs into userspace imho.
> 
> Frankly, if that is really required it would almost make more sense to
> be able to attach a new bpf program type to procfs that would allow to
> filter procfs entries. Then the filter could be done purely in
> userspace. If signed bpf lands one could then even ship signed programs
> that are attachable by userns root.

I'll ask the podman developers how much more comfortable they would be
using bpf to control file visibility in procfs. thanks for the idea.
  

On Thu, Jan 26, 2023 at 02:39:30PM +0100, Alexey Gladkov wrote:
> > In general, such flexibility belongs into userspace imho.
> > 
> > Frankly, if that is really required it would almost make more sense to
> > be able to attach a new bpf program type to procfs that would allow to
> > filter procfs entries. Then the filter could be done purely in
> > userspace. If signed bpf lands one could then even ship signed programs
> > that are attachable by userns root.
> 
> I'll ask the podman developers how much more comfortable they would be
> using bpf to control file visibility in procfs. thanks for the idea.

I write for history.

After digging into eBPF, I came to the conclusion that nothing needs to be
done in kernel space. Access can be controlled via "lsm/file_open". Access
can be controlled per cgroup or per mountpoint, depending on the task.
Each project has its own choice.

Many thanks for pointing out eBPF.