From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753038AbbE0XnQ (ORCPT <rfc822;w@1wt.eu>);
	Wed, 27 May 2015 19:43:16 -0400
Received: from mail.linuxfoundation.org ([140.211.169.12]:59040 "EHLO
	mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1752789AbbE0XnN (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Wed, 27 May 2015 19:43:13 -0400
Date: Wed, 27 May 2015 16:43:12 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: Vasily Averin <vvs@virtuozzo.com>
Cc: linux-kernel@vger.kernel.org, Kees Cook <keescook@chromium.org>,
        Josh Boyer <jwboyer@redhat.com>, Eric Paris <eparis@redhat.com>
Subject: Re: [PATCH v2] security_syslog() should be called once only
Message-Id: <20150527164312.a22ad8bb748acaddbea3bf70@linux-foundation.org>
In-Reply-To: <5561F9E0.6050504@virtuozzo.com>
References: <20150514150154.dbfb8ab275aa30d0fe93172b@linux-foundation.org>
	<5561F9E0.6050504@virtuozzo.com>
X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu)
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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?