2008-10-29 19:07:42

by Eric Paris

[permalink] [raw]
Subject: [PATCH -v1 1/3] SECURITY: new capable_noaudit interface

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 <[email protected]>
---

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 capabilities functions
*/
-extern int cap_capable(struct task_struct *tsk, int cap);
+extern int cap_capable(struct task_struct *tsk, int cap, int audit);
extern int cap_settime(struct timespec *ts, struct timezone *tz);
extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
extern int cap_ptrace_traceme(struct task_struct *parent);
@@ -1307,7 +1307,7 @@ struct security_operations {
kernel_cap_t *effective,
kernel_cap_t *inheritable,
kernel_cap_t *permitted);
- int (*capable) (struct task_struct *tsk, int cap);
+ int (*capable) (struct task_struct *tsk, int cap, int audit);
int (*acct) (struct file *file);
int (*sysctl) (struct ctl_table *table, int op);
int (*quotactl) (int cmds, int type, int id, struct super_block *sb);
@@ -1577,6 +1577,7 @@ void security_capset_set(struct task_struct *target,
kernel_cap_t *inheritable,
kernel_cap_t *permitted);
int security_capable(struct task_struct *tsk, int cap);
+int security_capable_noaudit(struct task_struct *tsk, int cap);
int security_acct(struct file *file);
int security_sysctl(struct ctl_table *table, int op);
int security_quotactl(int cmds, int type, int id, struct super_block *sb);
@@ -1781,7 +1782,12 @@ static inline void security_capset_set(struct task_struct *target,

static inline int security_capable(struct task_struct *tsk, int cap)
{
- return cap_capable(tsk, cap);
+ return cap_capable(tsk, cap, 1);
+}
+
+static inline int security_capable_noaudit(struct task_struct *tsk, int cap)
+{
+ return cap_capable(tsk, cap, 0);
}

static inline int security_acct(struct file *file)
diff --git a/security/commoncap.c b/security/commoncap.c
index 399bfdb..243e223 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -48,7 +48,7 @@ EXPORT_SYMBOL(cap_netlink_recv);
* returns 0 when a task has a capability, but the kernel's capable()
* returns 1 for this case.
*/
-int cap_capable (struct task_struct *tsk, int cap)
+int cap_capable(struct task_struct *tsk, int cap, int audit)
{
/* Derived from include/linux/sched.h:capable. */
if (cap_raised(tsk->cap_effective, cap))
@@ -111,7 +111,7 @@ static inline int cap_inh_is_capped(void)
* to the old permitted set. That is, if the current task
* does *not* possess the CAP_SETPCAP capability.
*/
- return (cap_capable(current, CAP_SETPCAP) != 0);
+ return (cap_capable(current, CAP_SETPCAP, 1) != 0);
}

static inline int cap_limit_ptraced_target(void) { return 1; }
@@ -640,7 +640,7 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
|| ((current->securebits & SECURE_ALL_LOCKS
& ~arg2)) /*[2]*/
|| (arg2 & ~(SECURE_ALL_LOCKS | SECURE_ALL_BITS)) /*[3]*/
- || (cap_capable(current, CAP_SETPCAP) != 0)) { /*[4]*/
+ || (cap_capable(current, CAP_SETPCAP, 1) != 0)) { /*[4]*/
/*
* [1] no changing of bits that are locked
* [2] no unlocking of locks
@@ -705,7 +705,7 @@ int cap_vm_enough_memory(struct mm_struct *mm, long pages)
{
int cap_sys_admin = 0;

- if (cap_capable(current, CAP_SYS_ADMIN) == 0)
+ if (cap_capable(current, CAP_SYS_ADMIN, 0) == 0)
cap_sys_admin = 1;
return __vm_enough_memory(mm, pages, cap_sys_admin);
}
diff --git a/security/security.c b/security/security.c
index 255b085..271f9a7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -163,7 +163,12 @@ void security_capset_set(struct task_struct *target,

int security_capable(struct task_struct *tsk, int cap)
{
- return security_ops->capable(tsk, cap);
+ return security_ops->capable(tsk, cap, 1);
+}
+
+int security_capable_noaudit(struct task_struct *tsk, int cap)
+{
+ return security_ops->capable(tsk, cap, 0);
}

int security_acct(struct file *file)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 3e3fde7..74a28bc 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1365,12 +1365,14 @@ static int task_has_perm(struct task_struct *tsk1,

/* Check whether a task is allowed to use a capability. */
static int task_has_capability(struct task_struct *tsk,
- int cap)
+ int cap, int audit)
{
struct task_security_struct *tsec;
struct avc_audit_data ad;
+ struct av_decision avd;
u16 sclass;
u32 av = CAP_TO_MASK(cap);
+ int rc;

tsec = tsk->security;

@@ -1390,7 +1392,11 @@ static int task_has_capability(struct task_struct *tsk,
"SELinux: out of range capability %d\n", cap);
BUG();
}
- return avc_has_perm(tsec->sid, tsec->sid, sclass, av, &ad);
+
+ rc = avc_has_perm_noaudit(tsec->sid, tsec->sid, sclass, av, 0, &avd);
+ if (audit)
+ avc_audit(tsec->sid, tsec->sid, sclass, av, &avd, rc, &ad);
+ return rc;
}

