[PATCH] selinux: put the mmap() DAC controls before the MAC controls

[PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Paul Moore]

It turns out that doing the SELinux MAC checks for mmap() before the
DAC checks was causing users and the SELinux policy folks headaches
as users were seeing a lot of SELinux AVC denials for the
memprotect:mmap_zero permission that would have also been denied by
the normal DAC capability checks (CAP_SYS_RAWIO).

Example:

 # cat mmap_test.c
  #include <stdlib.h>
  #include <stdio.h>
  #include <errno.h>
  #include <sys/mman.h>

  int main(int argc, char *argv[])
  {
        int rc;
        void *mem;

        mem = mmap(0x0, 4096,
                   PROT_READ | PROT_WRITE,
                   MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
        if (mem == MAP_FAILED)
                return errno;
        printf("mem = %p\n", mem);
        munmap(mem, 4096);

        return 0;
  }
 # gcc -g -O0 -o mmap_test mmap_test.c
 # ./mmap_test
 mem = (nil)
 # ausearch -m AVC | grep mmap_zero
 type=AVC msg=audit(...): avc:  denied  { mmap_zero }
   for pid=1025 comm="mmap_test"
   scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
   tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
   tclass=memprotect

This patch corrects things so that when the above example is run by a
user without CAP_SYS_RAWIO the SELinux AVC is no longer generated as
the DAC capability check fails before the SELinux permission check.

Signed-off-by: Paul Moore <pmoore@redhat.com>
---
 security/selinux/hooks.c |   20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 57b0b49..e3664ae 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3205,24 +3205,20 @@ error:
 
 static int selinux_mmap_addr(unsigned long addr)
 {
-	int rc = 0;
-	u32 sid = current_sid();
+	int rc;
+
+	/* do DAC check on address space usage */
+	rc = cap_mmap_addr(addr);
+	if (rc)
+		return rc;
 
-	/*
-	 * notice that we are intentionally putting the SELinux check before
-	 * the secondary cap_file_mmap check.  This is such a likely attempt
-	 * at bad behaviour/exploit that we always want to get the AVC, even
-	 * if DAC would have also denied the operation.
-	 */
 	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
+		u32 sid = current_sid();
 		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
 				  MEMPROTECT__MMAP_ZERO, NULL);
-		if (rc)
-			return rc;
 	}
 
-	/* do DAC check on address space usage */
-	return cap_mmap_addr(addr);
+	return rc;
 }
 
 static int selinux_mmap_file(struct file *file, unsigned long reqprot,

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Stephen Smalley]

On 02/27/2014 09:30 AM, Paul Moore wrote:
> It turns out that doing the SELinux MAC checks for mmap() before the
> DAC checks was causing users and the SELinux policy folks headaches
> as users were seeing a lot of SELinux AVC denials for the
> memprotect:mmap_zero permission that would have also been denied by
> the normal DAC capability checks (CAP_SYS_RAWIO).

So you think that the explanation given in the comment for the current
ordering is no longer valid?

