Messages in this thread Patch in this message | | | Date | Wed, 27 May 2015 16:43:12 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH v2] security_syslog() should be called once only |
| |
On Sun, 24 May 2015 19:18:40 +0300 Vasily Averin <vvs@virtuozzo.com> wrote:
> v2: subject changed, patch comment modified > > Fixes: 637241a900cb ("kmsg: honor dmesg_restrict sysctl on /dev/kmsg") > > Final version of patch 637241a900cb ("kmsg: honor dmesg_restrict sysctl > on /dev/kmsg") lost few hooks, as result security_syslog() are processed > incorrectly: > - open of /dev/kmsg checks syslog access permissions by using > check_syslog_permissions() where security_syslog() is not called > if dmesg_restrict is set. > - syslog syscall and /proc/kmsg calls do_syslog() > where security_syslog can be executed twice > (inside check_syslog_permissions() and then directly in do_syslog()) > > With this patch security_syslog() is called once only in all syslog-related > operations regardless of dmesg_restrict value. > > ... > > --- a/kernel/printk/printk.c > +++ b/kernel/printk/printk.c > @@ -484,11 +484,11 @@ int check_syslog_permissions(int type, bool from_file) > * already done the capabilities checks at open time. > */ > if (from_file && type != SYSLOG_ACTION_OPEN) > - return 0; > + goto ok;
So we run security_syslog() for actions other than open() (of kmsg). Why?
Also, that from_file handling makes me cry.
#define SYSLOG_FROM_READER 0 #define SYSLOG_FROM_PROC 1
That's not a boolean - it's an enumerated value with two values currently defined.
But the code in check_syslog_permissions() treats it as a boolean and also hardwires the knowledge that SYSLOG_FROM_PROC == 1 (or == `true`).
And the name is wrong: it should be called from_proc to match SYSLOG_FROM_PROC.
One possible fix would be something like this, plus various fixups/audit:
--- a/kernel/printk/printk.c~security_syslog-should-be-called-once-only-fix +++ a/kernel/printk/printk.c @@ -489,13 +489,13 @@ static int syslog_action_restricted(int type != SYSLOG_ACTION_SIZE_BUFFER; } -int check_syslog_permissions(int type, bool from_file) +int check_syslog_permissions(int type, int source) { /* * If this is from /proc/kmsg and we've already opened it, then we've * already done the capabilities checks at open time. */ - if (from_file && type != SYSLOG_ACTION_OPEN) + if (source == SYSLOG_FROM_PROC && type != SYSLOG_ACTION_OPEN) goto ok; if (syslog_action_restricted(type)) { _
And `type' should be renamed to `action' for heavens sake. Kees, were you drunk?
| |