/* Check whether a task is allowed to use a system operation. */
@@ -1801,15 +1807,15 @@ static void selinux_capset_set(struct task_struct *target, kernel_cap_t *effecti
secondary_ops->capset_set(target, effective, inheritable, permitted);
}

-static int selinux_capable(struct task_struct *tsk, int cap)
+static int selinux_capable(struct task_struct *tsk, int cap, int audit)
{
int rc;

- rc = secondary_ops->capable(tsk, cap);
+ rc = secondary_ops->capable(tsk, cap, audit);
if (rc)
return rc;

- return task_has_capability(tsk, cap);
+ return task_has_capability(tsk, cap, audit);
}

static int selinux_sysctl_get_sid(ctl_table *table, u16 tclass, u32 *sid)
@@ -1974,7 +1980,7 @@ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages)
int rc, cap_sys_admin = 0;
struct task_security_struct *tsec = current->security;

- rc = secondary_ops->capable(current, CAP_SYS_ADMIN);
+ rc = secondary_ops->capable(current, CAP_SYS_ADMIN, 0);
if (rc == 0)
rc = avc_has_perm_noaudit(tsec->sid, tsec->sid,
SECCLASS_CAPABILITY,
@@ -2819,7 +2825,7 @@ static int selinux_inode_getsecurity(const struct inode *inode, const char *name
* and lack of permission just means that we fall back to the
* in-core context value, not a denial.
*/
- error = secondary_ops->capable(current, CAP_MAC_ADMIN);
+ error = secondary_ops->capable(current, CAP_MAC_ADMIN, 1);
if (!error)
error = avc_has_perm_noaudit(tsec->sid, tsec->sid,
SECCLASS_CAPABILITY2,


2008-10-29 19:07:23

by Eric Paris

[permalink] [raw]
Subject: [PATCH -v1 2/3] vm: use new has_capability_noaudit

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 <[email protected]>
---

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;

/*

2008-10-29 19:07:57

by Eric Paris

[permalink] [raw]
Subject: [PATCH -v1 3/3] filesystems: use has_capability_noaudit interface for reserved blocks checks

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 <[email protected]>
---

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 (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/ubifs/budget.c b/fs/ubifs/budget.c
index 1a4973e..6d13259 100644
--- a/fs/ubifs/budget.c
+++ b/fs/ubifs/budget.c
@@ -32,6 +32,8 @@

#include "ubifs.h"
#include <linux/writeback.h>
+#include <linux/capability.h>
+#include <linux/security.h>
#include <asm/div64.h>

/*
@@ -363,7 +365,7 @@ long long ubifs_calc_available(const struct ubifs_info *c, int min_idx_lebs)
*/
static int can_use_rp(struct ubifs_info *c)
{
- if (current->fsuid == c->rp_uid || capable(CAP_SYS_RESOURCE) ||
+ if (current->fsuid == c->rp_uid || has_capability_noaudit(current, CAP_SYS_RESOURCE) ||
(c->rp_gid != 0 && in_group_p(c->rp_gid)))
return 1;
return 0;
diff --git a/fs/ufs/balloc.c b/fs/ufs/balloc.c
index 0d9ada1..a0a7425 100644
--- a/fs/ufs/balloc.c
+++ b/fs/ufs/balloc.c
@@ -15,6 +15,7 @@
#include <linux/quotaops.h>
#include <linux/buffer_head.h>
#include <linux/capability.h>
+#include <linux/security.h>
#include <linux/bitops.h>
#include <asm/byteorder.h>

@@ -411,7 +412,7 @@ u64 ufs_new_fragments(struct inode *inode, void *p, u64 fragment,
/*
* There is not enough space for user on the device
*/
- if (!capable(CAP_SYS_RESOURCE) && ufs_freespace(uspi, UFS_MINFREE) <= 0) {
+ if (!has_capability_noaudit(current, CAP_SYS_RESOURCE) && ufs_freespace(uspi, UFS_MINFREE) <= 0) {
unlock_super (sb);
UFSD("EXIT (FAILED)\n");
return 0;
diff --git a/security/commoncap.c b/security/commoncap.c
index 243e223..ef0083f 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -55,6 +55,7 @@ int cap_capable(struct task_struct *tsk, int cap, int audit)
return 0;
return -EPERM;
}
+EXPORT_SYMBOL(cap_capable);

int cap_settime(struct timespec *ts, struct timezone *tz)
{
diff --git a/security/security.c b/security/security.c
index 271f9a7..157e3a3 100644
--- a/security/security.c
+++ b/security/security.c
@@ -170,6 +170,7 @@ int security_capable_noaudit(struct task_struct *tsk, int cap)
{
return security_ops->capable(tsk, cap, 0);
}
+EXPORT_SYMBOL(security_capable_noaudit);

int security_acct(struct file *file)
{

2008-10-29 19:16:53

by Stephen Smalley

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

On Wed, 2008-10-29 at 15:06 -0400, Eric Paris wrote:
> 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 <[email protected]>
> ---
>
> 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)) {

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
decide priorities for the OOM killer.


> 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;
>
> /*
--
Stephen Smalley
National Security Agency

2008-10-29 19:57:28

by Eric Paris

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

On Wed, 2008-10-29 at 15:15 -0400, Stephen Smalley wrote:
> On Wed, 2008-10-29 at 15:06 -0400, Eric Paris wrote:
> > 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 <[email protected]>
> > ---
> >
> > 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)) {
>
> 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
> decide priorities for the OOM killer.

Will be fixed in -v2

2008-10-30 15:30:27

by Serge E. Hallyn

[permalink] [raw]
Subject: Re: [PATCH -v1 1/3] SECURITY: new capable_noaudit interface

Quoting Eric Paris ([email protected]):
> 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 <[email protected]>

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

2008-10-30 16:46:38

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH -v1 1/3] SECURITY: new capable_noaudit interface

On Thursday 30 October 2008 11:29:40 am Serge E. Hallyn wrote:
> Quoting Eric Paris ([email protected]):
> > 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 <[email protected]>
>
> Please introduce some meaningful defines instead of passing 0 and 1.
> I.e.
>
> #define CAP_NOAUDIT 0
> #define CAP_AUDIT 1
>
> Otherwise, looks fine.

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

2008-10-30 17:17:54

by Eric Paris

[permalink] [raw]
Subject: Re: [PATCH -v1 1/3] SECURITY: new capable_noaudit interface

On Thu, 2008-10-30 at 12:46 -0400, Paul Moore wrote:
> On Thursday 30 October 2008 11:29:40 am Serge E. Hallyn wrote:
> > Quoting Eric Paris ([email protected]):
> > > 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 <[email protected]>
> >
> > Please introduce some meaningful defines instead of passing 0 and 1.
> > I.e.
> >
> > #define CAP_NOAUDIT 0
> > #define CAP_AUDIT 1
> >
> > Otherwise, looks fine.
>
> 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);

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

2008-10-30 17:30:54

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH -v1 1/3] SECURITY: new capable_noaudit interface

On Thursday 30 October 2008 1:17:32 pm Eric Paris wrote:
> On Thu, 2008-10-30 at 12:46 -0400, Paul Moore wrote:
> > On Thursday 30 October 2008 11:29:40 am Serge E. Hallyn wrote:
> > > Quoting Eric Paris ([email protected]):
> > > > 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 <[email protected]>
> > >
> > > Please introduce some meaningful defines instead of passing 0 and
> > > 1. I.e.
> > >
> > > #define CAP_NOAUDIT 0
> > > #define CAP_AUDIT 1
> > >
> > > Otherwise, looks fine.
> >
> > 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);
>
> 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?

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

--
paul moore
linux @ hp