> 
> Example:
> 
>  # cat mmap_test.c
>   #include <stdlib.h>
>   #include <stdio.h>
>   #include <errno.h>
>   #include <sys/mman.h>
> 
>   int main(int argc, char *argv[])
>   {
>         int rc;
>         void *mem;
> 
>         mem = mmap(0x0, 4096,
>                    PROT_READ | PROT_WRITE,
>                    MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
>         if (mem == MAP_FAILED)
>                 return errno;
>         printf("mem = %p\n", mem);
>         munmap(mem, 4096);
> 
>         return 0;
>   }
>  # gcc -g -O0 -o mmap_test mmap_test.c
>  # ./mmap_test
>  mem = (nil)
>  # ausearch -m AVC | grep mmap_zero
>  type=AVC msg=audit(...): avc:  denied  { mmap_zero }
>    for pid=1025 comm="mmap_test"
>    scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>    tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>    tclass=memprotect
> 
> This patch corrects things so that when the above example is run by a
> user without CAP_SYS_RAWIO the SELinux AVC is no longer generated as
> the DAC capability check fails before the SELinux permission check.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>
> ---
>  security/selinux/hooks.c |   20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 57b0b49..e3664ae 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3205,24 +3205,20 @@ error:
>  
>  static int selinux_mmap_addr(unsigned long addr)
>  {
> -	int rc = 0;
> -	u32 sid = current_sid();
> +	int rc;
> +
> +	/* do DAC check on address space usage */
> +	rc = cap_mmap_addr(addr);
> +	if (rc)
> +		return rc;
>  
> -	/*
> -	 * notice that we are intentionally putting the SELinux check before
> -	 * the secondary cap_file_mmap check.  This is such a likely attempt
> -	 * at bad behaviour/exploit that we always want to get the AVC, even
> -	 * if DAC would have also denied the operation.
> -	 */
>  	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
> +		u32 sid = current_sid();
>  		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
>  				  MEMPROTECT__MMAP_ZERO, NULL);
> -		if (rc)
> -			return rc;
>  	}
>  
> -	/* do DAC check on address space usage */
> -	return cap_mmap_addr(addr);
> +	return rc;
>  }
>  
>  static int selinux_mmap_file(struct file *file, unsigned long reqprot,
> 
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> 

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Stephen Smalley]

On 02/27/2014 10:57 AM, Stephen Smalley wrote:
> On 02/27/2014 09:30 AM, Paul Moore wrote:
>> It turns out that doing the SELinux MAC checks for mmap() before the
>> DAC checks was causing users and the SELinux policy folks headaches
>> as users were seeing a lot of SELinux AVC denials for the
>> memprotect:mmap_zero permission that would have also been denied by
>> the normal DAC capability checks (CAP_SYS_RAWIO).
> 
> So you think that the explanation given in the comment for the current
> ordering is no longer valid?

And if so, is there in fact any real value in retaining the SELinux
check at all?  Do we in fact ever allow sys_rawio and not mmap_zero (or
if we do, is there a real reason for it)?

> 
>>
>> Example:
>>
>>  # cat mmap_test.c
>>   #include <stdlib.h>
>>   #include <stdio.h>
>>   #include <errno.h>
>>   #include <sys/mman.h>
>>
>>   int main(int argc, char *argv[])
>>   {
>>         int rc;
>>         void *mem;
>>
>>         mem = mmap(0x0, 4096,
>>                    PROT_READ | PROT_WRITE,
>>                    MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
>>         if (mem == MAP_FAILED)
>>                 return errno;
>>         printf("mem = %p\n", mem);
>>         munmap(mem, 4096);
>>
>>         return 0;
>>   }
>>  # gcc -g -O0 -o mmap_test mmap_test.c
>>  # ./mmap_test
>>  mem = (nil)
>>  # ausearch -m AVC | grep mmap_zero
>>  type=AVC msg=audit(...): avc:  denied  { mmap_zero }
>>    for pid=1025 comm="mmap_test"
>>    scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>>    tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>>    tclass=memprotect
>>
>> This patch corrects things so that when the above example is run by a
>> user without CAP_SYS_RAWIO the SELinux AVC is no longer generated as
>> the DAC capability check fails before the SELinux permission check.
>>
>> Signed-off-by: Paul Moore <pmoore@redhat.com>
>> ---
>>  security/selinux/hooks.c |   20 ++++++++------------
>>  1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 57b0b49..e3664ae 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -3205,24 +3205,20 @@ error:
>>  
>>  static int selinux_mmap_addr(unsigned long addr)
>>  {
>> -	int rc = 0;
>> -	u32 sid = current_sid();
>> +	int rc;
>> +
>> +	/* do DAC check on address space usage */
>> +	rc = cap_mmap_addr(addr);
>> +	if (rc)
>> +		return rc;
>>  
>> -	/*
>> -	 * notice that we are intentionally putting the SELinux check before
>> -	 * the secondary cap_file_mmap check.  This is such a likely attempt
>> -	 * at bad behaviour/exploit that we always want to get the AVC, even
>> -	 * if DAC would have also denied the operation.
>> -	 */
>>  	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
>> +		u32 sid = current_sid();
>>  		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
>>  				  MEMPROTECT__MMAP_ZERO, NULL);
>> -		if (rc)
>> -			return rc;
>>  	}
>>  
>> -	/* do DAC check on address space usage */
>> -	return cap_mmap_addr(addr);
>> +	return rc;
>>  }
>>  
>>  static int selinux_mmap_file(struct file *file, unsigned long reqprot,
>>
>> _______________________________________________
>> Selinux mailing list
>> Selinux@tycho.nsa.gov
>> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
>> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
>>
> 
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> 

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Paul Moore]

