LinuxLists.cc - [PATCH -v3 1/3] Capabilities: move cap_file

[permalink] [raw]

Subject: [PATCH -v3 3/3] Security/SELinux: seperate lsm specific mmap_min_addr

Currently SELinux enforcement of controls on the ability to map low memory
is determined by the mmap_min_addr tunable. This patch causes SELinux to
ignore the tunable and instead use a seperate Kconfig option specific to how
much space the LSM should protect.

The tunable will now only control the need for CAP_SYS_RAWIO and SELinux
permissions will always protect the amount of low memory designated by
DEFAULT_LSM_MMAP_MIN_ADDR.

This allows users who need to disable the mmap_min_addr controls (usual reason
being they run WINE as a non-root user) to do so and still have SELinux
controls preventing confined domains (like a web server) from being able to
map some area of low memory.

Signed-off-by: Eric Paris <[email protected]>
---

include/linux/mm.h | 15 --------------
include/linux/security.h | 17 ++++++++++++++++
kernel/sysctl.c | 7 ++++---
mm/Kconfig | 6 +++---
mm/mmap.c | 3 ---
mm/nommu.c | 3 ---
security/Kconfig | 16 +++++++++++++++
security/Makefile | 2 +-
security/commoncap.c | 2 +-
security/min_addr.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++
security/selinux/hooks.c | 2 +-
11 files changed, 92 insertions(+), 30 deletions(-)
create mode 100644 security/min_addr.c

diff --git a/include/linux/mm.h b/include/linux/mm.h
index ba3a7cb..9a72cc7 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -34,8 +34,6 @@ extern int sysctl_legacy_va_layout;
#define sysctl_legacy_va_layout 0
#endif

-extern unsigned long mmap_min_addr;
-
#include <asm/page.h>
#include <asm/pgtable.h>
#include <asm/processor.h>
@@ -575,19 +573,6 @@ static inline void set_page_links(struct page *page, enum zone_type zone,
}

/*
- * If a hint addr is less than mmap_min_addr change hint to be as
- * low as possible but still greater than mmap_min_addr
- */
-static inline unsigned long round_hint_to_min(unsigned long hint)
-{
- hint &= PAGE_MASK;
- if (((void *)hint != NULL) &&
- (hint < mmap_min_addr))
- return PAGE_ALIGN(mmap_min_addr);
- return hint;
-}
-
-/*
* Some inline functions in vmstat.h depend on page_zone()
*/
#include <linux/vmstat.h>
diff --git a/include/linux/security.h b/include/linux/security.h
index 963a48f..7b43115 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -28,6 +28,7 @@
#include <linux/resource.h>
#include <linux/sem.h>
#include <linux/shm.h>
+#include <linux/mm.h> /* PAGE_ALIGN */
#include <linux/msg.h>
#include <linux/sched.h>
#include <linux/key.h>
@@ -95,6 +96,7 @@ extern int cap_netlink_send(struct sock *sk, struct sk_buff *skb);
extern int cap_netlink_recv(struct sk_buff *skb, int cap);

extern unsigned long mmap_min_addr;
+extern unsigned long dac_mmap_min_addr;
/*
* Values used in the task_security_ops calls
*/
@@ -147,6 +149,21 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
opts->num_mnt_opts = 0;
}

+/*
+ * If a hint addr is less than mmap_min_addr change hint to be as
+ * low as possible but still greater than mmap_min_addr
+ */
+static inline unsigned long round_hint_to_min(unsigned long hint)
+{
+ hint &= PAGE_MASK;
+ if (((void *)hint != NULL) &&
+ (hint < mmap_min_addr))
+ return PAGE_ALIGN(mmap_min_addr);
+ return hint;
+}
+
+extern int mmap_min_addr_handler(struct ctl_table *table, int write, struct file *filp,
+ void __user *buffer, size_t *lenp, loff_t *ppos);
/**
* struct security_operations - main security structure
*
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 98e0232..58be760 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -49,6 +49,7 @@
#include <linux/acpi.h>
#include <linux/reboot.h>
#include <linux/ftrace.h>
+#include <linux/security.h>
#include <linux/slow-work.h>
#include <linux/perf_counter.h>

@@ -1306,10 +1307,10 @@ static struct ctl_table vm_table[] = {
{
.ctl_name = CTL_UNNUMBERED,
.procname = "mmap_min_addr",
- .data = &mmap_min_addr,
- .maxlen = sizeof(unsigned long),
+ .data = &dac_mmap_min_addr,
+ .maxlen = sizeof(unsigned long),
.mode = 0644,
- .proc_handler = &proc_doulongvec_minmax,
+ .proc_handler = &mmap_min_addr_handler,
},
#ifdef CONFIG_NUMA
{
diff --git a/mm/Kconfig b/mm/Kconfig
index c948d4c..fe5f674 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -225,9 +225,9 @@ config DEFAULT_MMAP_MIN_ADDR
For most ia64, ppc64 and x86 users with lots of address space
a value of 65536 is reasonable and should cause no problems.
On arm and other archs it should not be higher than 32768.
- Programs which use vm86 functionality would either need additional
- permissions from either the LSM or the capabilities module or have
- this protection disabled.
+ Programs which use vm86 functionality or have some need to map
+ this low address space will need CAP_SYS_RAWIO or disable this
+ protection by setting the value to 0.

This value can be changed after boot using the
/proc/sys/vm/mmap_min_addr tunable.
diff --git a/mm/mmap.c b/mm/mmap.c
index 34579b2..8101de4 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -88,9 +88,6 @@ int sysctl_overcommit_ratio = 50; /* default is 50% */
int sysctl_max_map_count __read_mostly = DEFAULT_MAX_MAP_COUNT;
struct percpu_counter vm_committed_as;

