Re: [PATCH -v1 2/3] vm: use new has_capability_noaudit

Previous thread: none

Next thread: reiserfs_warning() spamming the syslog by Karl Kiniger on Wednesday, October 29, 2008 - 12:20 pm. (1 message)
From: Eric Paris
Date: Wednesday, October 29, 2008 - 12:06 pm

Add a new capable interface that will be used by systems that use audit to
make an A or B type decision instead of a security decision.  Currently
this is the case at least for filesystems when deciding if a process can use
the reserved 'root' blocks and for the case of things like the oom
algorithm determining if processes are root processes and should be less
likely to be killed.  These types of security system requests should not be
audited or logged since they are not really security decisions.  It would be
possible to solve this problem like the vm_enough_memory security check did
by creating a new LSM interface and moving all of the policy into that
interface but proves the needlessly bloat the LSM and provide complex
indirection.

This merely allows those decisions to be made where they belong and to not
flood logs or printk with denials for thing that are not security decisions.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 include/linux/capability.h |    1 +
 include/linux/security.h   |   12 +++++++++---
 security/commoncap.c       |    8 ++++----
 security/security.c        |    7 ++++++-
 security/selinux/hooks.c   |   20 +++++++++++++-------
 5 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/include/linux/capability.h b/include/linux/capability.h
index 9d1fe30..67b6fa9 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -514,6 +514,7 @@ kernel_cap_t cap_set_effective(const kernel_cap_t pE_new);
  * Note that this does not set PF_SUPERPRIV on the task.
  */
 #define has_capability(t, cap) (security_capable((t), (cap)) == 0)
+#define has_capability_noaudit(t, cap) (security_capable_noaudit((t), (cap)) == 0)
 
 extern int capable(int cap);
 
diff --git a/include/linux/security.h b/include/linux/security.h
index f5c4a51..88375e4 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -44,7 +44,7 @@ struct audit_krule;
  * These functions are in security/capability.c and are used
  * as the default ...
From: Eric Paris
Date: Wednesday, October 29, 2008 - 12:06 pm

