* [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n @ 2010-11-13 17:26 Joe Perches 2010-11-13 17:50 ` Linus Torvalds 0 siblings, 1 reply; 23+ messages in thread From: Joe Perches @ 2010-11-13 17:26 UTC (permalink / raw) To: Dan Rosenberg, LKML Cc: Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, Linus Torvalds 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; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 17:26 [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n Joe Perches @ 2010-11-13 17:50 ` Linus Torvalds 2010-11-13 18:25 ` Dan Rosenberg ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Linus Torvalds @ 2010-11-13 17:50 UTC (permalink / raw) To: Joe Perches Cc: Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton [-- 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; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 17:50 ` Linus Torvalds @ 2010-11-13 18:25 ` Dan Rosenberg 2010-11-13 20:22 ` Linus Torvalds 2010-11-13 19:51 ` Joe Perches 2010-11-14 2:44 ` Kees Cook 2 siblings, 1 reply; 23+ messages in thread From: Dan Rosenberg @ 2010-11-13 18:25 UTC (permalink / raw) To: Linus Torvalds Cc: Joe Perches, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton > > 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 18:25 ` Dan Rosenberg @ 2010-11-13 20:22 ` Linus Torvalds 2010-11-14 3:05 ` Kees Cook 2010-11-14 12:35 ` Ingo Molnar 0 siblings, 2 replies; 23+ messages in thread From: Linus Torvalds @ 2010-11-13 20:22 UTC (permalink / raw) To: Dan Rosenberg, James Morris Cc: Joe Perches, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton [-- 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; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 20:22 ` Linus Torvalds @ 2010-11-14 3:05 ` Kees Cook 2010-11-14 12:35 ` Ingo Molnar 1 sibling, 0 replies; 23+ messages in thread From: Kees Cook @ 2010-11-14 3:05 UTC (permalink / raw) To: Linus Torvalds Cc: Dan Rosenberg, James Morris, Joe Perches, LKML, Ingo Molnar, Eugene Teo, Andrew Morton 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 20:22 ` Linus Torvalds 2010-11-14 3:05 ` Kees Cook @ 2010-11-14 12:35 ` Ingo Molnar 1 sibling, 0 replies; 23+ messages in thread From: Ingo Molnar @ 2010-11-14 12:35 UTC (permalink / raw) To: Linus Torvalds Cc: Dan Rosenberg, James Morris, Joe Perches, LKML, Eugene Teo, Kees Cook, Andrew Morton * 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 17:50 ` Linus Torvalds 2010-11-13 18:25 ` Dan Rosenberg @ 2010-11-13 19:51 ` Joe Perches 2010-11-13 20:01 ` Joe Perches 2010-11-13 20:31 ` Linus Torvalds 2010-11-14 2:44 ` Kees Cook 2 siblings, 2 replies; 23+ messages in thread From: Joe Perches @ 2010-11-13 19:51 UTC (permalink / raw) To: Linus Torvalds Cc: Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton 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; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 19:51 ` Joe Perches @ 2010-11-13 20:01 ` Joe Perches 2010-11-13 20:31 ` Linus Torvalds 1 sibling, 0 replies; 23+ messages in thread From: Joe Perches @ 2010-11-13 20:01 UTC (permalink / raw) To: Linus Torvalds Cc: Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton 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; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 19:51 ` Joe Perches 2010-11-13 20:01 ` Joe Perches @ 2010-11-13 20:31 ` Linus Torvalds 2010-11-15 1:16 ` James Morris 2010-11-15 17:04 ` Eric Paris 1 sibling, 2 replies; 23+ messages in thread From: Linus Torvalds @ 2010-11-13 20:31 UTC (permalink / raw) To: Joe Perches Cc: Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, James Morris 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 20:31 ` Linus Torvalds @ 2010-11-15 1:16 ` James Morris 2010-11-15 17:04 ` Eric Paris 1 sibling, 0 replies; 23+ messages in thread From: James Morris @ 2010-11-15 1:16 UTC (permalink / raw) To: Linus Torvalds Cc: Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, linux-security-module 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> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 20:31 ` Linus Torvalds 2010-11-15 1:16 ` James Morris @ 2010-11-15 17:04 ` Eric Paris 2010-11-15 17:34 ` Eric Paris 1 sibling, 1 reply; 23+ messages in thread From: Eric Paris @ 2010-11-15 17:04 UTC (permalink / raw) To: Linus Torvalds Cc: Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, James Morris 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 17:04 ` Eric Paris @ 2010-11-15 17:34 ` Eric Paris 2010-11-15 17:41 ` Linus Torvalds 0 siblings, 1 reply; 23+ messages in thread From: Eric Paris @ 2010-11-15 17:34 UTC (permalink / raw) To: Linus Torvalds Cc: Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, James Morris, LSM List [-- 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; ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 17:34 ` Eric Paris @ 2010-11-15 17:41 ` Linus Torvalds 2010-11-15 17:45 ` Eric Paris 2010-11-15 22:16 ` James Morris 0 siblings, 2 replies; 23+ messages in thread From: Linus Torvalds @ 2010-11-15 17:41 UTC (permalink / raw) To: Eric Paris Cc: Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, James Morris, LSM List 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 17:41 ` Linus Torvalds @ 2010-11-15 17:45 ` Eric Paris 2010-11-15 18:20 ` Linus Torvalds 2010-11-15 22:13 ` James Morris 2010-11-15 22:16 ` James Morris 1 sibling, 2 replies; 23+ messages in thread From: Eric Paris @ 2010-11-15 17:45 UTC (permalink / raw) To: Linus Torvalds Cc: Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, James Morris, LSM List 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 17:45 ` Eric Paris @ 2010-11-15 18:20 ` Linus Torvalds 2010-11-15 22:13 ` James Morris 1 sibling, 0 replies; 23+ messages in thread From: Linus Torvalds @ 2010-11-15 18:20 UTC (permalink / raw) To: Eric Paris Cc: Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, James Morris, LSM List 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 17:45 ` Eric Paris 2010-11-15 18:20 ` Linus Torvalds @ 2010-11-15 22:13 ` James Morris 2010-11-15 22:43 ` Eric Paris 1 sibling, 1 reply; 23+ messages in thread From: James Morris @ 2010-11-15 22:13 UTC (permalink / raw) To: Eric Paris Cc: Linus Torvalds, Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, LSM List 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> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 22:13 ` James Morris @ 2010-11-15 22:43 ` Eric Paris 2010-11-15 22:58 ` James Morris 0 siblings, 1 reply; 23+ messages in thread From: Eric Paris @ 2010-11-15 22:43 UTC (permalink / raw) To: James Morris Cc: Eric Paris, Linus Torvalds, Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, LSM List 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 22:43 ` Eric Paris @ 2010-11-15 22:58 ` James Morris 2010-11-15 23:08 ` Eric Paris 0 siblings, 1 reply; 23+ messages in thread From: James Morris @ 2010-11-15 22:58 UTC (permalink / raw) To: Eric Paris Cc: Eric Paris, Linus Torvalds, Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, LSM List 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> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 22:58 ` James Morris @ 2010-11-15 23:08 ` Eric Paris 0 siblings, 0 replies; 23+ messages in thread From: Eric Paris @ 2010-11-15 23:08 UTC (permalink / raw) To: James Morris Cc: Linus Torvalds, Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, LSM List 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 17:41 ` Linus Torvalds 2010-11-15 17:45 ` Eric Paris @ 2010-11-15 22:16 ` James Morris 2010-11-15 22:36 ` Linus Torvalds 1 sibling, 1 reply; 23+ messages in thread From: James Morris @ 2010-11-15 22:16 UTC (permalink / raw) To: Linus Torvalds Cc: Eric Paris, Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, LSM List 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> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 22:16 ` James Morris @ 2010-11-15 22:36 ` Linus Torvalds 2010-11-15 22:51 ` James Morris 0 siblings, 1 reply; 23+ messages in thread From: Linus Torvalds @ 2010-11-15 22:36 UTC (permalink / raw) To: James Morris Cc: Eric Paris, Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, LSM List 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-15 22:36 ` Linus Torvalds @ 2010-11-15 22:51 ` James Morris 0 siblings, 0 replies; 23+ messages in thread From: James Morris @ 2010-11-15 22:51 UTC (permalink / raw) To: Linus Torvalds Cc: Eric Paris, Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Kees Cook, Andrew Morton, LSM List 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> ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n 2010-11-13 17:50 ` Linus Torvalds 2010-11-13 18:25 ` Dan Rosenberg 2010-11-13 19:51 ` Joe Perches @ 2010-11-14 2:44 ` Kees Cook 2 siblings, 0 replies; 23+ messages in thread From: Kees Cook @ 2010-11-14 2:44 UTC (permalink / raw) To: Linus Torvalds Cc: Joe Perches, Dan Rosenberg, LKML, Ingo Molnar, Eugene Teo, Andrew Morton 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2010-11-15 23:09 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-11-13 17:26 [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=y and CONFIG_PRINTK=n Joe Perches 2010-11-13 17:50 ` Linus Torvalds 2010-11-13 18:25 ` Dan Rosenberg 2010-11-13 20:22 ` Linus Torvalds 2010-11-14 3:05 ` Kees Cook 2010-11-14 12:35 ` Ingo Molnar 2010-11-13 19:51 ` Joe Perches 2010-11-13 20:01 ` Joe Perches 2010-11-13 20:31 ` Linus Torvalds 2010-11-15 1:16 ` James Morris 2010-11-15 17:04 ` Eric Paris 2010-11-15 17:34 ` Eric Paris 2010-11-15 17:41 ` Linus Torvalds 2010-11-15 17:45 ` Eric Paris 2010-11-15 18:20 ` Linus Torvalds 2010-11-15 22:13 ` James Morris 2010-11-15 22:43 ` Eric Paris 2010-11-15 22:58 ` James Morris 2010-11-15 23:08 ` Eric Paris 2010-11-15 22:16 ` James Morris 2010-11-15 22:36 ` Linus Torvalds 2010-11-15 22:51 ` James Morris 2010-11-14 2:44 ` Kees Cook
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).