On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
> On 02/27/2014 09:30 AM, Paul Moore wrote:
> > It turns out that doing the SELinux MAC checks for mmap() before the
> > DAC checks was causing users and the SELinux policy folks headaches
> > as users were seeing a lot of SELinux AVC denials for the
> > memprotect:mmap_zero permission that would have also been denied by
> > the normal DAC capability checks (CAP_SYS_RAWIO).
> 
> So you think that the explanation given in the comment for the current
> ordering is no longer valid?

Yes and no.  Arguably there is still some value in it but there are enough 
problems with it as-is that I think the value is starting to be outweighed by 
the pain it is causing (Dan can be very annoying when he wants something <g>).  
For those users who still want notification of processes trying to mmap() low 
addresses, I think an audit watch is a much better approach.  I don't think 
SELinux shouldn't be acting as an intrustion detection tool when we have other 
things that do that job.

Let's also not forget that the MAC-before-DAC approach goes against the 
general approach to applying SELinux controls, so there is some argument to be 
had for consistency as well.

Do you have a strong objection to this patch?

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Stephen Smalley]

On 02/27/2014 11:22 AM, Paul Moore wrote:
> On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
>> On 02/27/2014 09:30 AM, Paul Moore wrote:
>>> It turns out that doing the SELinux MAC checks for mmap() before the
>>> DAC checks was causing users and the SELinux policy folks headaches
>>> as users were seeing a lot of SELinux AVC denials for the
>>> memprotect:mmap_zero permission that would have also been denied by
>>> the normal DAC capability checks (CAP_SYS_RAWIO).
>>
>> So you think that the explanation given in the comment for the current
>> ordering is no longer valid?
> 
> Yes and no.  Arguably there is still some value in it but there are enough 
> problems with it as-is that I think the value is starting to be outweighed by 
> the pain it is causing (Dan can be very annoying when he wants something <g>).  
> For those users who still want notification of processes trying to mmap() low 
> addresses, I think an audit watch is a much better approach.  I don't think 
> SELinux shouldn't be acting as an intrustion detection tool when we have other 
> things that do that job.
> 
> Let's also not forget that the MAC-before-DAC approach goes against the 
> general approach to applying SELinux controls, so there is some argument to be 
> had for consistency as well.
> 
> Do you have a strong objection to this patch?

No, although I do wonder if we ought to just dispense with mmap_zero
altogether at this point.  It made sense when there was no capability
check or if the capability was one of the extremely broad ones (e.g.
CAP_SYS_ADMIN), but I don't really see why we can't be just as
restrictive with CAP_SYS_RAWIO / sys_rawio as with mmap_zero.

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Christopher J. PeBenito]

On 02/27/2014 11:26 AM, Stephen Smalley wrote:
> On 02/27/2014 11:22 AM, Paul Moore wrote:
>> On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
>>> On 02/27/2014 09:30 AM, Paul Moore wrote:
>>>> It turns out that doing the SELinux MAC checks for mmap() before the
>>>> DAC checks was causing users and the SELinux policy folks headaches
>>>> as users were seeing a lot of SELinux AVC denials for the
>>>> memprotect:mmap_zero permission that would have also been denied by
>>>> the normal DAC capability checks (CAP_SYS_RAWIO).
>>>
>>> So you think that the explanation given in the comment for the current
>>> ordering is no longer valid?
>>
>> Yes and no.  Arguably there is still some value in it but there are enough 
>> problems with it as-is that I think the value is starting to be outweighed by 
>> the pain it is causing (Dan can be very annoying when he wants something <g>).  
>> For those users who still want notification of processes trying to mmap() low 
>> addresses, I think an audit watch is a much better approach.  I don't think 
>> SELinux shouldn't be acting as an intrustion detection tool when we have other 
>> things that do that job.
>>
>> Let's also not forget that the MAC-before-DAC approach goes against the 
>> general approach to applying SELinux controls, so there is some argument to be 
>> had for consistency as well.
>>
>> Do you have a strong objection to this patch?
> 
> No, although I do wonder if we ought to just dispense with mmap_zero
> altogether at this point.  It made sense when there was no capability
> check or if the capability was one of the extremely broad ones (e.g.
> CAP_SYS_ADMIN), but I don't really see why we can't be just as
> restrictive with CAP_SYS_RAWIO / sys_rawio as with mmap_zero.

