Message-ID: <2q6hzkvep2g3z6m2jrwbw2j3sbydf6tgj2obwd6hgmm7xzgsg3@ddr5ghmsia5k>
Date: Sat, 28 Sep 2024 14:43:33 +0100
From: Pedro Falcato <pedro.falcato@...il.com>
To: Jeff Xu <jeffxu@...omium.org>
Cc: akpm@...ux-foundation.org, keescook@...omium.org, corbet@....net, 
	jeffxu@...gle.com, jorgelo@...omium.org, groeck@...omium.org, 
	linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org, linux-mm@...ck.org, 
	jannh@...gle.com, sroettger@...gle.com, linux-hardening@...r.kernel.org, 
	willy@...radead.org, gregkh@...uxfoundation.org, torvalds@...ux-foundation.org, 
	deraadt@...nbsd.org, usama.anjum@...labora.com, surenb@...gle.com, merimus@...gle.com, 
	rdunlap@...radead.org, lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com, 
	enh@...gle.com
Subject: Re: [PATCH v1 1/1] mseal: update mseal.rst

On Fri, Sep 27, 2024 at 06:29:30PM GMT, Jeff Xu wrote:
> Hi Pedro,
> 
> On Fri, Sep 27, 2024 at 3:59 PM Pedro Falcato <pedro.falcato@...il.com> wrote:
<snip>
> > > +
> > > +   Blocked mm syscall:
> > > +      - munmap
> > > +      - mmap
> > > +      - mremap
> > > +      - mprotect and pkey_mprotect
> > > +      - some destructive madvise behaviors: MADV_DONTNEED, MADV_FREE,
> > > +        MADV_DONTNEED_LOCKED, MADV_FREE, MADV_DONTFORK, MADV_WIPEONFORK
> > > +
> > > +   The first set of syscall to block is munmap, mremap, mmap. They can
> > > +   either leave an empty space in the address space, therefore allow
> > > +   replacement with a new mapping with new set of attributes, or can
> > > +   overwrite the existing mapping with another mapping.
> > > +
> > > +   mprotect and pkey_mprotect are blocked because they changes the
> >                                                           change
> > > +   protection bits (rwx) of the mapping.
> > > +
> > > +   Some destructive madvice behaviors (MADV_DONTNEED, MADV_FREE,
> > > +   MADV_DONTNEED_LOCKED, MADV_FREE, MADV_DONTFORK, MADV_WIPEONFORK)
> > > +   for anonymous memory, when users don't have write permission to the
> > > +   memory. Those behaviors can alter region contents by discarding pages,
> > > +   effectively a memset(0) for anonymous memory.
> >
> > What's the difference between anonymous memory and MAP_PRIVATE | MAP_FILE?
> >
> MAP_FILE seems not used ?
> anonymous mapping is the mapping that is not backed by a file.

MAP_FILE is actually defined as 0 usually :) But I meant file-backed private mappings.

> > The feature now, as is (as far as I understand!) will allow you to do things like MADV_DONTNEED
> > on a read-only file mapping. e.g .text. This is obviously wrong?
> >
> When a MADV_DONTNEED is called, pages will be freed, on file-backed
> mapping,  if the process reads from the mapping again, the content
> will be retrieved from the file.
> 

Sorry, it was late and I gave you a crap example. Consider this:
a file-backed MAP_PRIVATE vma is marked RW. I write to it, then RO-it + mseal.

The attacker later gets me to MADV_DONTNEED that VMA. You've just lost data.

The big problem here is with anon _pages_, not anon vmas.

> For anonymous mapping, since  there is no file backup, if process
> reads from the mapping, 0 is filled, hence equivalent to memset(0)
> 
> > > +
> > > +   Kernel will return -EPERM for blocked syscalls.
> > > +
> > > +   When blocked syscall return -EPERM due to sealing, the memory regions may or may not be changed, depends on the syscall being blocked:
> > > +      - munmap: munmap is atomic. If one of VMAs in the given range is
> > > +        sealed, none of VMAs are updated.
> > > +      - mprotect, pkey_mprotect, madvise: partial update might happen, e.g.
> > > +        when mprotect over multiple VMAs, mprotect might update the beginning
> > > +        VMAs before reaching the sealed VMA and return -EPERM.
> > > +      - mmap and mremap: undefined behavior.
> >
> > mmap and mremap are actually not undefined as they use munmap semantics for their unmapping.
> > Whether this is something we'd want to document, I don't know honestly (nor do I think is ever written down in POSIX?)
> >
> I'm not sure if I can declare mmap/mremap as atomic.
> 
> Although, it might be possible to achieve this due to munmap being
> atomic. I'm not sure  as I didn't test this. Would you like to find
> out ?

