Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932521AbbFDRAh (ORCPT ); Thu, 4 Jun 2015 13:00:37 -0400 Received: from mail-vn0-f50.google.com ([209.85.216.50]:44482 "EHLO mail-vn0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932120AbbFDRAQ (ORCPT ); Thu, 4 Jun 2015 13:00:16 -0400 MIME-Version: 1.0 In-Reply-To: <20150527164312.a22ad8bb748acaddbea3bf70@linux-foundation.org> References: <20150514150154.dbfb8ab275aa30d0fe93172b@linux-foundation.org> <5561F9E0.6050504@virtuozzo.com> <20150527164312.a22ad8bb748acaddbea3bf70@linux-foundation.org> Date: Thu, 4 Jun 2015 10:00:14 -0700 X-Google-Sender-Auth: lOm2alV1Ejff3vdlizveLyQvKes Message-ID: Subject: Re: [PATCH v2] security_syslog() should be called once only From: Kees Cook To: Andrew Morton Cc: Vasily Averin , LKML , Josh Boyer , Eric Paris Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3873 Lines: 107 On Wed, May 27, 2015 at 4:43 PM, Andrew Morton wrote: > On Sun, 24 May 2015 19:18:40 +0300 Vasily Averin 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/