The oomkiller calculations make decisions based on capabilities.  Since
these are not security decisions and LSMs should not record if they fall
the request they should use the new has_capability_noaudit() interface so
the denials will not be recorded.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 fs/proc/base.c |    2 +-
 mm/oom_kill.c  |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 486cf3f..ef83e81 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1020,7 +1020,7 @@ static ssize_t oom_adjust_write(struct file *file, const char __user *buf,
 	task = get_proc_task(file->f_path.dentry->d_inode);
 	if (!task)
 		return -ESRCH;
-	if (oom_adjust < task->oomkilladj && !capable(CAP_SYS_RESOURCE)) {
+	if (oom_adjust < task->oomkilladj && !has_capability_noaudit(current, CAP_SYS_RESOURCE)) {
 		put_task_struct(task);
 		return -EACCES;
 	}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 64e5b4b..34a458a 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -129,8 +129,8 @@ unsigned long badness(struct task_struct *p, unsigned long uptime)
 	 * Superuser processes are usually more important, so we make it
 	 * less likely that we kill those.
 	 */
-	if (has_capability(p, CAP_SYS_ADMIN) ||
-	    has_capability(p, CAP_SYS_RESOURCE))
+	if (has_capability_noaudit(p, CAP_SYS_ADMIN) ||
+	    has_capability_noaudit(p, CAP_SYS_RESOURCE))
 		points /= 4;
 
 	/*
@@ -139,7 +139,7 @@ unsigned long badness(struct task_struct *p, unsigned long uptime)
 	 * tend to only have this flag set on applications they think
 	 * of as important.
 	 */
-	if (has_capability(p, CAP_SYS_RAWIO))
+	if (has_capability_noaudit(p, CAP_SYS_RAWIO))
 		points /= 4;
 
 	/*

--

From: Stephen Smalley
Date: Wednesday, October 29, 2008 - 12:15 pm

This one looks like an actual permission check to see whether the
current task is authorized to modify this value (by writing to some proc
node).  Which should be audited.  Unlike the others, where they are
checking whether some other task has a capability in order to help
-- 
Stephen Smalley
National Security Agency

--

From: Eric Paris
Date: Wednesday, October 29, 2008 - 12:57 pm

Will be fixed in -v2

--

From: Eric Paris
Date: Wednesday, October 29, 2008 - 12:07 pm

ext2, ext3, ufs, and ubifs all check for  CAP_SYS_RESOURCE to determine
if they should allow reserved blocks to be used.  A process not having
this capability is not failing some security decision and should not be
audited.  Thus move to using has_capability_noaudit.

Signed-off-by: Eric Paris <eparis@redhat.com>
---

 fs/ext2/balloc.c     |    3 ++-
 fs/ext3/balloc.c     |    3 ++-
 fs/ubifs/budget.c    |    4 +++-
 fs/ufs/balloc.c      |    3 ++-
 security/commoncap.c |    1 +
 security/security.c  |    1 +
 6 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/fs/ext2/balloc.c b/fs/ext2/balloc.c
index 6dac7ba..bf34375 100644
--- a/fs/ext2/balloc.c
+++ b/fs/ext2/balloc.c
@@ -16,6 +16,7 @@
 #include <linux/sched.h>
 #include <linux/buffer_head.h>
 #include <linux/capability.h>
+#include <linux/security.h>
 
 /*
  * balloc.c contains the blocks allocation and deallocation routines
@@ -1192,7 +1193,7 @@ static int ext2_has_free_blocks(struct ext2_sb_info *sbi)
 
 	free_blocks = percpu_counter_read_positive(&sbi->s_freeblocks_counter);
 	root_blocks = le32_to_cpu(sbi->s_es->s_r_blocks_count);
-	if (free_blocks < root_blocks + 1 && !capable(CAP_SYS_RESOURCE) &&
+	if (free_blocks < root_blocks + 1 && !has_capability_noaudit(current, CAP_SYS_RESOURCE) &&
 		sbi->s_resuid != current->fsuid &&
 		(sbi->s_resgid == 0 || !in_group_p (sbi->s_resgid))) {
 		return 0;
diff --git a/fs/ext3/balloc.c b/fs/ext3/balloc.c
index f5b57a2..e60dd8e 100644
--- a/fs/ext3/balloc.c
+++ b/fs/ext3/balloc.c
@@ -13,6 +13,7 @@
 
 #include <linux/time.h>
 #include <linux/capability.h>
+#include <linux/security.h>
 #include <linux/fs.h>
 #include <linux/jbd.h>
 #include <linux/ext3_fs.h>
@@ -1421,7 +1422,7 @@ static int ext3_has_free_blocks(struct ext3_sb_info *sbi)
 
 	free_blocks = percpu_counter_read_positive(&sbi->s_freeblocks_counter);
 	root_blocks = le32_to_cpu(sbi->s_es->s_r_blocks_count);
-	if (free_blocks < root_blocks + 1 && !capable(CAP_SYS_RESOURCE) &&
+	if ...
From: Serge E. Hallyn
Date: Thursday, October 30, 2008 - 8:29 am

Please introduce some meaningful defines instead of passing 0 and 1.
I.e.

#define CAP_NOAUDIT 0
#define CAP_AUDIT 1

Otherwise, looks fine.

thanks,
-serge
--

From: Paul Moore
Date: Thursday, October 30, 2008 - 9:46 am

As a general rule aren't boolean arguments like this frowned upon, with 
variations on the function preferred, i.e. something like below?

 int cap_capable(struct task_struct *tsk, int cap);
 int cap_capable_audit(struct task_struct *tsk, int cap);

-- 
paul moore
linux @ hp
--

From: Eric Paris
Date: Thursday, October 30, 2008 - 10:17 am

Well from outside the "security" subsystem people should call either

has_capability()
has_capability_noaudit()
or
capable()   (which calls has_capability())

How far down do I have to keep duplicating functionality to avoid
booleans?

-Eric

--

From: Paul Moore
Date: Thursday, October 30, 2008 - 10:29 am

Probably not this far :)  Sorry, reading mail too quickly ...

-- 
paul moore
linux @ hp
--

Previous thread: none

Next thread: reiserfs_warning() spamming the syslog by Karl Kiniger on Wednesday, October 29, 2008 - 12:20 pm. (1 message)