Messages in this thread | | | Date | Thu, 4 Jun 2015 10:00:14 -0700 | Subject | Re: [PATCH v2] security_syslog() should be called once only | From | Kees Cook <> |
| |
On Wed, May 27, 2015 at 4:43 PM, Andrew Morton <akpm@linux-foundation.org> wrote: > 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?
No, it's just been historically ugly code. I tried to improve it (before it was just using hard coded numbers) by adding the #defines for the actions.
The fundamental issue is that the syslog syscall interface has the ability to perform handle-less actions. We need to security-check all the syscall actions, but only the "open" on (at the time) /proc/kmsg. Adding devkmsg then gave us a third entry point, and it got even uglier.
So, the semantics for the security check depends on what was historically called "type" (but is better known by its SYSLOG_ACTION_* values). Linus wants security checking done on open where possible, so for files, the checking must be done against an implicit "SYSLOG_ACTION_OPEN". For the syscall, each action is individually checked.
With that in mind, what can we do to make this sensible, and cover /proc/kmsg, /dev/kmsg, and the syscall interface?
-Kees
-- Kees Cook Chrome OS Security
| |