Wouldn't removing mmap_zero be contrary to SELinux having fine-grained access controls, since CAP_SYS_RAWIO is broader than this specific case?

-- 
Chris PeBenito
Tresys Technology, LLC
www.tresys.com | oss.tresys.com

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Stephen Smalley]

On 02/27/2014 11:40 AM, Christopher J. PeBenito wrote:
> On 02/27/2014 11:26 AM, Stephen Smalley wrote:
>> On 02/27/2014 11:22 AM, Paul Moore wrote:
>>> On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
>>>> On 02/27/2014 09:30 AM, Paul Moore wrote:
>>>>> It turns out that doing the SELinux MAC checks for mmap() before the
>>>>> DAC checks was causing users and the SELinux policy folks headaches
>>>>> as users were seeing a lot of SELinux AVC denials for the
>>>>> memprotect:mmap_zero permission that would have also been denied by
>>>>> the normal DAC capability checks (CAP_SYS_RAWIO).
>>>>
>>>> So you think that the explanation given in the comment for the current
>>>> ordering is no longer valid?
>>>
>>> Yes and no.  Arguably there is still some value in it but there are enough 
>>> problems with it as-is that I think the value is starting to be outweighed by 
>>> the pain it is causing (Dan can be very annoying when he wants something <g>).  
>>> For those users who still want notification of processes trying to mmap() low 
>>> addresses, I think an audit watch is a much better approach.  I don't think 
>>> SELinux shouldn't be acting as an intrustion detection tool when we have other 
>>> things that do that job.
>>>
>>> Let's also not forget that the MAC-before-DAC approach goes against the 
>>> general approach to applying SELinux controls, so there is some argument to be 
>>> had for consistency as well.
>>>
>>> Do you have a strong objection to this patch?
>>
>> No, although I do wonder if we ought to just dispense with mmap_zero
>> altogether at this point.  It made sense when there was no capability
>> check or if the capability was one of the extremely broad ones (e.g.
>> CAP_SYS_ADMIN), but I don't really see why we can't be just as
>> restrictive with CAP_SYS_RAWIO / sys_rawio as with mmap_zero.
> 
> Wouldn't removing mmap_zero be contrary to SELinux having fine-grained access controls, since CAP_SYS_RAWIO is broader than this specific case?

Show me a real world scenario where I would want to allow sys_rawio but
not mmap_zero and where doing so would provide real security benefit.

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Paul Moore]

On Thursday, February 27, 2014 11:26:35 AM Stephen Smalley wrote:
> On 02/27/2014 11:22 AM, Paul Moore wrote:
> > On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
> >> On 02/27/2014 09:30 AM, Paul Moore wrote:
> >>> It turns out that doing the SELinux MAC checks for mmap() before the
> >>> DAC checks was causing users and the SELinux policy folks headaches
> >>> as users were seeing a lot of SELinux AVC denials for the
> >>> memprotect:mmap_zero permission that would have also been denied by
> >>> the normal DAC capability checks (CAP_SYS_RAWIO).
> >> 
> >> So you think that the explanation given in the comment for the current
> >> ordering is no longer valid?
> > 
> > Yes and no.  Arguably there is still some value in it but there are enough
> > problems with it as-is that I think the value is starting to be outweighed
> > by the pain it is causing (Dan can be very annoying when he wants
> > something <g>). For those users who still want notification of processes
> > trying to mmap() low addresses, I think an audit watch is a much better
> > approach.  I don't think SELinux shouldn't be acting as an intrustion
> > detection tool when we have other things that do that job.
> > 
> > Let's also not forget that the MAC-before-DAC approach goes against the
> > general approach to applying SELinux controls, so there is some argument
> > to be had for consistency as well.
> > 
> > Do you have a strong objection to this patch?
> 
> No, although I do wonder if we ought to just dispense with mmap_zero
> altogether at this point.  It made sense when there was no capability
> check or if the capability was one of the extremely broad ones (e.g.
> CAP_SYS_ADMIN), but I don't really see why we can't be just as
> restrictive with CAP_SYS_RAWIO / sys_rawio as with mmap_zero.

