[PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Joe Perches]

dmesg_restrict is guarded by #ifdef CONFIG_PRINTK in kernel.h
Its uses need to be guarded as well.

Signed-off-by: Joe Perches <joe@perches.com>
---
 kernel/sysctl.c      |    2 +-
 security/commoncap.c |    2 ++
 2 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..5abfa15 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -702,7 +702,6 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &ten_thousand,
 	},
-#endif
 	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
@@ -712,6 +711,7 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
+#endif
 	{
 		.procname	= "ngroups_max",
 		.data		= &ngroups_max,
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..29f2368 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
 {
 	if (type != SYSLOG_ACTION_OPEN && from_file)
 		return 0;
+#ifdef CONFIG_PRINTK
 	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
+#endif
 	if ((type != SYSLOG_ACTION_READ_ALL &&
 	     type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
 		return -EPERM;

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 1635 bytes --]

On Sat, Nov 13, 2010 at 9:26 AM, Joe Perches <joe@perches.com> wrote:
>
> dmesg_restrict is guarded by #ifdef CONFIG_PRINTK in kernel.h
> Its uses need to be guarded as well.

Fair enough, but I think this part:

> diff --git a/security/commoncap.c b/security/commoncap.c
> index 04b80f9..29f2368 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
>  {
>        if (type != SYSLOG_ACTION_OPEN && from_file)
>                return 0;
> +#ifdef CONFIG_PRINTK
>        if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
>                return -EPERM;
> +#endif
>        if ((type != SYSLOG_ACTION_READ_ALL &&
>             type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
>                return -EPERM;

is incredibly ugly. If CONFIG_PRINTK isn't set, the whole function
just becomes pointless, so why guard just that one part of it?

So I would suggest guarding the whole thing, and just returning 0 if
CONFIG_PRINTK isn't set. Or preferably just move the dmesg_restrict
test into do_syslog, and stop playing stupid games with
"security_syslog()", which actually goes away if you disable the you
disable CONFIG_SECURITY.

SECURITY_DMESG_RESTRICT is totally independent of CONFIG_SECURITY, so
doing it in security_syslog() was a bug to begin with.

Or we should make SECURITY_DMESG_RESTRICT _depend_ on CONFIG_SECURITY,
and move it entirely into security/commoncap.c, and not pollute
kernel/printk.c at all with it.

Anyway, suggested replacement patch attached. Comments?

                         Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1475 bytes --]

 kernel/printk.c      |    3 +++
 kernel/sysctl.c      |    2 +-
 security/commoncap.c |    2 --
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 38e7d58..15dd585 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -274,6 +274,9 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	char c;
 	int error = 0;
 
+	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
 	error = security_syslog(type, from_file);
 	if (error)
 		return error;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..5abfa15 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -702,7 +702,6 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &ten_thousand,
 	},
-#endif
 	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
@@ -712,6 +711,7 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
+#endif
 	{
 		.procname	= "ngroups_max",
 		.data		= &ngroups_max,
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..5e632b4 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -895,8 +895,6 @@ 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;

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Dan Rosenberg]

> 
> Anyway, suggested replacement patch attached. Comments?
> 

The desired behavior was to allow a reader with CAP_SYS_ADMIN to open
the syslog via /proc/kmsg and continue reading it even after dropping
capabilities, which is why it was placed where it was.  I see no problem
with moving it back out to do_syslog, but ideally the same behavior
should be replicated.

-Dan

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Joe Perches]

On Sat, 2010-11-13 at 09:50 -0800, Linus Torvalds wrote:
> On Sat, Nov 13, 2010 at 9:26 AM, Joe Perches <joe@perches.com> wrote:
> > dmesg_restrict is guarded by #ifdef CONFIG_PRINTK in kernel.h
> > Its uses need to be guarded as well.
> Fair enough, but I think this part:
> > diff --git a/security/commoncap.c b/security/commoncap.c
> > index 04b80f9..29f2368 100644
> > --- a/security/commoncap.c
> > +++ b/security/commoncap.c
> > @@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
> >  {
> >        if (type != SYSLOG_ACTION_OPEN && from_file)
> >                return 0;
> > +#ifdef CONFIG_PRINTK
> >        if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> >                return -EPERM;
> > +#endif
> >        if ((type != SYSLOG_ACTION_READ_ALL &&
> >             type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
> >                return -EPERM;
> 
> is incredibly ugly. If CONFIG_PRINTK isn't set, the whole function
> just becomes pointless, so why guard just that one part of it?
> 
> So I would suggest guarding the whole thing, and just returning 0 if
> CONFIG_PRINTK isn't set. Or preferably just move the dmesg_restrict
> test into do_syslog, and stop playing stupid games with
> "security_syslog()", which actually goes away if you disable the you
> disable CONFIG_SECURITY.
> 
> SECURITY_DMESG_RESTRICT is totally independent of CONFIG_SECURITY, so
> doing it in security_syslog() was a bug to begin with.
> 
> Or we should make SECURITY_DMESG_RESTRICT _depend_ on CONFIG_SECURITY,
> and move it entirely into security/commoncap.c, and not pollute
> kernel/printk.c at all with it.
> 
> Anyway, suggested replacement patch attached. Comments?
> 
>                          Linus

Maybe something like this?

Make CONFIG_SECURITY_DMESG_RESTRICT depend on CONFIG_SECURITY
Remove dependency on CONFIG_PRINTK

Uncompiled/untested

---
 include/linux/kernel.h |    5 ++++-
 kernel/sysctl.c        |    2 ++
 security/Kconfig       |    1 +
 security/commoncap.c   |    2 ++
 4 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fc3da9e..b9595e8 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -130,6 +130,10 @@ extern const char linux_proc_banner[];
 
 extern int console_printk[];
 
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+extern int dmesg_restrict;
+#endif
+
 #define console_loglevel (console_printk[0])
 #define default_message_loglevel (console_printk[1])
 #define minimum_console_loglevel (console_printk[2])
@@ -293,7 +297,6 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 				   unsigned int interval_msec);
 
 extern int printk_delay_msec;
-extern int dmesg_restrict;
 
 /*
  * Print a one-time message (analogous to WARN_ONCE() et al):
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..5d7eaab 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -703,6 +703,7 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &ten_thousand,
 	},
 #endif
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
 	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
@@ -712,6 +713,7 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
+#endif
 	{
 		.procname	= "ngroups_max",
 		.data		= &ngroups_max,
diff --git a/security/Kconfig b/security/Kconfig
index e80da95..c6583d6 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -41,6 +41,7 @@ config KEYS_DEBUG_PROC_KEYS
 
 config SECURITY_DMESG_RESTRICT
 	bool "Restrict unprivileged access to the kernel syslog"
+	depends on SECURITY
 	default n
 	help
 	  This enforces restrictions on unprivileged users reading the kernel
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..37759b2 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -895,8 +895,10 @@ int cap_syslog(int type, bool from_file)
 {
 	if (type != SYSLOG_ACTION_OPEN && from_file)
 		return 0;
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
 	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
+#endif
 	if ((type != SYSLOG_ACTION_READ_ALL &&
 	     type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
 		return -EPERM;

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Joe Perches]

Maybe something like this is better?

Remove dmesg_restrict from kernel.h printk.c
Move dmesg_restrict to security/commoncap.c
Add extern int dmesg_restrict to kernel/sysctl.c
Use CONFIG_SECURITY_DMESG_RESTRICT as variable guard

uncompiled/untested.

---
 include/linux/kernel.h |    1 -
 kernel/printk.c        |    6 ------
 kernel/sysctl.c        |    6 ++++++
 security/Kconfig       |    1 +
 security/commoncap.c   |    8 ++++++++
 5 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fc3da9e..b526947 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -293,7 +293,6 @@ extern bool printk_timed_ratelimit(unsigned long *caller_jiffies,
 				   unsigned int interval_msec);
 
 extern int printk_delay_msec;
-extern int dmesg_restrict;
 
 /*
  * Print a one-time message (analogous to WARN_ONCE() et al):
diff --git a/kernel/printk.c b/kernel/printk.c
index 38e7d58..b2ebaee 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -261,12 +261,6 @@ static inline void boot_delay_msec(void)
 }
 #endif
 
-#ifdef CONFIG_SECURITY_DMESG_RESTRICT
-int dmesg_restrict = 1;
-#else
-int dmesg_restrict;
-#endif
-
 int do_syslog(int type, char __user *buf, int len, bool from_file)
 {
 	unsigned i, j, limit, count;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..f8ba761 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -106,6 +106,10 @@ extern int sysctl_nr_trim_pages;
 #ifdef CONFIG_BLOCK
 extern int blk_iopoll_enabled;
 #endif
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+extern int dmesg_restrict;
+#endif
+
 
 /* Constants used for minimum and  maximum */
 #ifdef CONFIG_LOCKUP_DETECTOR
@@ -703,6 +707,7 @@ static struct ctl_table kern_table[] = {
 		.extra2		= &ten_thousand,
 	},
 #endif
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
 	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
@@ -712,6 +717,7 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
+#endif
 	{
 		.procname	= "ngroups_max",
 		.data		= &ngroups_max,
diff --git a/security/Kconfig b/security/Kconfig
index e80da95..c6583d6 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -41,6 +41,7 @@ config KEYS_DEBUG_PROC_KEYS
 
 config SECURITY_DMESG_RESTRICT
 	bool "Restrict unprivileged access to the kernel syslog"
+	depends on SECURITY
 	default n
 	help
 	  This enforces restrictions on unprivileged users reading the kernel
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..08066df 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -883,6 +883,12 @@ error:
 	return error;
 }
 
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+int dmesg_restrict = 1;
+#else
+int dmesg_restrict;
+#endif
+
 /**
  * cap_syslog - Determine whether syslog function is permitted
  * @type: Function requested
@@ -895,8 +901,10 @@ int cap_syslog(int type, bool from_file)
 {
 	if (type != SYSLOG_ACTION_OPEN && from_file)
 		return 0;
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
 	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
 		return -EPERM;
+#endif
 	if ((type != SYSLOG_ACTION_READ_ALL &&
 	     type != SYSLOG_ACTION_SIZE_BUFFER) && !capable(CAP_SYS_ADMIN))
 		return -EPERM;

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]

On Sat, Nov 13, 2010 at 10:25 AM, Dan Rosenberg
<drosenberg@vsecurity.com> wrote:
>
>>
>> Anyway, suggested replacement patch attached. Comments?
>>
>
> The desired behavior was to allow a reader with CAP_SYS_ADMIN to open
> the syslog via /proc/kmsg and continue reading it even after dropping
> capabilities, which is why it was placed where it was.  I see no problem
> with moving it back out to do_syslog, but ideally the same behavior
> should be replicated.

Hmm. No wonder I missed that. The security interface is totally
idiotic. If the intention is for /proc/kmsg security checks to be done
at open time, then dammit, that logic should _not_ be inside some
random security policy.

So I missed the intention, because the code is written in such an odd
way. Those security hooks were obviously done as a
"search-and-replace" kind of thing, rather than trying to make sense.

I suspect "from_file" should never be passed to the security hook,
since the only point would be exactly that "do security checks of
/proc/kmsg at open time" - which I think is better done totally
independent of the security model - otherwise the security models just
inevitably just always do fundamentally different things.

Security people should be the ones to know that the way to do security
is to make it obvious, instead of having totally crazy interfaces for
hooks that make no sense. "Not making sense" is how obvious patches
then miss the point of the check.

So what happens now is that the capability-based logic thinks the
rules are about "open time", while the _other_ security rules seem to
think it's about read time (_and_ open time - they just ignore the
whole from_file).

So which one is right? Making it a case of "random security models can
implement totally random semantics" is just stupid.

So my suspicion is that the intent was to just do the check at open
time, and the confusing interface just means that selinux and others
didn't even realize what the whole intent of that "from_file" thing
was. Why not just fix that. How does this (UNTESTED!) patch look?

I've added James Morris to the recipients list. Comments?

(The diffstat says that this adds more lines than it removes, but that
is misleading: it is due to actually commenting the rule that checks
are done open-time for /proc/kmsg)

                         Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 6188 bytes --]

 include/linux/security.h   |   11 +++++------
 kernel/printk.c            |   15 ++++++++++++---
 kernel/sysctl.c            |    2 +-
 security/commoncap.c       |    7 +------
 security/security.c        |    4 ++--
 security/selinux/hooks.c   |    4 ++--
 security/smack/smack_lsm.c |    4 ++--
 7 files changed, 25 insertions(+), 22 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index b8246a8..10f10f1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -77,7 +77,7 @@ 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_syslog(int type);
 extern int cap_vm_enough_memory(struct mm_struct *mm, long pages);
 
 struct msghdr;
@@ -1270,7 +1270,6 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	logging to the console.
  *	See the syslog(2) manual page for an explanation of the @type values.
  *	@type contains the type of action.
- *	@from_file indicates the context of action (if it came from /proc).
  *	Return 0 if permission is granted.
  * @settime:
  *	Check permission to change the system time.
@@ -1388,7 +1387,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);
 
@@ -1671,7 +1670,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);
@@ -1901,9 +1900,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 cap_syslog(type);
 }
 
 static inline int security_settime(struct timespec *ts, struct timezone *tz)
diff --git a/kernel/printk.c b/kernel/printk.c
index 38e7d58..a5bfa5a 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -274,9 +274,18 @@ 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 (error)
-		return error;
+	/*
+	 * If we have use /proc/kmsg and the open succeeded,
+	 * we don't do any extra security checks: they were
+	 * done at open time.
+	 */
+	if (type == SYSLOG_ACTION_OPEN || !from_file) {
+		if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		error = security_syslog(type);
+		if (error)
+			return error;
+	}
 
 	switch (type) {
 	case SYSLOG_ACTION_CLOSE:	/* Close log */
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index b65bf63..5abfa15 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -702,7 +702,6 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &ten_thousand,
 	},
-#endif
 	{
 		.procname	= "dmesg_restrict",
 		.data		= &dmesg_restrict,
@@ -712,6 +711,7 @@ static struct ctl_table kern_table[] = {
 		.extra1		= &zero,
 		.extra2		= &one,
 	},
+#endif
 	{
 		.procname	= "ngroups_max",
 		.data		= &ngroups_max,
diff --git a/security/commoncap.c b/security/commoncap.c
index 04b80f9..8ce2400 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -886,17 +886,12 @@ 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)
+int cap_syslog(int type)
 {
-	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;
diff --git a/security/security.c b/security/security.c
index 3ef5e2a..1b798d3 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 d9154cf..13cf8f1 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1973,11 +1973,11 @@ 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);
+	rc = cap_syslog(type);
 	if (rc)
 		return rc;
 
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index bc39f40..6d59b6d 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -157,12 +157,12 @@ 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 type)
 {
 	int rc;
 	char *sp = current_security();
 
-	rc = cap_syslog(type, from_file);
+	rc = cap_syslog(type);
 	if (rc != 0)
 		return rc;
 

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Linus Torvalds]

On Sat, Nov 13, 2010 at 11:51 AM, Joe Perches <joe@perches.com> wrote:
>
> Maybe something like this?
>
> Make CONFIG_SECURITY_DMESG_RESTRICT depend on CONFIG_SECURITY
> Remove dependency on CONFIG_PRINTK

No, this is wrong:

> +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> +extern int dmesg_restrict;
> +#endif

CONFIG_SECURITY_DMESG_RESTRICT is supposed to be about the initial
_value_ of dmesg_restrict, not about whether it exists or not. If you
don't have CONFIG_SECURITY, you still end up defaulting to the common
capability model, and it would still want that dmesg_restrict thing.

But what can make sense is to move "dmesg_restrict" into
security/commoncap.c, and just make it about capabilities. Of course,
that then means that if you use some other security model that just
doesn't care about capabilities at all, they'll never care about
dmesg_restrict either. So that, to me, smells of really bad interface
design.

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.

That's why I don't think we should do things like this inside the
security models themselves. People just get really confused about what
the real semantics are.

If something should be generic (and by all accounts, that's the
intention of 'dmesg_restrict', the same way it was for
'mmap_min_addr'). Which is why I'd change the whole idiotic
security_syslog() model itself as per the patch I just sent out.

                               Linus

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Kees Cook]

On Sat, Nov 13, 2010 at 09:50:22AM -0800, Linus Torvalds wrote:
> So I would suggest guarding the whole thing, and just returning 0 if
> CONFIG_PRINTK isn't set. Or preferably just move the dmesg_restrict
> test into do_syslog, and stop playing stupid games with
> "security_syslog()", which actually goes away if you disable the you
> disable CONFIG_SECURITY.
> 
> SECURITY_DMESG_RESTRICT is totally independent of CONFIG_SECURITY, so
> doing it in security_syslog() was a bug to begin with.
> 
> Or we should make SECURITY_DMESG_RESTRICT _depend_ on CONFIG_SECURITY,
> and move it entirely into security/commoncap.c, and not pollute
> kernel/printk.c at all with it.

It seems saner to put it in commoncaps to me, but either way, it must happen
after the from_file and OPEN test or it will break /proc/kmesg readers who
open and drop privs (as done with sysklogd, etc).

> +	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
>...
>  {
>  	if (type != SYSLOG_ACTION_OPEN && from_file)
>  		return 0;
> -	if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> -		return -EPERM;

i.e.

if (dmesg_restrict && (!from_file || type == SYSLOG_ACTION_OPEN) &&
    !capable(CAP_SYS_ADMIN))

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Kees Cook]

On Sat, Nov 13, 2010 at 12:22:15PM -0800, Linus Torvalds wrote:
> Hmm. No wonder I missed that. The security interface is totally
> idiotic. If the intention is for /proc/kmsg security checks to be done
> at open time, then dammit, that logic should _not_ be inside some
> random security policy.

I think the real problem is that this interface exists as both a syscall
and a /proc file (sysklogd things use the /proc file). Dropping the
from_file means that security policy cannot revalidate the policy
(sometimes it might want to block the read, i.e. passing the open fd to
another process that is not privileged). But since nothing is actually
using from_file yet, I guess it's not a big deal.

And note that I'm not defending any specific part of it; I'm just trying
to point out what not be possible in the future if we drop from_file
like this.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> (The diffstat says that this adds more lines than it removes, but that
> is misleading: it is due to actually commenting the rule that checks
> are done open-time for /proc/kmsg)
> 
>                          Linus
>
>  include/linux/security.h   |   11 +++++------
>  kernel/printk.c            |   15 ++++++++++++---
>  kernel/sysctl.c            |    2 +-
>  security/commoncap.c       |    7 +------
>  security/security.c        |    4 ++--
>  security/selinux/hooks.c   |    4 ++--
>  security/smack/smack_lsm.c |    4 ++--
>  7 files changed, 25 insertions(+), 22 deletions(-)

I gave it some testing in -tip and your patch also solves the !EMBEDDED build bug 
(and it also still restricts unprivileged dmesg output when desired), so:

 Acked-by: Ingo Molnar <mingo@elte.hu>
 Tested-by: Ingo Molnar <mingo@elte.hu>

Thanks,

	Ingo

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[James Morris]

On Sat, 13 Nov 2010, Linus Torvalds wrote:

[Adding the LSM list]

> CONFIG_SECURITY_DMESG_RESTRICT is supposed to be about the initial
> _value_ of dmesg_restrict, not about whether it exists or not. If you
> don't have CONFIG_SECURITY, you still end up defaulting to the common
> capability model, and it would still want that dmesg_restrict thing.
> 
> But what can make sense is to move "dmesg_restrict" into
> security/commoncap.c, and just make it about capabilities. Of course,
> that then means that if you use some other security model that just
> doesn't care about capabilities at all, they'll never care about
> dmesg_restrict either. So that, to me, smells of really bad interface
> design.

Yes, it should not be possible for an LSM to reduce the default security 
-- an interface which allows this breaks the security model.

> 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.
>
> That's why I don't think we should do things like this inside the
> security models themselves. People just get really confused about what
> the real semantics are.
> 
> If something should be generic (and by all accounts, that's the
> intention of 'dmesg_restrict', the same way it was for
> 'mmap_min_addr'). Which is why I'd change the whole idiotic
> security_syslog() model itself as per the patch I just sent out.

Looks like the right approach to me.

Kees, does this patch work for you?

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.


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Eric Paris]

On Sat, Nov 13, 2010 at 3:31 PM, Linus Torvalds
<torvalds@linux-foundation.org> 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

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Eric Paris]

[-- Attachment #1: Type: text/plain, Size: 2794 bytes --]

On Mon, Nov 15, 2010 at 12:04 PM, Eric Paris <eparis@parisplace.org> wrote:
> On Sat, Nov 13, 2010 at 3:31 PM, Linus Torvalds
> <torvalds@linux-foundation.org> 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

[-- Attachment #2: tmp.patch --]
[-- Type: text/x-patch, Size: 5870 bytes --]

commit acef990cde0dbdc9e88df2da54cac4debc6a503f
Author: Eric Paris <eparis@redhat.com>
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;
 

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Linus Torvalds]

On Mon, Nov 15, 2010 at 9:34 AM, Eric Paris <eparis@parisplace.org> wrote:
>
> I'm sure somebody somewhere hates it, but I was thinking something
> like the attached.

I certainly like it. If the old rule should have been that you _have_
to call cap_syslog(), then just eviscerating that entirely and putting
it in the generic code is definitely the right thing.

Anyway, I wanted to do -rc2 yesterday, but I don't really want this
kernel build problem to remain (even if it's not a very relevant
config for most people).

So if people can quickly agree on this, I'll take it and do -rc2 later
today, otherwise I'll do Joe's trivial patch as a stop-gap measure
pending approval for cleanup/fixing the security interfaces.

                              Linus

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Eric Paris]

On Mon, Nov 15, 2010 at 12:41 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> If the old rule should have been that you _have_
> to call cap_syslog(), then just eviscerating that entirely and putting
> it in the generic code is definitely the right thing.

That is the rule for ALL of the hooks in commoncap.c.  The one time I
tried to do something else *cough*mmap_min_addr*cough* I screwed it
up.  I'll put a note in my todo list about looking into lifting all of
commoncap.c into the callers.

-Eric

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Linus Torvalds]

On Mon, Nov 15, 2010 at 9:45 AM, Eric Paris <eparis@parisplace.org> wrote:
>
> That is the rule for ALL of the hooks in commoncap.c.  The one time I
> tried to do something else *cough*mmap_min_addr*cough* I screwed it
> up.  I'll put a note in my todo list about looking into lifting all of
> commoncap.c into the callers.

Into "security/security.c" itself? That would work, except it doesn't
work exactly in a situation like this where the whole interface was
polluted by the commoncap version simply having fundamentally
different semantics (ie the whole "no security check at read time,
only at open time"). Passing the whole "from_file" thing around was
just ugly.

And while passing the commoncap cases down into the callers of the
"security_xyz()" interface itself makes sense in this case, I don't
think it makes sense in general. With 'security_syslog()' there really
was just one very specific call-site. Other security wrappers have
many more (eg "security_vm_enough_memory()") call sites, and moving
the cap_xyz() code to those callsites would be totally wrong
duplication.

                        Linus

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[James Morris]

On Mon, 15 Nov 2010, Eric Paris wrote:

> On Mon, Nov 15, 2010 at 12:41 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > If the old rule should have been that you _have_
> > to call cap_syslog(), then just eviscerating that entirely and putting
> > it in the generic code is definitely the right thing.
> 
> That is the rule for ALL of the hooks in commoncap.c.  The one time I
> tried to do something else *cough*mmap_min_addr*cough* I screwed it
> up.  I'll put a note in my todo list about looking into lifting all of
> commoncap.c into the callers.

If it's a requirement of the API that all of the cap calls are made 
first, then build it into the API, so developers can't make a mistake.  
e.g. have the LSM API do the secondary stacking of caps behind the scenes.

I had thought that the idea was that some LSM may want to not implement 
capabilities at all, on which case, it should still not be possible for 
the API to weaken the default security with or without caps.  In any case, 
mixing generic logic with capabilities logic seems to be a fundamental 
issue, and one which we should avoid, and remove where it may exist (I did 
audit the hooks after the mmap_min_addr thing, but it's worth checking 
again).


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[James Morris]

On Mon, 15 Nov 2010, Linus Torvalds wrote:

> On Mon, Nov 15, 2010 at 9:34 AM, Eric Paris <eparis@parisplace.org> wrote:
> >
> > I'm sure somebody somewhere hates it, but I was thinking something
> > like the attached.
> 
> I certainly like it. If the old rule should have been that you _have_
> to call cap_syslog(), then just eviscerating that entirely and putting
> it in the generic code is definitely the right thing.
> 
> Anyway, I wanted to do -rc2 yesterday, but I don't really want this
> kernel build problem to remain (even if it's not a very relevant
> config for most people).
> 
> So if people can quickly agree on this, I'll take it and do -rc2 later
> today, otherwise I'll do Joe's trivial patch as a stop-gap measure
> pending approval for cleanup/fixing the security interfaces.

Looks good to me:

Acked-by: James Morris <jmorris@namei.org>


(I'd really like to see new security features get fully reviewed on the 
LSM list and bake in -next for a while in the future, no matter how 
obviously correct they seem).


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Linus Torvalds]

On Mon, Nov 15, 2010 at 2:16 PM, James Morris <jmorris@namei.org> wrote:
>
> Acked-by: James Morris <jmorris@namei.org>

Ok, now we need sign-off and changelog from Eric... Ping?

> (I'd really like to see new security features get fully reviewed on the
> LSM list and bake in -next for a while in the future, no matter how
> obviously correct they seem).

Hey, I can still just do the trivial "just fix the build" part without
actually cleaning up the security layer for -rc2. Just let me know
which one you'd prefer.

                     Linus

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Eric Paris]