-/* amount of vm to protect from userspace access */
-unsigned long mmap_min_addr = CONFIG_DEFAULT_MMAP_MIN_ADDR;
-
/*
* Check that a process has enough memory to allocate a new virtual
* mapping. 0 means there is enough memory for the allocation to
diff --git a/mm/nommu.c b/mm/nommu.c
index 53cab10..28754c4 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -69,9 +69,6 @@ int sysctl_max_map_count = DEFAULT_MAX_MAP_COUNT;
int sysctl_nr_trim_pages = CONFIG_NOMMU_INITIAL_TRIM_EXCESS;
int heap_stack_gap = 0;

-/* amount of vm to protect from userspace access */
-unsigned long mmap_min_addr = CONFIG_DEFAULT_MMAP_MIN_ADDR;
-
atomic_long_t mmap_pages_allocated;

EXPORT_SYMBOL(mem_map);
diff --git a/security/Kconfig b/security/Kconfig
index d23c839..9c60c34 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -113,6 +113,22 @@ config SECURITY_ROOTPLUG

If you are unsure how to answer this question, answer N.

+config LSM_MMAP_MIN_ADDR
+ int "Low address space for LSM to from user allocation"
+ depends on SECURITY && SECURITY_SELINUX
+ default 65535
+ help
+ This is the portion of low virtual memory which should be protected
+ from userspace allocation. Keeping a user from writing to low pages
+ can help reduce the impact of kernel NULL pointer bugs.
+
+ For most ia64, ppc64 and x86 users with lots of address space
+ a value of 65536 is reasonable and should cause no problems.
+ On arm and other archs it should not be higher than 32768.
+ Programs which use vm86 functionality or have some need to map
+ this low address space will need the permission specific to the
+ systems running LSM.
+
source security/selinux/Kconfig
source security/smack/Kconfig
source security/tomoyo/Kconfig
diff --git a/security/Makefile b/security/Makefile
index c67557c..b56e7f9 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -8,7 +8,7 @@ subdir-$(CONFIG_SECURITY_SMACK) += smack
subdir-$(CONFIG_SECURITY_TOMOYO) += tomoyo

# always enable default capabilities
-obj-y += commoncap.o
+obj-y += commoncap.o min_addr.o