Seems like a reasonable argument to me.  I pinged Eric to get his thoughts on 
the issue since he added the check originally; if he is okay with removing it, 
I'll go ahead do it.

-- 
paul moore
security and virtualization @ redhat

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/27/2014 02:25 PM, Paul Moore wrote:
> On Thursday, February 27, 2014 11:26:35 AM Stephen Smalley wrote:
>> On 02/27/2014 11:22 AM, Paul Moore wrote:
>>> On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
>>>> On 02/27/2014 09:30 AM, Paul Moore wrote:
>>>>> It turns out that doing the SELinux MAC checks for mmap() before
>>>>> the DAC checks was causing users and the SELinux policy folks
>>>>> headaches as users were seeing a lot of SELinux AVC denials for
>>>>> the memprotect:mmap_zero permission that would have also been
>>>>> denied by the normal DAC capability checks (CAP_SYS_RAWIO).
>>>> 
>>>> So you think that the explanation given in the comment for the
>>>> current ordering is no longer valid?
>>> 
>>> Yes and no.  Arguably there is still some value in it but there are
>>> enough problems with it as-is that I think the value is starting to be
>>> outweighed by the pain it is causing (Dan can be very annoying when he
>>> wants something <g>). For those users who still want notification of
>>> processes trying to mmap() low addresses, I think an audit watch is a
>>> much better approach.  I don't think SELinux shouldn't be acting as an
>>> intrustion detection tool when we have other things that do that job.
>>> 
>>> Let's also not forget that the MAC-before-DAC approach goes against
>>> the general approach to applying SELinux controls, so there is some
>>> argument to be had for consistency as well.
>>> 
>>> Do you have a strong objection to this patch?
>> 
>> No, although I do wonder if we ought to just dispense with mmap_zero 
>> altogether at this point.  It made sense when there was no capability 
>> check or if the capability was one of the extremely broad ones (e.g. 
>> CAP_SYS_ADMIN), but I don't really see why we can't be just as 
>> restrictive with CAP_SYS_RAWIO / sys_rawio as with mmap_zero.
> 
> Seems like a reasonable argument to me.  I pinged Eric to get his thoughts
> on the issue since he added the check originally; if he is okay with
> removing it, I'll go ahead do it.
> 
The only thing is this is a nice debugging tool for the kernel.  Finding apps
that accidentally mmap_zero.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMPk1kACgkQrlYvE4MpobPVwACfZQGC8tldE6F5PXKLeYgELrYT
t28An21+GMg/0Jipe+8TLiKcHuBSYLM1
=TwuX
-----END PGP SIGNATURE-----

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Stephen Smalley]