On Tue, 2010-11-16 at 09:13 +1100, James Morris wrote:
> On Mon, 15 Nov 2010, Eric Paris wrote:
> 
> > On Mon, Nov 15, 2010 at 12:41 PM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> > > If the old rule should have been that you _have_
> > > to call cap_syslog(), then just eviscerating that entirely and putting
> > > it in the generic code is definitely the right thing.
> > 
> > That is the rule for ALL of the hooks in commoncap.c.  The one time I
> > tried to do something else *cough*mmap_min_addr*cough* I screwed it
> > up.  I'll put a note in my todo list about looking into lifting all of
> > commoncap.c into the callers.
> 
> If it's a requirement of the API that all of the cap calls are made 
> first, then build it into the API, so developers can't make a mistake.  
> e.g. have the LSM API do the secondary stacking of caps behind the scenes.

At this point it's a defacto requirement since noone is doing anything
like that.  My mmap_min_addr screw up is, to the best of my knowledge,
the only time anyone has intentionally not called the caps code...

And I sorta like the idea of moving the cap_* calls directly into
security_*.  Great, another item on the todo list.  Lift as many cap
calls into the caller as is reasonable (I don't think there are
many/any) and if not possible lift them directory into security_*.  If
someone else really wants to make a system truely without capabilities
lets look at there solution then....

