Message-ID: <5734C3CB.7080105@upv.es>
Date:	Thu, 12 May 2016 19:56:27 +0200
From:	Hector Marco-Gisbert <hecmargi@....es>
To:	Eric Paris <eparis@...hat.com>, Kees Cook <keescook@...omium.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	James Morris <james.l.morris@...cle.com>,
	"Serge E. Hallyn" <serge@...lyn.com>,
	linux-security-module <linux-security-module@...r.kernel.org>,
	Ismael Ripoll <iripoll@....es>
Subject: Re: [PATCH] Honor mmap_min_addr with the actual minimum

Thanks for the clarification. Below some comments.

> 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...
> 

Well, you can set the permissions of this file to 0600 or even use SELINUX to
prevent exploit authors to read this value :)

In any case if dac value is lower than lsm then any userspace application can
obtain the lsm value by justing calling mmap().

All these issues comes out because we are working in a new ASLR (ASLRNG) and it
would be useful to known "the worst case" (the maximum of SELINUX and CAP or any
other) at user level just for testing purposes. But we agree that is not a big
issue and maybe does not worth to export that value.

Best,
Hector.


>>
>> 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@....es>
>>>>>> Acked-by: Ismael Ripoll Ripoll <iripoll@....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)
>>>
>>>
>>>
>>

-- 
Dr. Hector Marco-Gisbert @ http://hmarco.org/
Cyber Security Researcher @ http://cybersecurity.upv.es
Universitat Politècnica de València (Spain)

Powered by blists - more mailing lists