On 02/27/2014 02:34 PM, Daniel J Walsh wrote:
> On 02/27/2014 02:25 PM, Paul Moore wrote:
>> On Thursday, February 27, 2014 11:26:35 AM Stephen Smalley wrote:
>>> On 02/27/2014 11:22 AM, Paul Moore wrote:
>>>> On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
>>>>> On 02/27/2014 09:30 AM, Paul Moore wrote:
>>>>>> It turns out that doing the SELinux MAC checks for mmap() before
>>>>>> the DAC checks was causing users and the SELinux policy folks
>>>>>> headaches as users were seeing a lot of SELinux AVC denials for
>>>>>> the memprotect:mmap_zero permission that would have also been
>>>>>> denied by the normal DAC capability checks (CAP_SYS_RAWIO).
>>>>>
>>>>> So you think that the explanation given in the comment for the
>>>>> current ordering is no longer valid?
>>>>
>>>> Yes and no.  Arguably there is still some value in it but there are
>>>> enough problems with it as-is that I think the value is starting to be
>>>> outweighed by the pain it is causing (Dan can be very annoying when he
>>>> wants something <g>). For those users who still want notification of
>>>> processes trying to mmap() low addresses, I think an audit watch is a
>>>> much better approach.  I don't think SELinux shouldn't be acting as an
>>>> intrustion detection tool when we have other things that do that job.
>>>>
>>>> Let's also not forget that the MAC-before-DAC approach goes against
>>>> the general approach to applying SELinux controls, so there is some
>>>> argument to be had for consistency as well.
>>>>
>>>> Do you have a strong objection to this patch?
>>>
>>> No, although I do wonder if we ought to just dispense with mmap_zero 
>>> altogether at this point.  It made sense when there was no capability 
>>> check or if the capability was one of the extremely broad ones (e.g. 
>>> CAP_SYS_ADMIN), but I don't really see why we can't be just as 
>>> restrictive with CAP_SYS_RAWIO / sys_rawio as with mmap_zero.
> 
>> Seems like a reasonable argument to me.  I pinged Eric to get his thoughts
>> on the issue since he added the check originally; if he is okay with
>> removing it, I'll go ahead do it.
> 
> The only thing is this is a nice debugging tool for the kernel.  Finding apps
> that accidentally mmap_zero.

You'll still see sys_rawio avc denials and the audit syscall record will
show that it was mmap of a low address.

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Stephen Smalley]

On 02/27/2014 02:52 PM, Stephen Smalley wrote:
> On 02/27/2014 02:34 PM, Daniel J Walsh wrote:
>> On 02/27/2014 02:25 PM, Paul Moore wrote:
>>> On Thursday, February 27, 2014 11:26:35 AM Stephen Smalley wrote:
>>>> On 02/27/2014 11:22 AM, Paul Moore wrote:
>>>>> On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley wrote:
>>>>>> On 02/27/2014 09:30 AM, Paul Moore wrote:
>>>>>>> It turns out that doing the SELinux MAC checks for mmap() before
>>>>>>> the DAC checks was causing users and the SELinux policy folks
>>>>>>> headaches as users were seeing a lot of SELinux AVC denials for
>>>>>>> the memprotect:mmap_zero permission that would have also been
>>>>>>> denied by the normal DAC capability checks (CAP_SYS_RAWIO).
>>>>>>
>>>>>> So you think that the explanation given in the comment for the
>>>>>> current ordering is no longer valid?
>>>>>
>>>>> Yes and no.  Arguably there is still some value in it but there are
>>>>> enough problems with it as-is that I think the value is starting to be
>>>>> outweighed by the pain it is causing (Dan can be very annoying when he
>>>>> wants something <g>). For those users who still want notification of
>>>>> processes trying to mmap() low addresses, I think an audit watch is a
>>>>> much better approach.  I don't think SELinux shouldn't be acting as an
>>>>> intrustion detection tool when we have other things that do that job.
>>>>>
>>>>> Let's also not forget that the MAC-before-DAC approach goes against
>>>>> the general approach to applying SELinux controls, so there is some
>>>>> argument to be had for consistency as well.
>>>>>
>>>>> Do you have a strong objection to this patch?
>>>>
>>>> No, although I do wonder if we ought to just dispense with mmap_zero 
>>>> altogether at this point.  It made sense when there was no capability 
>>>> check or if the capability was one of the extremely broad ones (e.g. 
>>>> CAP_SYS_ADMIN), but I don't really see why we can't be just as 
>>>> restrictive with CAP_SYS_RAWIO / sys_rawio as with mmap_zero.
>>
>>> Seems like a reasonable argument to me.  I pinged Eric to get his thoughts
>>> on the issue since he added the check originally; if he is okay with
>>> removing it, I'll go ahead do it.
>>
>> The only thing is this is a nice debugging tool for the kernel.  Finding apps
>> that accidentally mmap_zero.
> 
> You'll still see sys_rawio avc denials and the audit syscall record will
> show that it was mmap of a low address.