> I had thought that the idea was that some LSM may want to not implement 
> capabilities at all, on which case, it should still not be possible for 
> the API to weaken the default security with or without caps.

Not sure how that's possible.  I mean, I guess it's possible if the
fabled LSM reimplements the cap call, but I'm not sure how you can
remove a restrictive only security check without 'weakening' the system
in some way.

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[James Morris]

On Mon, 15 Nov 2010, Linus Torvalds wrote:

> On Mon, Nov 15, 2010 at 2:16 PM, James Morris <jmorris@namei.org> wrote:
> >
> > Acked-by: James Morris <jmorris@namei.org>
> 
> Ok, now we need sign-off and changelog from Eric... Ping?
> 
> > (I'd really like to see new security features get fully reviewed on the
> > LSM list and bake in -next for a while in the future, no matter how
> > obviously correct they seem).
> 
> Hey, I can still just do the trivial "just fix the build" part without
> actually cleaning up the security layer for -rc2. Just let me know
> which one you'd prefer.

I'd prefer to get the right code in (Eric's).


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[James Morris]

On Mon, 15 Nov 2010, Eric Paris wrote:

> Not sure how that's possible.  I mean, I guess it's possible if the
> fabled LSM reimplements the cap call, but I'm not sure how you can
> remove a restrictive only security check without 'weakening' the system
> in some way.

