[RFC][PATCH] selinux: add a map permission check for mmap

[RFC][PATCH] selinux: add a map permission check for mmap

[Stephen Smalley]

Add a map permission check on mmap so that we can distinguish memory mapped
access (since it has different implications for revocation). When a file
is opened and then read or written via syscalls like read(2)/write(2),
we revalidate access on each read/write operation via
selinux_file_permission() and therefore can revoke access if the
process context, the file context, or the policy changes in such a
manner that access is no longer allowed. When a file is opened and then
memory mapped via mmap(2) and then subsequently read or written directly
in memory, we presently have no way to revalidate or revoke access.
The purpose of a separate map permission check on mmap(2) is to permit
policy to prohibit memory mapping of specific files for which we need
to ensure that every access is revalidated, particularly useful for
scenarios where we expect the file to be relabeled at runtime in order
to reflect state changes (e.g. cross-domain solution, assured pipeline
without data copying).

Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
---
NB I chose not to define a new policy capability for this permission,
since it is adequately covered by handle_unknown for compatibility and
others seemed to agree that this does not fall into the category of
changes requiring a new policy capability.  I also chose to define the
permission for socket classes in addition to file classes and let it
be checked for both.

 security/selinux/hooks.c            | 12 ++++++++++++
 security/selinux/include/classmap.h |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e67a526..5432628 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3550,6 +3550,18 @@ static int selinux_mmap_addr(unsigned long addr)
 static int selinux_mmap_file(struct file *file, unsigned long reqprot,
 			     unsigned long prot, unsigned long flags)
 {
+	struct common_audit_data ad;
+	int rc;
+
+	if (file) {
+		ad.type = LSM_AUDIT_DATA_FILE;
+		ad.u.file = file;
+		rc = inode_has_perm(current_cred(), file_inode(file),
+				    FILE__MAP, &ad);
+		if (rc)
+			return rc;
+	}
+
 	if (selinux_checkreqprot)
 		prot = reqprot;
 
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 1e0cc9b..3e49a78 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -1,7 +1,7 @@
 #include <linux/capability.h>
 
 #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
-    "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append"
+    "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
 
 #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
     "rename", "execute", "quotaon", "mounton", "audit_access", \
-- 
2.9.3

Re: [RFC][PATCH] selinux: add a map permission check for mmap

[Joshua Brindle]

Stephen Smalley wrote:
> Add a map permission check on mmap so that we can distinguish memory mapped
> access (since it has different implications for revocation). When a file
> is opened and then read or written via syscalls like read(2)/write(2),
> we revalidate access on each read/write operation via
> selinux_file_permission() and therefore can revoke access if the
> process context, the file context, or the policy changes in such a
> manner that access is no longer allowed. When a file is opened and then
> memory mapped via mmap(2) and then subsequently read or written directly
> in memory, we presently have no way to revalidate or revoke access.
> The purpose of a separate map permission check on mmap(2) is to permit
> policy to prohibit memory mapping of specific files for which we need
> to ensure that every access is revalidated, particularly useful for
> scenarios where we expect the file to be relabeled at runtime in order
> to reflect state changes (e.g. cross-domain solution, assured pipeline
> without data copying).
>
> Signed-off-by: Stephen Smalley<sds@tycho.nsa.gov>
> ---
> NB I chose not to define a new policy capability for this permission,
> since it is adequately covered by handle_unknown for compatibility and
> others seemed to agree that this does not fall into the category of
> changes requiring a new policy capability.  I also chose to define the
> permission for socket classes in addition to file classes and let it
> be checked for both.

Thank you. This is very helpful. This fully closes the relabel 
revocation issue, right?

Is it actually safe to tell people that relabel+move is sufficient in an 
assured pipeline now?

>
>   security/selinux/hooks.c            | 12 ++++++++++++
>   security/selinux/include/classmap.h |  2 +-
>   2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e67a526..5432628 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3550,6 +3550,18 @@ static int selinux_mmap_addr(unsigned long addr)
>   static int selinux_mmap_file(struct file *file, unsigned long reqprot,
>   			     unsigned long prot, unsigned long flags)
>   {
> +	struct common_audit_data ad;
> +	int rc;
> +
> +	if (file) {
> +		ad.type = LSM_AUDIT_DATA_FILE;
> +		ad.u.file = file;
> +		rc = inode_has_perm(current_cred(), file_inode(file),
> +				    FILE__MAP,&ad);
> +		if (rc)
> +			return rc;
> +	}
> +
>   	if (selinux_checkreqprot)
>   		prot = reqprot;
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 1e0cc9b..3e49a78 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -1,7 +1,7 @@
>   #include<linux/capability.h>
>
>   #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
> -    "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append"
> +    "getattr", "setattr", "lock", "relabelfrom", "relabelto", "append", "map"
>
>   #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link", \
>       "rename", "execute", "quotaon", "mounton", "audit_access", \

Re: [RFC][PATCH] selinux: add a map permission check for mmap

[Stephen Smalley]

On Fri, 2017-05-05 at 09:14 -0400, Stephen Smalley wrote:
> Add a map permission check on mmap so that we can distinguish memory
> mapped
> access (since it has different implications for revocation). When a
> file
> is opened and then read or written via syscalls like
> read(2)/write(2),
> we revalidate access on each read/write operation via
> selinux_file_permission() and therefore can revoke access if the
> process context, the file context, or the policy changes in such a
> manner that access is no longer allowed. When a file is opened and
> then
> memory mapped via mmap(2) and then subsequently read or written
> directly
> in memory, we presently have no way to revalidate or revoke access.
> The purpose of a separate map permission check on mmap(2) is to
> permit
> policy to prohibit memory mapping of specific files for which we need
> to ensure that every access is revalidated, particularly useful for
> scenarios where we expect the file to be relabeled at runtime in
> order
> to reflect state changes (e.g. cross-domain solution, assured
> pipeline
> without data copying).
> 
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> NB I chose not to define a new policy capability for this permission,
> since it is adequately covered by handle_unknown for compatibility
> and
> others seemed to agree that this does not fall into the category of
> changes requiring a new policy capability.  I also chose to define
> the
> permission for socket classes in addition to file classes and let it
> be checked for both.
> 
>  security/selinux/hooks.c            | 12 ++++++++++++
>  security/selinux/include/classmap.h |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e67a526..5432628 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3550,6 +3550,18 @@ static int selinux_mmap_addr(unsigned long
> addr)
>  static int selinux_mmap_file(struct file *file, unsigned long
> reqprot,
>  			     unsigned long prot, unsigned long
> flags)
>  {
> +	struct common_audit_data ad;
> +	int rc;
> +
> +	if (file) {
> +		ad.type = LSM_AUDIT_DATA_FILE;
> +		ad.u.file = file;
> +		rc = inode_has_perm(current_cred(),
> file_inode(file),
> +				    FILE__MAP, &ad);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	if (selinux_checkreqprot)
>  		prot = reqprot;
>  
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 1e0cc9b..3e49a78 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -1,7 +1,7 @@
>  #include <linux/capability.h>
>  
>  #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
> -    "getattr", "setattr", "lock", "relabelfrom", "relabelto",
> "append"
> +    "getattr", "setattr", "lock", "relabelfrom", "relabelto",
> "append", "map"
>  
>  #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link",
> \
>      "rename", "execute", "quotaon", "mounton", "audit_access", \

Below is the refpolicy patch (relative to the Fedora rawhide policy
source RPM) I used for testing.  I simply added the map permission
declaration to the common file and common socket definitions since I
wasn't concerned with compatibility for kernels that predate the
dynamic class/perm mapping support (< 2.6.33, RHEL < 6).  Not sure
whether that's acceptable for upstream refpolicy or not; if not, it
will need to be added at the end of the existing lists for all file and
socket classes.  With regard to allow rules, I initially tried just
adding map permission to mmap_file_perms and exec_file_perms, which
covered most shared libraries and executables, but quickly ran into a
number of cases where programs and libraries required it just for
mmap'ing data files.  I addressed several of those but more kept
popping up.  Abstractly, any domain that previously was allowed read
permission could have used mmap, so if the goal is to ensure 100%
backward compatibility in refpolicy, we would need to add it to every
rule that allows read permission.  In my limited testing, it seemed to
suffice to add it to the _file_perms and _chr_file_perms macros that
already allowed open (although technically a process that
inherits/receives a descriptor could mmap it as well).  I think it is
ok to allow map permission widely in refpolicy (as with open
permission), and leave restricting it to specialized domains (as with
sandbox domains for open) given its narrow use case.

diff -ru serefpolicy-3.13.1.orig/policy/flask/access_vectors serefpolicy-3.13.1/policy/flask/access_vectors
--- serefpolicy-3.13.1.orig/policy/flask/access_vectors	2017-05-03 16:25:32.750029182 -0400
+++ serefpolicy-3.13.1/policy/flask/access_vectors	2017-05-04 09:18:58.999737761 -0400
@@ -20,6 +20,7 @@
 	relabelfrom
 	relabelto
 	append
+	map
 	unlink
 	link
 	rename
@@ -47,6 +48,7 @@
 	relabelfrom
 	relabelto
 	append
+	map
 # socket-specific
 	bind
 	connect
diff -ru serefpolicy-3.13.1.orig/policy/support/obj_perm_sets.spt serefpolicy-3.13.1/policy/support/obj_perm_sets.spt
--- serefpolicy-3.13.1.orig/policy/support/obj_perm_sets.spt	2017-05-03 16:25:32.751029170 -0400
+++ serefpolicy-3.13.1/policy/support/obj_perm_sets.spt	2017-05-04 09:34:15.418877678 -0400
@@ -153,19 +153,19 @@
 define(`getattr_file_perms',`{ getattr }')
 define(`setattr_file_perms',`{ setattr }')
 define(`read_inherited_file_perms',`{ getattr read ioctl lock }')
-define(`read_file_perms',`{ open read_inherited_file_perms }')
-define(`mmap_file_perms',`{ getattr open read execute ioctl }')
-define(`exec_file_perms',`{ getattr open read execute ioctl execute_no_trans }')
+define(`read_file_perms',`{ open map read_inherited_file_perms }')
+define(`mmap_file_perms',`{ getattr open map read execute ioctl }')
+define(`exec_file_perms',`{ getattr open map read execute ioctl execute_no_trans }')
 define(`append_inherited_file_perms',`{ getattr append }')
 define(`append_file_perms',`{ open lock ioctl append_inherited_file_perms }')
 define(`write_inherited_file_perms',`{ getattr write append lock ioctl }')
-define(`write_file_perms',`{ open write_inherited_file_perms }')
+define(`write_file_perms',`{ open map write_inherited_file_perms }')
 define(`rw_inherited_file_perms',`{ getattr read write append ioctl lock }')
-define(`rw_file_perms',`{ open rw_inherited_file_perms }')
-define(`create_file_perms',`{ getattr create open }')
+define(`rw_file_perms',`{ open map rw_inherited_file_perms }')
+define(`create_file_perms',`{ getattr create open map }')
 define(`rename_file_perms',`{ getattr rename }')
 define(`delete_file_perms',`{ getattr unlink }')
-define(`manage_file_perms',`{ create open getattr setattr read write append rename link unlink ioctl lock }')
+define(`manage_file_perms',`{ create open map getattr setattr read write append rename link unlink ioctl lock }')
 define(`relabelfrom_file_perms',`{ getattr relabelfrom }')
 define(`relabelto_file_perms',`{ getattr relabelto }')
 define(`relabel_file_perms',`{ getattr relabelfrom relabelto }')
@@ -245,15 +245,15 @@
 #
 define(`getattr_chr_file_perms',`{ getattr }')
 define(`setattr_chr_file_perms',`{ setattr }')
-define(`read_chr_file_perms',`{ getattr open read lock ioctl }')
+define(`read_chr_file_perms',`{ getattr open map read lock ioctl }')
 define(`append_chr_file_perms',`{ getattr open append lock ioctl }')
-define(`write_chr_file_perms',`{ getattr open write append lock ioctl }')
+define(`write_chr_file_perms',`{ getattr open map write append lock ioctl }')
 define(`rw_inherited_chr_file_perms',`{ getattr read write append ioctl lock }')
-define(`rw_chr_file_perms',`{ open rw_inherited_chr_file_perms }')
+define(`rw_chr_file_perms',`{ open map rw_inherited_chr_file_perms }')
 define(`create_chr_file_perms',`{ getattr create }')
 define(`rename_chr_file_perms',`{ getattr rename }')
 define(`delete_chr_file_perms',`{ getattr unlink }')
-define(`manage_chr_file_perms',`{ create open getattr setattr read write append rename link unlink ioctl lock }')
+define(`manage_chr_file_perms',`{ create open map getattr setattr read write append rename link unlink ioctl lock }')
 define(`relabelfrom_chr_file_perms',`{ getattr relabelfrom }')
 define(`relabelto_chr_file_perms',`{ getattr relabelto }')
 define(`relabel_chr_file_perms',`{ getattr relabelfrom relabelto }')

Re: [RFC][PATCH] selinux: add a map permission check for mmap

[Stephen Smalley]

On Fri, 2017-05-05 at 09:42 -0400, Joshua Brindle wrote:
> Stephen Smalley wrote:
> > Add a map permission check on mmap so that we can distinguish
> > memory mapped
> > access (since it has different implications for revocation). When a
> > file
> > is opened and then read or written via syscalls like
> > read(2)/write(2),
> > we revalidate access on each read/write operation via
> > selinux_file_permission() and therefore can revoke access if the
> > process context, the file context, or the policy changes in such a
> > manner that access is no longer allowed. When a file is opened and
> > then
> > memory mapped via mmap(2) and then subsequently read or written
> > directly
> > in memory, we presently have no way to revalidate or revoke access.
> > The purpose of a separate map permission check on mmap(2) is to
> > permit
> > policy to prohibit memory mapping of specific files for which we
> > need
> > to ensure that every access is revalidated, particularly useful for
> > scenarios where we expect the file to be relabeled at runtime in
> > order
> > to reflect state changes (e.g. cross-domain solution, assured
> > pipeline
> > without data copying).
> > 
> > Signed-off-by: Stephen Smalley<sds@tycho.nsa.gov>
> > ---
> > NB I chose not to define a new policy capability for this
> > permission,
> > since it is adequately covered by handle_unknown for compatibility
> > and
> > others seemed to agree that this does not fall into the category of
> > changes requiring a new policy capability.  I also chose to define
> > the
> > permission for socket classes in addition to file classes and let
> > it
> > be checked for both.
> 
> Thank you. This is very helpful. This fully closes the relabel 
> revocation issue, right?
> 
> Is it actually safe to tell people that relabel+move is sufficient in
> an 
> assured pipeline now?

It isn't a 100% solution, but it is likely sufficient to address their
concerns. When possible, I would still encourage use of copying rather
than relabeling but this does reduce the risk associated with doing so.

The residual concern is with respect to in-progress system calls that
have already passed the associated permission checks but not yet
completed the data transfer.  In Flask/Fluke, we could interrupt such
operations or wait for them to complete (with bounded delay).  Not so
much in Linux.  There is also some level of async I/O support in the
Linux kernel, which could widen the window between the permission check
and the data transfer, although the kernel AIO implementation seems to
be perpetually incomplete and is not used by the glibc aio
implementation.  However, this is less of a concern than the ability to
map the file and read/write the data indefinitely after the relabel
completes, which is what this patch is seeking to help prevent.

Obviously to leverage this facility one must ensure that no domains are
 allowed map permission to the relevant file types.  That will require
removal of unconfined, careful policy writing (e.g. don't use refpolicy
patterns/permission sets that might include map) and appropriate use of
neverallows to ensure it isn't undermined elsewhere.

> 
> > 
> >   security/selinux/hooks.c            | 12 ++++++++++++
> >   security/selinux/include/classmap.h |  2 +-
> >   2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index e67a526..5432628 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -3550,6 +3550,18 @@ static int selinux_mmap_addr(unsigned long
> > addr)
> >   static int selinux_mmap_file(struct file *file, unsigned long
> > reqprot,
> >   			     unsigned long prot, unsigned long
> > flags)
> >   {
> > +	struct common_audit_data ad;
> > +	int rc;
> > +
> > +	if (file) {
> > +		ad.type = LSM_AUDIT_DATA_FILE;
> > +		ad.u.file = file;
> > +		rc = inode_has_perm(current_cred(),
> > file_inode(file),
> > +				    FILE__MAP,&ad);
> > +		if (rc)
> > +			return rc;
> > +	}
> > +
> >   	if (selinux_checkreqprot)
> >   		prot = reqprot;
> > 
> > diff --git a/security/selinux/include/classmap.h
> > b/security/selinux/include/classmap.h
> > index 1e0cc9b..3e49a78 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -1,7 +1,7 @@
> >   #include<linux/capability.h>
> > 
> >   #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write",
> > "create", \
> > -    "getattr", "setattr", "lock", "relabelfrom", "relabelto",
> > "append"
> > +    "getattr", "setattr", "lock", "relabelfrom", "relabelto",
> > "append", "map"
> > 
> >   #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink",
> > "link", \
> >       "rename", "execute", "quotaon", "mounton", "audit_access", \

Re: [RFC][PATCH] selinux: add a map permission check for mmap

[Stephen Smalley]

On Fri, 2017-05-05 at 09:14 -0400, Stephen Smalley wrote:
> Add a map permission check on mmap so that we can distinguish memory
> mapped
> access (since it has different implications for revocation). When a
> file
> is opened and then read or written via syscalls like
> read(2)/write(2),
> we revalidate access on each read/write operation via
> selinux_file_permission() and therefore can revoke access if the
> process context, the file context, or the policy changes in such a
> manner that access is no longer allowed. When a file is opened and
> then
> memory mapped via mmap(2) and then subsequently read or written
> directly
> in memory, we presently have no way to revalidate or revoke access.
> The purpose of a separate map permission check on mmap(2) is to
> permit
> policy to prohibit memory mapping of specific files for which we need
> to ensure that every access is revalidated, particularly useful for
> scenarios where we expect the file to be relabeled at runtime in
> order
> to reflect state changes (e.g. cross-domain solution, assured
> pipeline
> without data copying).
> 
> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>

Unless you have any comments/objections, feel free to apply this as is
- I believe it is good to go, and I have also posted the corresponding
selinux-testsuite patch.

> ---
> NB I chose not to define a new policy capability for this permission,
> since it is adequately covered by handle_unknown for compatibility
> and
> others seemed to agree that this does not fall into the category of
> changes requiring a new policy capability.  I also chose to define
> the
> permission for socket classes in addition to file classes and let it
> be checked for both.
> 
>  security/selinux/hooks.c            | 12 ++++++++++++
>  security/selinux/include/classmap.h |  2 +-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index e67a526..5432628 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -3550,6 +3550,18 @@ static int selinux_mmap_addr(unsigned long
> addr)
>  static int selinux_mmap_file(struct file *file, unsigned long
> reqprot,
>  			     unsigned long prot, unsigned long
> flags)
>  {
> +	struct common_audit_data ad;
> +	int rc;
> +
> +	if (file) {
> +		ad.type = LSM_AUDIT_DATA_FILE;
> +		ad.u.file = file;
> +		rc = inode_has_perm(current_cred(),
> file_inode(file),
> +				    FILE__MAP, &ad);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	if (selinux_checkreqprot)
>  		prot = reqprot;
>  
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 1e0cc9b..3e49a78 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -1,7 +1,7 @@
>  #include <linux/capability.h>
>  
>  #define COMMON_FILE_SOCK_PERMS "ioctl", "read", "write", "create", \
> -    "getattr", "setattr", "lock", "relabelfrom", "relabelto",
> "append"
> +    "getattr", "setattr", "lock", "relabelfrom", "relabelto",
> "append", "map"
>  
>  #define COMMON_FILE_PERMS COMMON_FILE_SOCK_PERMS, "unlink", "link",
> \
>      "rename", "execute", "quotaon", "mounton", "audit_access", \

Re: [RFC][PATCH] selinux: add a map permission check for mmap

[Paul Moore]

On Fri, May 12, 2017 at 12:30 PM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Fri, 2017-05-05 at 09:14 -0400, Stephen Smalley wrote:
>> Add a map permission check on mmap so that we can distinguish memory
>> mapped
>> access (since it has different implications for revocation). When a
>> file
>> is opened and then read or written via syscalls like
>> read(2)/write(2),
>> we revalidate access on each read/write operation via
>> selinux_file_permission() and therefore can revoke access if the
>> process context, the file context, or the policy changes in such a
>> manner that access is no longer allowed. When a file is opened and
>> then
>> memory mapped via mmap(2) and then subsequently read or written
>> directly
>> in memory, we presently have no way to revalidate or revoke access.
>> The purpose of a separate map permission check on mmap(2) is to
>> permit
>> policy to prohibit memory mapping of specific files for which we need
>> to ensure that every access is revalidated, particularly useful for
>> scenarios where we expect the file to be relabeled at runtime in
>> order
>> to reflect state changes (e.g. cross-domain solution, assured
>> pipeline
>> without data copying).
>>
>> Signed-off-by: Stephen Smalley <sds@tycho.nsa.gov>
>
> Unless you have any comments/objections, feel free to apply this as is
> - I believe it is good to go, and I have also posted the corresponding
> selinux-testsuite patch.

No objections and I think the only comments I had were covered in the
policy capabilities thread (I agree that we should let handle_unknown
take care of things in this patch).

Looks good to me, merged.

-- 
paul moore
www.paul-moore.com