Looking at Fedora policy, there are differences in what domains are
allowed mmap_zero vs sys_rawio, although I don't know how
intentional/meaningful that is.

sesearch -A -p mmap_zero
vs
sesearch -A -p sys_rawio

Why for example does cupsd_t have sys_rawio?  That's rather disturbing.

I guess you should keep the separate check until those differences are
resolved.

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Stephen Smalley]

On 02/27/2014 09:30 AM, Paul Moore wrote:
> It turns out that doing the SELinux MAC checks for mmap() before the
> DAC checks was causing users and the SELinux policy folks headaches
> as users were seeing a lot of SELinux AVC denials for the
> memprotect:mmap_zero permission that would have also been denied by
> the normal DAC capability checks (CAP_SYS_RAWIO).
> 
> Example:
> 
>  # cat mmap_test.c
>   #include <stdlib.h>
>   #include <stdio.h>
>   #include <errno.h>
>   #include <sys/mman.h>
> 
>   int main(int argc, char *argv[])
>   {
>         int rc;
>         void *mem;
> 
>         mem = mmap(0x0, 4096,
>                    PROT_READ | PROT_WRITE,
>                    MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0);
>         if (mem == MAP_FAILED)
>                 return errno;
>         printf("mem = %p\n", mem);
>         munmap(mem, 4096);
> 
>         return 0;
>   }
>  # gcc -g -O0 -o mmap_test mmap_test.c
>  # ./mmap_test
>  mem = (nil)
>  # ausearch -m AVC | grep mmap_zero
>  type=AVC msg=audit(...): avc:  denied  { mmap_zero }
>    for pid=1025 comm="mmap_test"
>    scontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>    tcontext=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
>    tclass=memprotect
> 
> This patch corrects things so that when the above example is run by a
> user without CAP_SYS_RAWIO the SELinux AVC is no longer generated as
> the DAC capability check fails before the SELinux permission check.
> 
> Signed-off-by: Paul Moore <pmoore@redhat.com>

Acked-by:  Stephen Smalley <sds@tycho.nsa.gov>

