Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=yand CONFIG_PRINTK=n
From: Eric Paris
Date: Mon Nov 15 2010 - 12:34:55 EST
On Mon, Nov 15, 2010 at 12:04 PM, Eric Paris <eparis@xxxxxxxxxxxxxx> wrote:
> On Sat, Nov 13, 2010 at 3:31 PM, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>> We had this exact problem with the whole "mmap_min_addr" thing. People
>> _thought_ of it as generic, but because it was actually tested by the
>> security logic, if you ended up enabling SELinux the test actually
>> went away entirely (or maybe it was the other way around). So with
>> certain security models, the whole thing was bypassed, and the
>> security module actually became an _IN_security module.
>
> Your recollection is wrong, although your conclusions of the
> ramifications are right. Either SELinux or capabilities checked
> mmap_min_addr, depending on which was 'primary.' Just as they are
> different modules they checked for different things. Only doing
> SELinux checks was stronger for some situations, and only doing
> capability checks was stronger in some ways (and the reverse was
> obviously true). Today you get the best of both worlds since we
> really have 2 different mmap_min_addr values...
>
> In any case the result of that is that LSMs (ok 'I') need to be more
> careful making sure they interact properly with the generic
> capabilities hooks.
>
> From: James Morris
>> I want to ensure that LSMs which implement security_syslog can't end up
>> with a less secure system than the default, regardless of whether they
>> call cap_syslog or not.
>
> Which really means that this is total crap. If you don't call
> cap_syslog() you broke it. That's all there is to it. Calling the
> capability code is always required. full stop.
>
> I think this patch is broken though. SELinux and SMACK don't care
> about from_file and want to check every time no matter what. Your
> patch breaks that and only will call the LSM on occasion. It's only
> capabilities that likes those semantics. I think the entire contents
> of the cap_syslog hook should be moved up and that hook just dropped
> entirely.
>
> I'll code up what I'm thinking in a minute.....
>
> -Eric
I'm sure somebody somewhere hates it, but I was thinking something
like the attached.
include/linux/security.h | 9 ++++-----
kernel/printk.c | 11 ++++++++++-
security/capability.c | 5 +++++
security/commoncap.c | 21 ---------------------
security/security.c | 4 ++--
security/selinux/hooks.c | 6 +-----
security/smack/smack_lsm.c | 8 ++------
7 files changed, 24 insertions(+), 40 deletions(-)
(Personally I think that most of the hooks in commoncap.c code should
be moved out of security/ altogether and we should completely do away
with our current ghetto inter LSM calls. But that's just me)
-Eric
commit acef990cde0dbdc9e88df2da54cac4debc6a503f
Author: Eric Paris <eparis@xxxxxxxxxx>
Date: Mon Nov 15 12:20:27 2010 -0500
fix capabilities-restric
diff --git a/include/linux/security.h b/include/linux/security.h
index aee3b8f..d42619e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -77,7 +77,6 @@ extern int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3,
extern int cap_task_setscheduler(struct task_struct *p);
extern int cap_task_setioprio(struct task_struct *p, int ioprio);
extern int cap_task_setnice(struct task_struct *p, int nice);
-extern int cap_syslog(int type, bool from_file);
extern int cap_vm_enough_memory(struct mm_struct *mm, long pages);
struct msghdr;
@@ -1389,7 +1388,7 @@ struct security_operations {
int (*sysctl) (struct ctl_table *table, int op);
int (*quotactl) (int cmds, int type, int id, struct super_block *sb);
int (*quota_on) (struct dentry *dentry);
- int (*syslog) (int type, bool from_file);
+ int (*syslog) (int type);
int (*settime) (struct timespec *ts, struct timezone *tz);
int (*vm_enough_memory) (struct mm_struct *mm, long pages);
@@ -1675,7 +1674,7 @@ int security_real_capable_noaudit(struct task_struct *tsk, int cap);
int security_sysctl(struct ctl_table *table, int op);
int security_quotactl(int cmds, int type, int id, struct super_block *sb);
int security_quota_on(struct dentry *dentry);
-int security_syslog(int type, bool from_file);
+int security_syslog(int type);
int security_settime(struct timespec *ts, struct timezone *tz);
int security_vm_enough_memory(long pages);
int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
@@ -1907,9 +1906,9 @@ static inline int security_quota_on(struct dentry *dentry)
return 0;
}
-static inline int security_syslog(int type, bool from_file)
+static inline int security_syslog(int type)
{
- return cap_syslog(type, from_file);
+ return 0;
}
static inline int security_settime(struct timespec *ts, struct timezone *tz)
diff --git a/kernel/printk.c b/kernel/printk.c
index 38e7d58..d00efd1 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -274,7 +274,16 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
char c;
int error = 0;
- error = security_syslog(type, from_file);
+ if (type == SYSLOG_ACTION_OPEN || !from_file) {
+ if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ if ((type != SYSLOG_ACTION_READ_ALL &&
+ type != SYSLOG_ACTION_SIZE_BUFFER) &&
+ !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ }
+
+ error = security_syslog(type);
if (error)
return error;
diff --git a/security/capability.c b/security/capability.c
index d6d613a..6b5c6e9 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -17,6 +17,11 @@ static int cap_sysctl(ctl_table *table, int op)
return 0;
}
+static int cap_syslog(int type)
+{
+ return 0;
+}
+
static int cap_quotactl(int cmds, int type, int id, struct super_block *sb)
{
return 0;
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..64c2ed9 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -27,7 +27,6 @@
#include <linux/sched.h>
#include <linux/prctl.h>
#include <linux/securebits.h>
-#include <linux/syslog.h>
/*
* If a non-root user executes a setuid-root binary in
@@ -884,26 +883,6 @@ error:
}
/**
- * cap_syslog - Determine whether syslog function is permitted
- * @type: Function requested
- * @from_file: Whether this request came from an open file (i.e. /proc)
- *
- * Determine whether the current process is permitted to use a particular
- * syslog function, returning 0 if permission is granted, -ve if not.
- */
-int cap_syslog(int type, bool from_file)
-{
- if (type != SYSLOG_ACTION_OPEN && from_file)
- return 0;
- if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
- return -EPERM;
- if ((type != SYSLOG_ACTION_READ_ALL &&
- type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
- return -EPERM;
- return 0;
-}
-
-/**
* cap_vm_enough_memory - Determine whether a new virtual mapping is permitted
* @mm: The VM space in which the new mapping is to be made
* @pages: The size of the mapping
diff --git a/security/security.c b/security/security.c
index 259d3ad..639a72a 100644
--- a/security/security.c
+++ b/security/security.c
@@ -197,9 +197,9 @@ int security_quota_on(struct dentry *dentry)
return security_ops->quota_on(dentry);
}
-int security_syslog(int type, bool from_file)
+int security_syslog(int type)
{
- return security_ops->syslog(type, from_file);
+ return security_ops->syslog(type);
}
int security_settime(struct timespec *ts, struct timezone *tz)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 8ba5001..e066bc2 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1971,14 +1971,10 @@ static int selinux_quota_on(struct dentry *dentry)
return dentry_has_perm(cred, NULL, dentry, FILE__QUOTAON);
}
-static int selinux_syslog(int type, bool from_file)
+static int selinux_syslog(int type)
{
int rc;
- rc = cap_syslog(type, from_file);
- if (rc)
- return rc;
-
switch (type) {
case SYSLOG_ACTION_READ_ALL: /* Read last kernel messages */
case SYSLOG_ACTION_SIZE_BUFFER: /* Return size of the log buffer */
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 6cc47ef..f7b8bee 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -157,15 +157,11 @@ static int smack_ptrace_traceme(struct task_struct *ptp)
*
* Returns 0 on success, error code otherwise.
*/
-static int smack_syslog(int type, bool from_file)
+static int smack_syslog(int typefrom_file)
{
- int rc;
+ int rc = 0;
char *sp = current_security();
- rc = cap_syslog(type, from_file);
- if (rc != 0)
- return rc;
-
if (capable(CAP_MAC_OVERRIDE))
return 0;