Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Eric Paris <eparis@redhat.com>]

On Wed, 2016-05-11 at 14:54 +0200, Hector Marco-Gisbert wrote:
> 
> El 21/04/16 a las 00:12, Kees Cook escribió:
> > On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@up
> > v.es> wrote:
> > > > On Wed, Apr 6, 2016 at 12:07 PM, Hector Marco-Gisbert <hecmargi
> > > > @upv.es> wrote:
> > > > > The minimum address that a process is allowed to mmap when
> > > > > LSM is
> > > > > enabled is 0x10000 (65536). This value is tunable and
> > > > > exported via
> > > > > /proc/sys/vm/mmap_min_addr but it is not honored with the
> > > > > actual
> > > > > minimum value.
> > > > 
> > > > I think this is working as intended already, based on the
> > > > commit log
> > > > for 788084aba2ab7348257597496befcbccabdc98a3
> > > > 
> > > > See cap_mmap_addr (which uses dac_mmap_min_addr) vs SELinux's
> > > > hook
> > > > (which uses CONFIG_LSM_MMAP_MIN_ADDR), and everything else
> > > > (that uses
> > > > mmap_min_addr).
> > > > 
> > > > Without CONFIG_LSM_MMAP_MIN_ADDR, dac_mmap_min_addr always ==
> > > > mmap_min_addr.
> > > > 
> > > > With CONFIG_LSM_MMAP_MIN_ADDR, dac_mmap_min_addr can be less
> > > > than
> > > > mmap_min_addr, but mmap_min_addr will always be at least
> > > > CONFIG_LSM_MMAP_MIN_ADDR.
> > > > 
> > > > Eric may be able to shed more light on this...
> > > > 
> > > > -Kees
> > > 
> > > Ok, I see your point, but it seems that minimum address that a
> > > process is
> > > allowed to map is mmap_min_addr and not dac_mmap_min_addr.
> > > This is because mmap_min_addr can be seen as the
> > > max(dac_mmap_min_addr,
> > > CONFIG_LSM_MMAP_MIN_ADDR) which is correct (the minimum allowed
> > > address) but
> > > /proc/sys/vm/mmap_min_addr contains dac_mmap_min_addr which is
> > > not the minimum.
> > > 
> > > For example, if we set the CONFIG_LSM_MMAP_MIN_ADDR to 65536 and
> > > /proc/sys/vm/mmap_min_addr to 4096, then assuming that
> > > selinux_mmap_addr() has
> > > no permissions (it returns !=0), the minimum allowed address is
> > > 65536 not 4096.
> > > The mmap check is done in the security_mmap_addr(addr) function
> > > in mm/mmap.c
> > > file. It seems that we are exporting the dac_mmap_min_addr
> > > instead of the actual
> > > minimum.
> > > 
> > > Is this behavior intended ? I'm missing something here ?
> 
> Yes, the sysctl is reporting the dac value.
> 
> I think the meaning of the exported mmap_min_addr value was changed
> in the
> commit you pointed. A new variable was added (dac_mmap_min_addr) and
> it was
> replaced in the sysctl of "mmap_min_addr" but the exported name
> (/proc/sys/vm/mmap_min_addr) was not changed:
> 
> .procname = "mmap_min_addr",
> - .data = &mmap_min_addr,
> + .data = &dac_mmap_min_addr,
> 
> This can be confusing since the returned value is not the expected
> one (the
> minimum value according to sysctl/vm.txt) but the dac_mmap_min_addr.
> So, I think
> that If we need to export the dac value then we can do it but it
> would be
> desirable not to change the meaning of this exported value.
> 
> Maybe by renaming /proc/sys/vm/mmap_min_addr to
> /proc/sys/vm/dac_mmap_min_addr
> and adding a read-only /proc/sys/vm/mmap_min_addr ?

This breaks scripts which are currently setting mmap_min_addr (like
wine on ubuntu I think?). Seems like a non-starter.