> ---
>  security/selinux/hooks.c |   20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 57b0b49..e3664ae 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3205,24 +3205,20 @@ error:
>  
>  static int selinux_mmap_addr(unsigned long addr)
>  {
> -	int rc = 0;
> -	u32 sid = current_sid();
> +	int rc;
> +
> +	/* do DAC check on address space usage */
> +	rc = cap_mmap_addr(addr);
> +	if (rc)
> +		return rc;
>  
> -	/*
> -	 * notice that we are intentionally putting the SELinux check before
> -	 * the secondary cap_file_mmap check.  This is such a likely attempt
> -	 * at bad behaviour/exploit that we always want to get the AVC, even
> -	 * if DAC would have also denied the operation.
> -	 */
>  	if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
> +		u32 sid = current_sid();
>  		rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
>  				  MEMPROTECT__MMAP_ZERO, NULL);
> -		if (rc)
> -			return rc;
>  	}
>  
> -	/* do DAC check on address space usage */
> -	return cap_mmap_addr(addr);
> +	return rc;
>  }
>  
>  static int selinux_mmap_file(struct file *file, unsigned long reqprot,
> 
> _______________________________________________
> Selinux mailing list
> Selinux@tycho.nsa.gov
> To unsubscribe, send email to Selinux-leave@tycho.nsa.gov.
> To get help, send an email containing "help" to Selinux-request@tycho.nsa.gov.
> 
> 

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/27/2014 03:07 PM, Stephen Smalley wrote:
> On 02/27/2014 02:52 PM, Stephen Smalley wrote:
>> On 02/27/2014 02:34 PM, Daniel J Walsh wrote:
>>> On 02/27/2014 02:25 PM, Paul Moore wrote:
>>>> On Thursday, February 27, 2014 11:26:35 AM Stephen Smalley wrote:
>>>>> On 02/27/2014 11:22 AM, Paul Moore wrote:
>>>>>> On Thursday, February 27, 2014 10:57:46 AM Stephen Smalley
>>>>>> wrote:
>>>>>>> On 02/27/2014 09:30 AM, Paul Moore wrote:
>>>>>>>> It turns out that doing the SELinux MAC checks for mmap()
>>>>>>>> before the DAC checks was causing users and the SELinux
>>>>>>>> policy folks headaches as users were seeing a lot of SELinux
>>>>>>>> AVC denials for the memprotect:mmap_zero permission that
>>>>>>>> would have also been denied by the normal DAC capability
>>>>>>>> checks (CAP_SYS_RAWIO).
>>>>>>> 
>>>>>>> So you think that the explanation given in the comment for the 
>>>>>>> current ordering is no longer valid?
>>>>>> 
>>>>>> Yes and no.  Arguably there is still some value in it but there
>>>>>> are enough problems with it as-is that I think the value is
>>>>>> starting to be outweighed by the pain it is causing (Dan can be
>>>>>> very annoying when he wants something <g>). For those users who
>>>>>> still want notification of processes trying to mmap() low
>>>>>> addresses, I think an audit watch is a much better approach.  I
>>>>>> don't think SELinux shouldn't be acting as an intrustion
>>>>>> detection tool when we have other things that do that job.
>>>>>> 
>>>>>> Let's also not forget that the MAC-before-DAC approach goes
>>>>>> against the general approach to applying SELinux controls, so
>>>>>> there is some argument to be had for consistency as well.
>>>>>> 
>>>>>> Do you have a strong objection to this patch?
>>>>> 
>>>>> No, although I do wonder if we ought to just dispense with
>>>>> mmap_zero altogether at this point.  It made sense when there was
>>>>> no capability check or if the capability was one of the extremely
>>>>> broad ones (e.g. CAP_SYS_ADMIN), but I don't really see why we
>>>>> can't be just as restrictive with CAP_SYS_RAWIO / sys_rawio as with
>>>>> mmap_zero.
>>> 
>>>> Seems like a reasonable argument to me.  I pinged Eric to get his
>>>> thoughts on the issue since he added the check originally; if he is
>>>> okay with removing it, I'll go ahead do it.
>>> 
>>> The only thing is this is a nice debugging tool for the kernel.
>>> Finding apps that accidentally mmap_zero.
>> 
>> You'll still see sys_rawio avc denials and the audit syscall record will 
>> show that it was mmap of a low address.
> 
> Looking at Fedora policy, there are differences in what domains are allowed
> mmap_zero vs sys_rawio, although I don't know how intentional/meaningful
> that is.
> 
> sesearch -A -p mmap_zero vs sesearch -A -p sys_rawio
> 
> Why for example does cupsd_t have sys_rawio?  That's rather disturbing.
> 
> I guess you should keep the separate check until those differences are 
> resolved.
> 
> 
> 
> _______________________________________________ Selinux mailing list 
> Selinux@tycho.nsa.gov To unsubscribe, send email to
> Selinux-leave@tycho.nsa.gov. To get help, send an email containing "help"
> to Selinux-request@tycho.nsa.gov.
> 
> 

Don't know why cups has it but I will remove it.



-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlMPpiwACgkQrlYvE4MpobOnrACfUE/quImyDenbAQ0b3xytW5FU
2tEAniONTu0sIUbuqxObgofqZb/J+JQx
=oLp9
-----END PGP SIGNATURE-----

Re: [PATCH] selinux: put the mmap() DAC controls before the MAC controls

[Paul Moore]

On Thursday, February 27, 2014 03:07:55 PM Stephen Smalley wrote:
> Looking at Fedora policy, there are differences in what domains are
> allowed mmap_zero vs sys_rawio, although I don't know how
> intentional/meaningful that is.
> 
> sesearch -A -p mmap_zero
> vs
> sesearch -A -p sys_rawio
> 
> Why for example does cupsd_t have sys_rawio?  That's rather disturbing.
> 
> I guess you should keep the separate check until those differences are
> resolved.

No more comments overnight so I'm going to go ahead and push this to next with 
your ack added.  We can always remove the SELinux check at a later date.

Thanks.

-- 
paul moore
security and virtualization @ redhat