Re: [PATCH] Fix dmesg_restrict build failure with CONFIG_EMBEDDED=yand CONFIG_PRINTK=n
From: Linus Torvalds
Date: Sat Nov 13 2010 - 15:23:17 EST
On Sat, Nov 13, 2010 at 10:25 AM, Dan Rosenberg
<drosenberg@xxxxxxxxxxxxx> 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
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;