* [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure
@ 2010-11-15 23:36 Eric Paris
2010-11-16 11:58 ` Casey Schaufler
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Eric Paris @ 2010-11-15 23:36 UTC (permalink / raw)
To: linux-kernel, linux-security-module
Cc: torvalds, joe, drosenberg, mingo, eugeneteo, kees.cook, akpm,
jmorris, morgan, casey, serge
The addition of CONFIG_SECURITY_DMESG_RESTRICT resulted in a build failure
when CONFIG_PRINTK=n. This is because the capabilities code which used the
new option was built even though the variable in question didn't exist. The
patch here fixes this by moving the capabilities checks out of the LSM and
into the caller. All (known) LSMs should have been calling the capabilities
hook already so it actually makes the code organization better to eliminate
the hook altogether.
Signed-off-by: Eric Paris <eparis@redhat.com>
Acked-by: James Morris <jmorris@namei.org>
---
include/linux/security.h | 9 ++++-----
kernel/printk.c | 15 ++++++++++++++-
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, 28 insertions(+), 40 deletions(-)
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..9a2264f 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -274,7 +274,20 @@ 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 this is from /proc/kmsg we only do the capabilities checks
+ * at open time.
+ */
+ 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..778a28f 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;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure
2010-11-15 23:36 [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure Eric Paris
@ 2010-11-16 11:58 ` Casey Schaufler
2010-11-16 15:54 ` Kees Cook
2010-11-18 14:26 ` Serge E. Hallyn
2 siblings, 0 replies; 6+ messages in thread
From: Casey Schaufler @ 2010-11-16 11:58 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, linux-security-module, torvalds, joe, drosenberg,
mingo, eugeneteo, kees.cook, akpm, jmorris, morgan, serge
On 11/15/2010 3:36 PM, Eric Paris wrote:
> The addition of CONFIG_SECURITY_DMESG_RESTRICT resulted in a build failure
> when CONFIG_PRINTK=n. This is because the capabilities code which used the
> new option was built even though the variable in question didn't exist. The
> patch here fixes this by moving the capabilities checks out of the LSM and
> into the caller. All (known) LSMs should have been calling the capabilities
> hook already so it actually makes the code organization better to eliminate
> the hook altogether.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Acked-by: James Morris <jmorris@namei.org>
> ---
>
> include/linux/security.h | 9 ++++-----
> kernel/printk.c | 15 ++++++++++++++-
> 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, 28 insertions(+), 40 deletions(-)
>
> 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..9a2264f 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -274,7 +274,20 @@ 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 this is from /proc/kmsg we only do the capabilities checks
> + * at open time.
> + */
> + 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..778a28f 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;
I haven't tried the patch, but I don't see any problem with it.
I am withholding ACK only because I haven't actually tried it, and I
would not suggest that the patch be held up on my account.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure
2010-11-15 23:36 [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure Eric Paris
2010-11-16 11:58 ` Casey Schaufler
@ 2010-11-16 15:54 ` Kees Cook
2010-11-18 14:26 ` Serge E. Hallyn
2 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2010-11-16 15:54 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, linux-security-module, torvalds, joe, drosenberg,
mingo, eugeneteo, akpm, jmorris, morgan, casey, serge
On Mon, Nov 15, 2010 at 06:36:29PM -0500, Eric Paris wrote:
> The addition of CONFIG_SECURITY_DMESG_RESTRICT resulted in a build failure
> when CONFIG_PRINTK=n. This is because the capabilities code which used the
> new option was built even though the variable in question didn't exist. The
> patch here fixes this by moving the capabilities checks out of the LSM and
> into the caller. All (known) LSMs should have been calling the capabilities
> hook already so it actually makes the code organization better to eliminate
> the hook altogether.
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Acked-by: James Morris <jmorris@namei.org>
Acked-by: Kees Cook <kees.cook@canonical.com>
--
Kees Cook
Ubuntu Security Team
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure
2010-11-15 23:36 [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure Eric Paris
2010-11-16 11:58 ` Casey Schaufler
2010-11-16 15:54 ` Kees Cook
@ 2010-11-18 14:26 ` Serge E. Hallyn
2010-11-18 20:08 ` Kees Cook
2 siblings, 1 reply; 6+ messages in thread
From: Serge E. Hallyn @ 2010-11-18 14:26 UTC (permalink / raw)
To: Eric Paris
Cc: linux-kernel, linux-security-module, torvalds, joe, drosenberg,
mingo, eugeneteo, kees.cook, akpm, jmorris, morgan, casey
Quoting Eric Paris (eparis@redhat.com):
> @@ -274,7 +274,20 @@ 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 this is from /proc/kmsg we only do the capabilities checks
> + * at open time.
> + */
> + if (type == SYSLOG_ACTION_OPEN || !from_file) {
> + if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
Any chance we could introduce a new capability for this?
thanks,
-serge
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure
2010-11-18 14:26 ` Serge E. Hallyn
@ 2010-11-18 20:08 ` Kees Cook
2010-11-18 22:21 ` Serge Hallyn
0 siblings, 1 reply; 6+ messages in thread
From: Kees Cook @ 2010-11-18 20:08 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Eric Paris, linux-kernel, linux-security-module, torvalds, joe,
drosenberg, mingo, eugeneteo, akpm, jmorris, morgan, casey
Hi Serge,
On Thu, Nov 18, 2010 at 08:26:24AM -0600, Serge E. Hallyn wrote:
> Quoting Eric Paris (eparis@redhat.com):
> > @@ -274,7 +274,20 @@ 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 this is from /proc/kmsg we only do the capabilities checks
> > + * at open time.
> > + */
> > + if (type == SYSLOG_ACTION_OPEN || !from_file) {
> > + if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
>
> Any chance we could introduce a new capability for this?
Wasn't there talk of creating CAP_SYSLOG or something? Right now most
sysklogd implementations are just starting life as root, so making this
change now shouldn't break anyone.
-Kees
--
Kees Cook
Ubuntu Security Team
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure
2010-11-18 20:08 ` Kees Cook
@ 2010-11-18 22:21 ` Serge Hallyn
0 siblings, 0 replies; 6+ messages in thread
From: Serge Hallyn @ 2010-11-18 22:21 UTC (permalink / raw)
To: Kees Cook
Cc: Eric Paris, linux-kernel, linux-security-module, torvalds, joe,
drosenberg, mingo, eugeneteo, akpm, jmorris, morgan, casey
Quoting Kees Cook (kees.cook@canonical.com):
> Hi Serge,
>
> On Thu, Nov 18, 2010 at 08:26:24AM -0600, Serge E. Hallyn wrote:
> > Quoting Eric Paris (eparis@redhat.com):
> > > @@ -274,7 +274,20 @@ 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 this is from /proc/kmsg we only do the capabilities checks
> > > + * at open time.
> > > + */
> > > + if (type == SYSLOG_ACTION_OPEN || !from_file) {
> > > + if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> >
> > Any chance we could introduce a new capability for this?
>
> Wasn't there talk of creating CAP_SYSLOG or something? Right now most
> sysklogd implementations are just starting life as root, so making this
> change now shouldn't break anyone.
Uh, huh. I guess the last it was discussed was in March,
http://thread.gmane.org/gmane.linux.kernel.lsm/10615 . I
think not enough people argued about it so it slipped my
mind :) We need someone to flame to drive continued discussion :)
-serge
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-18 22:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-15 23:36 [PATCH] capabilities/syslog: open code cap_syslog logic to fix build failure Eric Paris
2010-11-16 11:58 ` Casey Schaufler
2010-11-16 15:54 ` Kees Cook
2010-11-18 14:26 ` Serge E. Hallyn
2010-11-18 20:08 ` Kees Cook
2010-11-18 22:21 ` Serge Hallyn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).