I just told you they use munmap under the hood. It's just that the requirement isn't actually
written down anywhere.

> 
> > >
> > >  Use cases:
> > >  ==========
> > >  - glibc:
> > >    The dynamic linker, during loading ELF executables, can apply sealing to
> > > -  non-writable memory segments.
> > > +  mapping segments.
> > >
> > >  - Chrome browser: protect some security sensitive data-structures.
> > >
> > > -Notes on which memory to seal:
> > > -==============================
> > > -
> > > -It might be important to note that sealing changes the lifetime of a mapping,
> > > -i.e. the sealed mapping won’t be unmapped till the process terminates or the
> > > -exec system call is invoked. Applications can apply sealing to any virtual
> > > -memory region from userspace, but it is crucial to thoroughly analyze the
> > > -mapping's lifetime prior to apply the sealing.
> > > +Don't use mseal on:
> > > +===================
> > > +Applications can apply sealing to any virtual memory region from userspace,
> > > +but it is *crucial to thoroughly analyze the mapping's lifetime* prior to
> > > +apply the sealing. This is because the sealed mapping *won’t be unmapped*
> > > +till the process terminates or the exec system call is invoked.
> >
> > There should probably be a nice disclaimer as to how most people don't need this or shouldn't use this.
> > At least in its current form.
> >
> Ya, the mseal is not for most apps. I mention the malloc example to stress that.
> 
> > <snip>
> > > -
> > > -
> > > -Additional notes:
> > > -=================
> > >  As Jann Horn pointed out in [3], there are still a few ways to write
> > > -to RO memory, which is, in a way, by design. Those cases are not covered
> > > -by mseal(). If applications want to block such cases, sandbox tools (such as
> > > -seccomp, LSM, etc) might be considered.
> > > +to RO memory, which is, in a way, by design. And those could be blocked
> > > +by different security measures.
> > >
> > >  Those cases are:
> > > -
> > > -- Write to read-only memory through /proc/self/mem interface.
> > > -- Write to read-only memory through ptrace (such as PTRACE_POKETEXT).
> > > -- userfaultfd.
> > > +   - Write to read-only memory through /proc/self/mem interface (FOLL_FORCE).
> > > +   - Write to read-only memory through ptrace (such as PTRACE_POKETEXT).
> > > +   - userfaultfd.
> >
> > I don't understand how this is not a problem, but MADV_DONTNEED is.
> > To me it seems that what we have now is completely useless, because you can trivially
> > bypass it using /proc/self/mem, which is enabled on most Linux systems.
> >
> > Before you mention ChromeOS or Chrome, I don't care. Kernel features aren't designed
> > for Chrome. They need to work with every other distro and application as well.
> >
> > It seems to me that the most sensible change is blocking/somehow distinguishing between /proc/self/mem and
> > /proc/<pid>/mem (some other process) and ptrace. As in blocking /proc/self/mem but allowing the other FOLL_FORCE's
> > as the traditional UNIX permission model allows.
> >
> IMO, it is a matter of  Divide and Conquer.  In a nutshell, mseal only
> prevents VMA's certain attributes (such as prot bits) from changing.
> It doesn't mean to say that sealed RO memory is immutable. To achieve
> that, the system needs to apply multiple security measures.

No, it's a matter of providing a sane API without tons of edgecases. Making a VMA immutable should make a VMA
immutable, and not require you to provide a crap ton of other mechanisms in order to truly make it immutable.
If I call mseal, I expect it to be sealed, not "sealed except when it's not, lol".

You haven't been able to quite specify what semantics are desirable out of this whole thing. Making
prot flags "immutable" is completely worthless if you can simply write to a random pseudofile and
have it bypass the whole thing (where a write to /proc/self/mem is semantically equivalent to
mprotect RW + write + mprotect RO). Making the vma immutable is completely worthless
if I can simply wipe anon pages. There has to be some end goal here (make contents immutable?
make sure VMA protection can't be changed? both?) which seems to be unclear from the kernel mmap-side.

If you insist on providing half-baked APIs (and waving off any concerns), I'm sure this would've been better
implemented as a random bpf program for chrome. Maybe we could revert this whole thing and give eBPF one
or two bits of vma flags for their own uses :)

> 
> For writing to /proc/pid/mem, it can be disabled via [1].  SELINUX and
> Landlock can achieve the same protection too.

I'm not blocking /proc/pid/mem, and my distro doesn't run any of those security modules :/

-- 
Pedro

Powered by blists - more mailing lists