# Object file lists
obj-$(CONFIG_SECURITY) += security.o capability.o
diff --git a/security/commoncap.c b/security/commoncap.c
index 9a731d7..0269b5c 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -1003,7 +1003,7 @@ int cap_file_mmap(struct file *file, unsigned long reqprot,
unsigned long prot, unsigned long flags,
unsigned long addr, unsigned long addr_only)
{
- if (addr < mmap_min_addr)
+ if (addr < dac_mmap_min_addr)
return cap_capable(current, current_cred(), CAP_SYS_RAWIO,
SECURITY_CAP_AUDIT);
return 0;
diff --git a/security/min_addr.c b/security/min_addr.c
new file mode 100644
index 0000000..14cc7b3
--- /dev/null
+++ b/security/min_addr.c
@@ -0,0 +1,49 @@
+#include <linux/init.h>
+#include <linux/mm.h>
+#include <linux/security.h>
+#include <linux/sysctl.h>
+
+/* amount of vm to protect from userspace access by both DAC and the LSM*/
+unsigned long mmap_min_addr;
+/* amount of vm to protect from userspace using CAP_SYS_RAWIO (DAC) */
+unsigned long dac_mmap_min_addr = CONFIG_DEFAULT_MMAP_MIN_ADDR;
+/* amount of vm to protect from userspace using the LSM = CONFIG_LSM_MMAP_MIN_ADDR */
+
+/*
+ * Update mmap_min_addr = max(dac_mmap_min_addr, CONFIG_LSM_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)
+ mmap_min_addr = dac_mmap_min_addr;
+ else
+ mmap_min_addr = CONFIG_LSM_MMAP_MIN_ADDR;
+#else
+ mmap_min_addr = dac_mmap_min_addr;
+#endif
+}
+
+/*
+ * sysctl handler which just sets dac_mmap_min_addr = the new value and then
+ * calls update_mmap_min_addr() so non MAP_FIXED hints get rounded properly
+ */
+int mmap_min_addr_handler(struct ctl_table *table, int write, struct file *filp,
+ void __user *buffer, size_t *lenp, loff_t *ppos)
+{
+ int ret;
+
+ ret = proc_doulongvec_minmax(table, write, filp, buffer, lenp, ppos);
+
+ update_mmap_min_addr();
+
+ return ret;
+}
+
+int __init init_mmap_min_addr(void)
+{
+ update_mmap_min_addr();
+
+ return 0;
+}
+pure_initcall(init_mmap_min_addr);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8a78f58..5dee883 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3040,7 +3040,7 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
* at bad behaviour/exploit that we always want to get the AVC, even
* if DAC would have also denied the operation.
*/
- if (addr < mmap_min_addr) {
+ if (addr < CONFIG_LSM_MMAP_MIN_ADDR) {
rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
MEMPROTECT__MMAP_ZERO, NULL);
if (rc)

2009-07-29 18:57:11

[permalink] [raw]

Subject: [PATCH -v3 2/3] SELinux: call cap_file_mmap in selinux_file_mmap

Currently SELinux does not check CAP_SYS_RAWIO in the file_mmap hook. This
means there is no DAC check on the ability to mmap low addresses in the
memory space. This function adds the DAC check for CAP_SYS_RAWIO while
maintaining the selinux check on mmap_zero. This means that processes
which need to mmap low memory will need CAP_SYS_RAWIO and mmap_zero but will
NOT need the SELinux sys_rawio capability.

Signed-off-by: Eric Paris <[email protected]>
---

security/selinux/hooks.c | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index e65677d..8a78f58 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3034,9 +3034,21 @@ static int selinux_file_mmap(struct file *file, unsigned long reqprot,
int rc = 0;
u32 sid = current_sid();

- if (addr < mmap_min_addr)
+ /*
+ * 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 < mmap_min_addr) {
rc = avc_has_perm(sid, sid, SECCLASS_MEMPROTECT,
MEMPROTECT__MMAP_ZERO, NULL);
+ if (rc)
+ return rc;
+ }
+
+ /* do DAC check on address space usage */
+ rc = cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
if (rc || addr_only)
return rc;

2009-07-30 05:14:30

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

Quoting Eric Paris ([email protected]):
> Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> into commoncap.c and then calls that function directly from
> security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> checks are done.

It also

1. changes the return value in error case from -EACCES to
-EPERM
2. no onger sets PF_SUPERPRIV in t->flags if the capability
is used.

Do we care about these?

-serge

> Signed-off-by: Eric Paris <[email protected]>
> ---
>
> include/linux/security.h | 7 ++++---
> security/capability.c | 9 ---------
> security/commoncap.c | 24 ++++++++++++++++++++++++
> 3 files changed, 28 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1459091..963a48f 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -66,6 +66,9 @@ extern int cap_inode_setxattr(struct dentry *dentry, const char *name,
> extern int cap_inode_removexattr(struct dentry *dentry, const char *name);
> extern int cap_inode_need_killpriv(struct dentry *dentry);
> extern int cap_inode_killpriv(struct dentry *dentry);
> +extern int cap_file_mmap(struct file *file, unsigned long reqprot,
> + unsigned long prot, unsigned long flags,
> + unsigned long addr, unsigned long addr_only);
> extern int cap_task_fix_setuid(struct cred *new, const struct cred *old, int flags);
> extern int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
> unsigned long arg4, unsigned long arg5);
> @@ -2197,9 +2200,7 @@ static inline int security_file_mmap(struct file *file, unsigned long reqprot,
> unsigned long addr,
> unsigned long addr_only)
> {
> - if ((addr < mmap_min_addr) && !capable(CAP_SYS_RAWIO))
> - return -EACCES;
> - return 0;
> + return cap_file_mmap(file, reqprot, prot, flags, addr, addr_only);
> }
>
> static inline int security_file_mprotect(struct vm_area_struct *vma,
> diff --git a/security/capability.c b/security/capability.c
> index f218dd3..ec05730 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -330,15 +330,6 @@ static int cap_file_ioctl(struct file *file, unsigned int command,
> return 0;
> }
>
> -static int cap_file_mmap(struct file *file, unsigned long reqprot,
> - unsigned long prot, unsigned long flags,
> - unsigned long addr, unsigned long addr_only)
> -{
> - if ((addr < mmap_min_addr) && !capable(CAP_SYS_RAWIO))
> - return -EACCES;
> - return 0;
> -}
> -
> static int cap_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
> unsigned long prot)
> {
> diff --git a/security/commoncap.c b/security/commoncap.c
> index aa97704..9a731d7 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -984,3 +984,27 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
> cap_sys_admin = 1;
> return __vm_enough_memory(mm, pages, cap_sys_admin);
> }
> +
> +/*
> + * cap_file_mmap - check if able to map given addr
> + * @file: unused
> + * @reqprot: unused
> + * @prot: unused
> + * @flags: unused
> + * @addr: address attempting to be mapped
> + * @addr_only: unused
> + *
> + * If the process is attempting to map memory below mmap_min_addr they need
> + * CAP_SYS_RAWIO. The other parameters to this function are unused by the
> + * capability security module. Returns 0 if this mapping should be allowed
> + * -EPERM if not.
> + */
> +int cap_file_mmap(struct file *file, unsigned long reqprot,
> + unsigned long prot, unsigned long flags,
> + unsigned long addr, unsigned long addr_only)
> +{
> + if (addr < mmap_min_addr)
> + return cap_capable(current, current_cred(), CAP_SYS_RAWIO,
> + SECURITY_CAP_AUDIT);
> + return 0;
> +}
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2009-07-30 15:42:10

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> Quoting Eric Paris ([email protected]):
> > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > into commoncap.c and then calls that function directly from
> > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > checks are done.
>
> It also
>
> 1. changes the return value in error case from -EACCES to
> -EPERM
> 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> is used.
>
> Do we care about these?

Personally, not really, but I'll gladly put them back if you care. #2
seems more interesting to me than number 1. I actually kinda like
getting EPERM from caps rather than EACCES since them I know if I was
denied by selinux or by caps.....

-Eric

2009-07-30 15:54:39

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

Quoting Eric Paris ([email protected]):
> On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > Quoting Eric Paris ([email protected]):
> > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > into commoncap.c and then calls that function directly from
> > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > checks are done.
> >
> > It also
> >
> > 1. changes the return value in error case from -EACCES to
> > -EPERM
> > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > is used.
> >
> > Do we care about these?
>
> Personally, not really, but I'll gladly put them back if you care. #2
> seems more interesting to me than number 1. I actually kinda like
> getting EPERM from caps rather than EACCES since them I know if I was
> denied by selinux or by caps.....
>
> -Eric

Yup, I asked bc I didn't particularly care myself.

I think I agree with you about -EPERM being better anyway. However I
(now) think in this case PF_SUPERPRIV definately should be set, as this
is a clear use of a capability to do something that couldn't have been
done without it.

thanks,
-serge

2009-07-30 15:59:12

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> Quoting Eric Paris ([email protected]):
> > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > Quoting Eric Paris ([email protected]):
> > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > into commoncap.c and then calls that function directly from
> > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > checks are done.
> > >
> > > It also
> > >
> > > 1. changes the return value in error case from -EACCES to
> > > -EPERM
> > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > is used.
> > >
> > > Do we care about these?
> >
> > Personally, not really, but I'll gladly put them back if you care. #2
> > seems more interesting to me than number 1. I actually kinda like
> > getting EPERM from caps rather than EACCES since them I know if I was
> > denied by selinux or by caps.....
> >
> > -Eric
>
> Yup, I asked bc I didn't particularly care myself.
>
> I think I agree with you about -EPERM being better anyway. However I
> (now) think in this case PF_SUPERPRIV definately should be set, as this
> is a clear use of a capability to do something that couldn't have been
> done without it.

On a related but different note, we should consider all current uses of
cap_capable(), as they represent capability checks that will not be
subject to a further restrictive check by other security modules. In
this case and in the vm_enough_memory case, that is intentional, but not
so clear for other uses in commoncap.c.

--
Stephen Smalley
National Security Agency

2009-07-30 17:52:02

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 11:58 -0400, Stephen Smalley wrote:
> On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> > Quoting Eric Paris ([email protected]):
> > > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > > Quoting Eric Paris ([email protected]):
> > > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > > into commoncap.c and then calls that function directly from
> > > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > > checks are done.
> > > >
> > > > It also
> > > >
> > > > 1. changes the return value in error case from -EACCES to
> > > > -EPERM
> > > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > > is used.
> > > >
> > > > Do we care about these?
> > >
> > > Personally, not really, but I'll gladly put them back if you care. #2
> > > seems more interesting to me than number 1. I actually kinda like
> > > getting EPERM from caps rather than EACCES since them I know if I was
> > > denied by selinux or by caps.....
> > >
> > > -Eric
> >
> > Yup, I asked bc I didn't particularly care myself.
> >
> > I think I agree with you about -EPERM being better anyway. However I
> > (now) think in this case PF_SUPERPRIV definately should be set, as this
> > is a clear use of a capability to do something that couldn't have been
> > done without it.
>
> On a related but different note, we should consider all current uses of
> cap_capable(), as they represent capability checks that will not be
> subject to a further restrictive check by other security modules. In
> this case and in the vm_enough_memory case, that is intentional, but not
> so clear for other uses in commoncap.c.

Most of commoncap.c is called either as a secondary hook from the active
lsm (aka selinux calls the commoncap.c functions) or in the !
CONFIG_SECURITY case.

I'll audit this afternoon to see which of them might not fit these
rules....

-Eric

2009-07-30 17:54:06

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> Quoting Eric Paris ([email protected]):
> > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > Quoting Eric Paris ([email protected]):
> > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > into commoncap.c and then calls that function directly from
> > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > checks are done.
> > >
> > > It also
> > >
> > > 1. changes the return value in error case from -EACCES to
> > > -EPERM
> > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > is used.
> > >
> > > Do we care about these?
> >
> > Personally, not really, but I'll gladly put them back if you care. #2
> > seems more interesting to me than number 1. I actually kinda like
> > getting EPERM from caps rather than EACCES since them I know if I was
> > denied by selinux or by caps.....
> >
> > -Eric
>
> Yup, I asked bc I didn't particularly care myself.
>
> I think I agree with you about -EPERM being better anyway. However I
> (now) think in this case PF_SUPERPRIV definately should be set, as this
> is a clear use of a capability to do something that couldn't have been
> done without it.

Easy enough, if I add PF_SUPERPRIV can I add your ACK? Basically just

ret = cap_capable();
if (!ret)
current->flags |= PF_SUPERPRIV;

return ret;

2009-07-30 18:32:56

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 13:50 -0400, Eric Paris wrote:
> On Thu, 2009-07-30 at 11:58 -0400, Stephen Smalley wrote:
> > On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> > > Quoting Eric Paris ([email protected]):
> > > > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > > > Quoting Eric Paris ([email protected]):
> > > > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > > > into commoncap.c and then calls that function directly from
> > > > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > > > checks are done.
> > > > >
> > > > > It also
> > > > >
> > > > > 1. changes the return value in error case from -EACCES to
> > > > > -EPERM
> > > > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > > > is used.
> > > > >
> > > > > Do we care about these?
> > > >
> > > > Personally, not really, but I'll gladly put them back if you care. #2
> > > > seems more interesting to me than number 1. I actually kinda like
> > > > getting EPERM from caps rather than EACCES since them I know if I was
> > > > denied by selinux or by caps.....
> > > >
> > > > -Eric
> > >
> > > Yup, I asked bc I didn't particularly care myself.
> > >
> > > I think I agree with you about -EPERM being better anyway. However I
> > > (now) think in this case PF_SUPERPRIV definately should be set, as this
> > > is a clear use of a capability to do something that couldn't have been
> > > done without it.
> >
> > On a related but different note, we should consider all current uses of
> > cap_capable(), as they represent capability checks that will not be
> > subject to a further restrictive check by other security modules. In
> > this case and in the vm_enough_memory case, that is intentional, but not
> > so clear for other uses in commoncap.c.
>
> Most of commoncap.c is called either as a secondary hook from the active
> lsm (aka selinux calls the commoncap.c functions) or in the !
> CONFIG_SECURITY case.
>
> I'll audit this afternoon to see which of them might not fit these
> rules....

I just went through all of the cap_* function in commoncap.c to see
which of them are being or are not being called from the selinux hooks.
Only 3 of them look interesting.

cap_inode_setxattr
cap_inode_removexattr
cap_vm_enough_memory

All of the other functions are either called from hooks.c or SELinux
does not define that LSM hook, so it just defaults to the cap_* hook.

These 3 are all a bit odd because the logic inside the cap_ hook is
duplicated inside the selinux_ hook. I'd much rather see the selinux_
hook call the cap_ hook. I'm going to think on that topic, but it's a
different set of patches and I don't see missing checks today as the
logic seems to line up.

-Eric

2009-07-30 19:41:56

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

Quoting Eric Paris ([email protected]):
> On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> > Quoting Eric Paris ([email protected]):
> > > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > > Quoting Eric Paris ([email protected]):
> > > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > > into commoncap.c and then calls that function directly from
> > > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > > checks are done.
> > > >
> > > > It also
> > > >
> > > > 1. changes the return value in error case from -EACCES to
> > > > -EPERM
> > > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > > is used.
> > > >
> > > > Do we care about these?
> > >
> > > Personally, not really, but I'll gladly put them back if you care. #2
> > > seems more interesting to me than number 1. I actually kinda like
> > > getting EPERM from caps rather than EACCES since them I know if I was
> > > denied by selinux or by caps.....
> > >
> > > -Eric
> >
> > Yup, I asked bc I didn't particularly care myself.
> >
> > I think I agree with you about -EPERM being better anyway. However I
> > (now) think in this case PF_SUPERPRIV definately should be set, as this
> > is a clear use of a capability to do something that couldn't have been
> > done without it.
>
> Easy enough, if I add PF_SUPERPRIV can I add your ACK? Basically just

Yup.

> ret = cap_capable();
> if (!ret)
> current->flags |= PF_SUPERPRIV;
>
> return ret;

Yup. (Maybe spell out 'if (ret == 0)' to help people keep straight
that 0 means ok with cap_capable(), but it's up to you)

thanks,
-serge

2009-07-30 19:43:26

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 13:50 -0400, Eric Paris wrote:
> On Thu, 2009-07-30 at 11:58 -0400, Stephen Smalley wrote:
> > On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> > > Quoting Eric Paris ([email protected]):
> > > > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > > > Quoting Eric Paris ([email protected]):
> > > > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > > > into commoncap.c and then calls that function directly from
> > > > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > > > checks are done.
> > > > >
> > > > > It also
> > > > >
> > > > > 1. changes the return value in error case from -EACCES to
> > > > > -EPERM
> > > > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > > > is used.
> > > > >
> > > > > Do we care about these?
> > > >
> > > > Personally, not really, but I'll gladly put them back if you care. #2
> > > > seems more interesting to me than number 1. I actually kinda like
> > > > getting EPERM from caps rather than EACCES since them I know if I was
> > > > denied by selinux or by caps.....
> > > >
> > > > -Eric
> > >
> > > Yup, I asked bc I didn't particularly care myself.
> > >
> > > I think I agree with you about -EPERM being better anyway. However I
> > > (now) think in this case PF_SUPERPRIV definately should be set, as this
> > > is a clear use of a capability to do something that couldn't have been
> > > done without it.
> >
> > On a related but different note, we should consider all current uses of
> > cap_capable(), as they represent capability checks that will not be
> > subject to a further restrictive check by other security modules. In
> > this case and in the vm_enough_memory case, that is intentional, but not
> > so clear for other uses in commoncap.c.
>
> Most of commoncap.c is called either as a secondary hook from the active
> lsm (aka selinux calls the commoncap.c functions) or in the !
> CONFIG_SECURITY case.
>
> I'll audit this afternoon to see which of them might not fit these
> rules....

That isn't what I meant. Most of the commoncap functions call capable()
rather than directly calling cap_capable(), thereby causing:
- PF_SUPERPRIV to be set, and
- The primary security module (e.g. SELinux) to apply its own
restrictive check.

That is useful as it allows SELinux or AppArmor or TOMOYO to veto e.g.
CAP_SYS_PTRACE without replicating the same logic within its own hook.

The current exceptions are:
cap_inh_is_capped() called from cap_capset(),
cap_task_prctl() in the PR_SET_SECUREBITS case,
cap_vm_enough_memory(),
cap_file_mmap() after your patch.

The latter two are indeed cases where we made a conscious choice that
SELinux would not apply its capability check against policy. But the
first two are unclear to me.

--
Stephen Smalley
National Security Agency

2009-07-30 19:48:08

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 14:31 -0400, Eric Paris wrote:
> On Thu, 2009-07-30 at 13:50 -0400, Eric Paris wrote:
> > On Thu, 2009-07-30 at 11:58 -0400, Stephen Smalley wrote:
> > > On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> > > > Quoting Eric Paris ([email protected]):
> > > > > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > > > > Quoting Eric Paris ([email protected]):
> > > > > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > > > > into commoncap.c and then calls that function directly from
> > > > > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > > > > checks are done.
> > > > > >
> > > > > > It also
> > > > > >
> > > > > > 1. changes the return value in error case from -EACCES to
> > > > > > -EPERM
> > > > > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > > > > is used.
> > > > > >
> > > > > > Do we care about these?
> > > > >
> > > > > Personally, not really, but I'll gladly put them back if you care. #2
> > > > > seems more interesting to me than number 1. I actually kinda like
> > > > > getting EPERM from caps rather than EACCES since them I know if I was
> > > > > denied by selinux or by caps.....
> > > > >
> > > > > -Eric
> > > >
> > > > Yup, I asked bc I didn't particularly care myself.
> > > >
> > > > I think I agree with you about -EPERM being better anyway. However I
> > > > (now) think in this case PF_SUPERPRIV definately should be set, as this
> > > > is a clear use of a capability to do something that couldn't have been
> > > > done without it.
> > >
> > > On a related but different note, we should consider all current uses of
> > > cap_capable(), as they represent capability checks that will not be
> > > subject to a further restrictive check by other security modules. In
> > > this case and in the vm_enough_memory case, that is intentional, but not
> > > so clear for other uses in commoncap.c.
> >
> > Most of commoncap.c is called either as a secondary hook from the active
> > lsm (aka selinux calls the commoncap.c functions) or in the !
> > CONFIG_SECURITY case.
> >
> > I'll audit this afternoon to see which of them might not fit these
> > rules....
>
> I just went through all of the cap_* function in commoncap.c to see
> which of them are being or are not being called from the selinux hooks.
> Only 3 of them look interesting.
>
> cap_inode_setxattr
> cap_inode_removexattr
> cap_vm_enough_memory
>
> All of the other functions are either called from hooks.c or SELinux
> does not define that LSM hook, so it just defaults to the cap_* hook.
>
> These 3 are all a bit odd because the logic inside the cap_ hook is
> duplicated inside the selinux_ hook. I'd much rather see the selinux_
> hook call the cap_ hook. I'm going to think on that topic, but it's a
> different set of patches and I don't see missing checks today as the
> logic seems to line up.

Yes, those 3 aren't a problem. I suppose selinux_inode_setotherxattr()
could be changed to call cap_inode_setxattr() before the SELinux setattr
check, as that would only happen in the non-selinux attribute case.

--
Stephen Smalley
National Security Agency

2009-07-30 19:55:31

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

On Thu, 2009-07-30 at 15:42 -0400, Stephen Smalley wrote:
> On Thu, 2009-07-30 at 13:50 -0400, Eric Paris wrote:
> > On Thu, 2009-07-30 at 11:58 -0400, Stephen Smalley wrote:
> > > On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> > > > Quoting Eric Paris ([email protected]):
> > > > > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > > > > Quoting Eric Paris ([email protected]):
> > > > > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > > > > into commoncap.c and then calls that function directly from
> > > > > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > > > > checks are done.
> > > > > >
> > > > > > It also
> > > > > >
> > > > > > 1. changes the return value in error case from -EACCES to
> > > > > > -EPERM
> > > > > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > > > > is used.
> > > > > >
> > > > > > Do we care about these?
> > > > >
> > > > > Personally, not really, but I'll gladly put them back if you care. #2
> > > > > seems more interesting to me than number 1. I actually kinda like
> > > > > getting EPERM from caps rather than EACCES since them I know if I was
> > > > > denied by selinux or by caps.....
> > > > >
> > > > > -Eric
> > > >
> > > > Yup, I asked bc I didn't particularly care myself.
> > > >
> > > > I think I agree with you about -EPERM being better anyway. However I
> > > > (now) think in this case PF_SUPERPRIV definately should be set, as this
> > > > is a clear use of a capability to do something that couldn't have been
> > > > done without it.
> > >
> > > On a related but different note, we should consider all current uses of
> > > cap_capable(), as they represent capability checks that will not be
> > > subject to a further restrictive check by other security modules. In
> > > this case and in the vm_enough_memory case, that is intentional, but not
> > > so clear for other uses in commoncap.c.
> >
> > Most of commoncap.c is called either as a secondary hook from the active
> > lsm (aka selinux calls the commoncap.c functions) or in the !
> > CONFIG_SECURITY case.
> >
> > I'll audit this afternoon to see which of them might not fit these
> > rules....
>
> That isn't what I meant. Most of the commoncap functions call capable()
> rather than directly calling cap_capable(), thereby causing:
> - PF_SUPERPRIV to be set, and
> - The primary security module (e.g. SELinux) to apply its own
> restrictive check.
>
> That is useful as it allows SELinux or AppArmor or TOMOYO to veto e.g.
> CAP_SYS_PTRACE without replicating the same logic within its own hook.
>
> The current exceptions are:
> cap_inh_is_capped() called from cap_capset(),
> cap_task_prctl() in the PR_SET_SECUREBITS case,
> cap_vm_enough_memory(),
> cap_file_mmap() after your patch.
>
> The latter two are indeed cases where we made a conscious choice that
> SELinux would not apply its capability check against policy. But the
> first two are unclear to me.

Sorry, the vm_enough_memory case was about auditing, not about applying
the check itself. Which might have been rendered moot by your later
changes to introduce SECURITY_CAP_NOAUDIT.

--
Stephen Smalley
National Security Agency

2009-07-30 20:01:48

[permalink] [raw]

Subject: Re: [PATCH -v3 1/3] Capabilities: move cap_file_mmap to commoncap.c

Quoting Stephen Smalley ([email protected]):
> On Thu, 2009-07-30 at 13:50 -0400, Eric Paris wrote:
> > On Thu, 2009-07-30 at 11:58 -0400, Stephen Smalley wrote:
> > > On Thu, 2009-07-30 at 10:54 -0500, Serge E. Hallyn wrote:
> > > > Quoting Eric Paris ([email protected]):
> > > > > On Thu, 2009-07-30 at 00:14 -0500, Serge E. Hallyn wrote:
> > > > > > Quoting Eric Paris ([email protected]):
> > > > > > > Currently we duplicate the mmap_min_addr test in cap_file_mmap and in
> > > > > > > security_file_mmap if !CONFIG_SECURITY. This patch moves cap_file_mmap
> > > > > > > into commoncap.c and then calls that function directly from
> > > > > > > security_file_mmap ifndef CONFIG_SECURITY like all of the other capability
> > > > > > > checks are done.
> > > > > >
> > > > > > It also
> > > > > >
> > > > > > 1. changes the return value in error case from -EACCES to
> > > > > > -EPERM
> > > > > > 2. no onger sets PF_SUPERPRIV in t->flags if the capability
> > > > > > is used.
> > > > > >
> > > > > > Do we care about these?
> > > > >
> > > > > Personally, not really, but I'll gladly put them back if you care. #2
> > > > > seems more interesting to me than number 1. I actually kinda like
> > > > > getting EPERM from caps rather than EACCES since them I know if I was
> > > > > denied by selinux or by caps.....
> > > > >
> > > > > -Eric
> > > >
> > > > Yup, I asked bc I didn't particularly care myself.
> > > >
> > > > I think I agree with you about -EPERM being better anyway. However I
> > > > (now) think in this case PF_SUPERPRIV definately should be set, as this
> > > > is a clear use of a capability to do something that couldn't have been
> > > > done without it.
> > >
> > > On a related but different note, we should consider all current uses of
> > > cap_capable(), as they represent capability checks that will not be
> > > subject to a further restrictive check by other security modules. In
> > > this case and in the vm_enough_memory case, that is intentional, but not
> > > so clear for other uses in commoncap.c.
> >
> > Most of commoncap.c is called either as a secondary hook from the active
> > lsm (aka selinux calls the commoncap.c functions) or in the !
> > CONFIG_SECURITY case.
> >
> > I'll audit this afternoon to see which of them might not fit these
> > rules....
>
> That isn't what I meant. Most of the commoncap functions call capable()
> rather than directly calling cap_capable(), thereby causing:
> - PF_SUPERPRIV to be set, and
> - The primary security module (e.g. SELinux) to apply its own
> restrictive check.
>
> That is useful as it allows SELinux or AppArmor or TOMOYO to veto e.g.
> CAP_SYS_PTRACE without replicating the same logic within its own hook.
>
> The current exceptions are:
> cap_inh_is_capped() called from cap_capset(),
> cap_task_prctl() in the PR_SET_SECUREBITS case,
> cap_vm_enough_memory(),
> cap_file_mmap() after your patch.
>
> The latter two are indeed cases where we made a conscious choice that
> SELinux would not apply its capability check against policy. But the
> first two are unclear to me.

cap_inh_is_capped:

I'm not sure why it's cap_capable() instead of capable(). However, if we
switch to using capable(), then we should switch the conditions in the caller
around, since at the moment just because the capable() check returned true
doesn't mean that we actually end up needing it.

(CC:ing Andrew Morgan as I believe he wrote this and may have had a reason)

cap_task_prctl: I don't see any reason why that shouldn't be capable().

cap_vm_enough_memory(): I seem to recall we explicitly decided that we
did not want PF_SUPERPRIV set in this case.

cap_file_mmap(): well now that you mention it, it does seem like SELinux
would want a say in whether the task gets CAP_SYS_RAWIO here, so maybe
it should be capable() after all?

-serge

2009-07-30 20:08:16