[PATCH] Honor mmap_min_addr with the actual minimum

[PATCH] Honor mmap_min_addr with the actual minimum

[Hector Marco-Gisbert]

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.

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

Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Kees Cook]

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

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



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Hector Marco-Gisbert]

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 ?

Thanks,
Hector.



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

Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Kees Cook]

On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@upv.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 ?

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)



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Hector Marco-Gisbert]

El 21/04/16 a las 00:12, Kees Cook escribió:
> On Tue, Apr 19, 2016 at 11:55 AM, Hector Marco-Gisbert <hecmargi@upv.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 ?

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

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

Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Eric Paris]

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

Re: [PATCH] Honor mmap_min_addr with the actual minimum

[Hector Marco-Gisbert]

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@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)
>>>
>>>
>>>
>>

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