You're trying to represent multiple values in a single value. It just
isn't possible. You could expose lsm_mmap_min_addr RO in another sysctl
(not sure of other places we expose kernel configs like that, but you
could).

I wouldn't say the meaning of mmap_min_addr changed, we just grew a new
(underdocumented) lsm_mmap_min_addr. mmap_min_addr continued to be
controlled by and controlling exactly the same thing.

dac_mmap_min_addr is controlled by capabilities.
lsm_mmap_min_addr is controlled by your LSM.

You can expose those 2 values. But it would be us to each process to
know how to use them. A process might be able to avoid the dac check
but not the mac check (aka a root process) or a process might be able
to avoid the mac check but not the dac check (wine).

No single value can represent this. The best you could do is expose the
lsm/mac value, but I'm not sure I see the value. All you are doing is
telling exploit authors exactly how high they have to put their nasty
bits...

> 
> If ok, I could send a patch.
> 
> In any case, I think we should update the doc (sysctl/vm.txt).
> 
> All these issue came to light because we are working on a new ASLR
> for userspace
> and for testing it would be easier if we know where the VMA starts
> (this can be
> changed at runtime and it affects to the available entropy).
> 
> 
> Best,
> Hector.
> 
> > 
> > I think it is -- the minimum is correct, it's just that the sysctl
> > may
> > be reporting the dac value. Eric, are you able to chime in on this?
> > 
> > -Kees
> > 
> > > 
> > > Thanks,
> > > Hector.
> > > 
> > > > 
> > > > > 
> > > > > It can be easily checked in a system typing:
> > > > > 
> > > > > $ cat /proc/sys/vm/mmap_min_addr
> > > > > 4096    # <= Incorrect, it should be 65536
> > > > > 
> > > > > $ echo 1024 > /proc/sys/vm/mmap_min_addr
> > > > > $ cat /proc/sys/vm/mmap_min_addr
> > > > > 1024    # <= Incorrect, it should be 65536
> > > > > 
> > > > > After applying the patch:
> > > > > 
> > > > > $ cat /proc/sys/vm/mmap_min_addr
> > > > > 65536    # <= It is correct
> > > > > 
> > > > > $ echo 1024 > /proc/sys/vm/mmap_min_addr
> > > > > $ cat /proc/sys/vm/mmap_min_addr
> > > > > 65536    # <= It is correct
> > > > > 
> > > > > 
> > > > > 
> > > > > Signed-off-by: Hector Marco-Gisbert <hecmargi@upv.es>
> > > > > Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es>
> > > > > ---
> > > > >  security/min_addr.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/security/min_addr.c b/security/min_addr.c
> > > > > index f728728..96d1811 100644
> > > > > --- a/security/min_addr.c
> > > > > +++ b/security/min_addr.c
> > > > > @@ -15,10 +15,12 @@ unsigned long dac_mmap_min_addr =
> > > > > CONFIG_DEFAULT_MMAP_MIN_ADDR;
> > > > >  static void update_mmap_min_addr(void)
> > > > >  {
> > > > >  #ifdef CONFIG_LSM_MMAP_MIN_ADDR
> > > > > -       if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR)
> > > > > +       if (dac_mmap_min_addr > CONFIG_LSM_MMAP_MIN_ADDR) {
> > > > >                 mmap_min_addr = dac_mmap_min_addr;
> > > > > -       else
> > > > > +       } else {
> > > > >                 mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR;
> > > > > +               dac_mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR;
> > > > > +       }
> > > > >  #else
> > > > >         mmap_min_addr = dac_mmap_min_addr;
> > > > >  #endif
> > > > > --
> > > > > 1.9.1
> > > > > 
> > > > 
> > > > 
> > > > 
> > > 
> > > --
> > > Dr. Hector Marco-Gisbert @ http://hmarco.org/
> > > Cyber Security Researcher @ http://cybersecurity.upv.es
> > > Universitat Politècnica de València (Spain)
> > 
> > 
> > 
>