If generic security logic is mixed into a capability call, then not 
implementing the cap call also loses the generic security logic.

e.g. dmesg_restrict should always be verified, regardless of whether 
cap_security() is called or not.

If cap_syslog() should always be called, then it should not be possible 
not not call it :-)


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n

[Eric Paris]

On Tue, 2010-11-16 at 09:58 +1100, James Morris wrote:
> On Mon, 15 Nov 2010, Eric Paris wrote:
> 
> > Not sure how that's possible.  I mean, I guess it's possible if the
> > fabled LSM reimplements the cap call, but I'm not sure how you can
> > remove a restrictive only security check without 'weakening' the system
> > in some way.
> 
> If generic security logic is mixed into a capability call, then not 
> implementing the cap call also loses the generic security logic.

I guess it comes down to what you define 'generic security logic.'
We've come to expect that capabilities are an indispensable mechanism
for control object access.  The prevalence of if (!capable(***))
throughout the kernel proves that fact.  I think that sometimes open
coding how we expect to use capabilities and sometimes hiding it behind
an LSM hook is just bad news.  I'd prefer all open coding, but that
might not be the best in all situations.  Hopefully I'll get a chance to
try to clean that up a little.

In any case, right now I need to go write a patch description since I
just compile tested it a couple of ways....

-Eric