[PATCH] syslog: check cap_syslog when dmesg_restrict

[PATCH] syslog: check cap_syslog when dmesg_restrict

[Serge E. Hallyn]

Eric Paris pointed out that it doesn't make sense to require
both CAP_SYS_ADMIN and CAP_SYSLOG for certain syslog actions.
So require CAP_SYSLOG, not CAP_SYS_ADMIN, when dmesg_restrict
is set.

(I'm also consolidating the now common error path)

Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com>
---
 Documentation/sysctl/kernel.txt |    2 +-
 kernel/printk.c                 |   20 ++++++++++----------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 209e158..5740671 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -219,7 +219,7 @@ dmesg_restrict:
 This toggle indicates whether unprivileged users are prevented from using
 dmesg(8) to view messages from the kernel's log buffer.  When
 dmesg_restrict is set to (0) there are no restrictions.  When
-dmesg_restrict is set set to (1), users must have CAP_SYS_ADMIN to use
+dmesg_restrict is set set to (1), users must have CAP_SYSLOG to use
 dmesg(8).
 
 The kernel config option CONFIG_SECURITY_DMESG_RESTRICT sets the default
diff --git a/kernel/printk.c b/kernel/printk.c
index 0712380..0cecba0 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -279,18 +279,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	 * at open time.
 	 */
 	if (type == SYSLOG_ACTION_OPEN || !from_file) {
-		if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
-			return -EPERM;
+		if (dmesg_restrict && !capable(CAP_SYSLOG))
+			goto warn; /* switch to return -EPERM after 2.6.39 */
 		if ((type != SYSLOG_ACTION_READ_ALL &&
 		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
-		    !capable(CAP_SYSLOG)) {
-			/* remove after 2.6.38 */
-			if (capable(CAP_SYS_ADMIN))
-				WARN_ONCE(1, "Attempt to access syslog with "
-				  "CAP_SYS_ADMIN but no CAP_SYSLOG "
-				  "(deprecated and denied).\n");
-			return -EPERM;
-		}
+		    !capable(CAP_SYSLOG))
+			goto warn; /* switch to return -EPERM after 2.6.39 */
 	}
 
 	error = security_syslog(type);
@@ -434,6 +428,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	}
 out:
 	return error;
+warn:
+	/* remove after 2.6.39 */
+	if (capable(CAP_SYS_ADMIN))
+		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
+		  "but no CAP_SYSLOG (deprecated and denied).\n");
+	return -EPERM;
 }
 
 SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)
-- 
1.7.0.4

Re: [PATCH] syslog: check cap_syslog when dmesg_restrict

[Eric Paris]

On Wed, 2010-12-08 at 15:19 +0000, Serge E. Hallyn wrote:
> Eric Paris pointed out that it doesn't make sense to require
> both CAP_SYS_ADMIN and CAP_SYSLOG for certain syslog actions.
> So require CAP_SYSLOG, not CAP_SYS_ADMIN, when dmesg_restrict
> is set.
> 
> (I'm also consolidating the now common error path)
> 
> Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com>
> ---
>  Documentation/sysctl/kernel.txt |    2 +-
>  kernel/printk.c                 |   20 ++++++++++----------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 209e158..5740671 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -219,7 +219,7 @@ dmesg_restrict:
>  This toggle indicates whether unprivileged users are prevented from using
>  dmesg(8) to view messages from the kernel's log buffer.  When
>  dmesg_restrict is set to (0) there are no restrictions.  When
> -dmesg_restrict is set set to (1), users must have CAP_SYS_ADMIN to use
> +dmesg_restrict is set set to (1), users must have CAP_SYSLOG to use
>  dmesg(8).
>  
>  The kernel config option CONFIG_SECURITY_DMESG_RESTRICT sets the default
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 0712380..0cecba0 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -279,18 +279,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	 * at open time.
>  	 */
>  	if (type == SYSLOG_ACTION_OPEN || !from_file) {
> -		if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> -			return -EPERM;
> +		if (dmesg_restrict && !capable(CAP_SYSLOG))
> +			goto warn; /* switch to return -EPERM after 2.6.39 */
>  		if ((type != SYSLOG_ACTION_READ_ALL &&
>  		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
> -		    !capable(CAP_SYSLOG)) {
> -			/* remove after 2.6.38 */
> -			if (capable(CAP_SYS_ADMIN))
> -				WARN_ONCE(1, "Attempt to access syslog with "
> -				  "CAP_SYS_ADMIN but no CAP_SYSLOG "
> -				  "(deprecated and denied).\n");
> -			return -EPERM;
> -		}
> +		    !capable(CAP_SYSLOG))
> +			goto warn; /* switch to return -EPERM after 2.6.39 */

Doesn't this return -EPERM right now?  I think the code might be
incorrect today as well......

I thought the flow was supposed to be

if (capable(CAP_SYSLOG))
	all good
else if (capable(CAP_SYS_ADMIN))
	WARN, but still good for now
else
	EPERM

But it looks to me like the flow is

if (capable(CAP_SYSLOG))
	all good
else if (capable(CAP_SYS_ADMIN))
	WARN, EPERM
else
	EPERM

>  	}
>  
>  	error = security_syslog(type);
> @@ -434,6 +428,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	}
>  out:
>  	return error;
> +warn:
> +	/* remove after 2.6.39 */
> +	if (capable(CAP_SYS_ADMIN))
> +		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
> +		  "but no CAP_SYSLOG (deprecated and denied).\n");
> +	return -EPERM;
>  }
>  
>  SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)

Re: [PATCH] syslog: check cap_syslog when dmesg_restrict

[Serge E. Hallyn]

Quoting Eric Paris (eparis@redhat.com):
> On Wed, 2010-12-08 at 15:19 +0000, Serge E. Hallyn wrote:
> Doesn't this return -EPERM right now?

Yes.

>   I think the code might be
> incorrect today as well......
> 
> I thought the flow was supposed to be
> 
> if (capable(CAP_SYSLOG))
> 	all good
> else if (capable(CAP_SYS_ADMIN))
> 	WARN, but still good for now

I prefer warn and deny.  Otherwise it's too easy to ignore warnings.  So
I prefer the msg to be there to explain why it failed - not that I expect
it to fail for anyone today.

> else
> 	EPERM
> 
> But it looks to me like the flow is
> 
> if (capable(CAP_SYSLOG))
> 	all good
> else if (capable(CAP_SYS_ADMIN))
> 	WARN, EPERM
> else
> 	EPERM
> 
> >  	}

Yup.

-serge

Re: [PATCH] syslog: check cap_syslog when dmesg_restrict

[Eric Paris]

On Wed, 2010-12-08 at 16:42 +0000, Serge E. Hallyn wrote:
> Quoting Eric Paris (eparis@redhat.com):

> >   I think the code might be
> > incorrect today as well......
> > 
> > I thought the flow was supposed to be
> > 
> > if (capable(CAP_SYSLOG))
> > 	all good
> > else if (capable(CAP_SYS_ADMIN))
> > 	WARN, but still good for now
> 
> I prefer warn and deny.  Otherwise it's too easy to ignore warnings.  So
> I prefer the msg to be there to explain why it failed - not that I expect
> it to fail for anyone today.

Ok, fine with me.

Acked-by: Eric Paris <eparis@redhat.com>

Re: [PATCH] syslog: check cap_syslog when dmesg_restrict

[Kees Cook]

On Wed, Dec 08, 2010 at 03:19:01PM +0000, Serge E. Hallyn wrote:
> Eric Paris pointed out that it doesn't make sense to require
> both CAP_SYS_ADMIN and CAP_SYSLOG for certain syslog actions.
> So require CAP_SYSLOG, not CAP_SYS_ADMIN, when dmesg_restrict
> is set.
> 
> (I'm also consolidating the now common error path)
> 
> Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com>

Acked-by: Kees Cook <kees.cook@canonical.com>

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] syslog: check cap_syslog when dmesg_restrict

[James Morris]

On Wed, 8 Dec 2010, Serge E. Hallyn wrote:

> Eric Paris pointed out that it doesn't make sense to require
> both CAP_SYS_ADMIN and CAP_SYSLOG for certain syslog actions.
> So require CAP_SYSLOG, not CAP_SYS_ADMIN, when dmesg_restrict
> is set.
> 
> (I'm also consolidating the now common error path)
> 
> Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com>

Applied.

(Please cc the lsm list with security patches).

> ---
>  Documentation/sysctl/kernel.txt |    2 +-
>  kernel/printk.c                 |   20 ++++++++++----------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 209e158..5740671 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -219,7 +219,7 @@ dmesg_restrict:
>  This toggle indicates whether unprivileged users are prevented from using
>  dmesg(8) to view messages from the kernel's log buffer.  When
>  dmesg_restrict is set to (0) there are no restrictions.  When
> -dmesg_restrict is set set to (1), users must have CAP_SYS_ADMIN to use
> +dmesg_restrict is set set to (1), users must have CAP_SYSLOG to use
>  dmesg(8).
>  
>  The kernel config option CONFIG_SECURITY_DMESG_RESTRICT sets the default
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 0712380..0cecba0 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -279,18 +279,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	 * at open time.
>  	 */
>  	if (type == SYSLOG_ACTION_OPEN || !from_file) {
> -		if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
> -			return -EPERM;
> +		if (dmesg_restrict && !capable(CAP_SYSLOG))
> +			goto warn; /* switch to return -EPERM after 2.6.39 */
>  		if ((type != SYSLOG_ACTION_READ_ALL &&
>  		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
> -		    !capable(CAP_SYSLOG)) {
> -			/* remove after 2.6.38 */
> -			if (capable(CAP_SYS_ADMIN))
> -				WARN_ONCE(1, "Attempt to access syslog with "
> -				  "CAP_SYS_ADMIN but no CAP_SYSLOG "
> -				  "(deprecated and denied).\n");
> -			return -EPERM;
> -		}
> +		    !capable(CAP_SYSLOG))
> +			goto warn; /* switch to return -EPERM after 2.6.39 */
>  	}
>  
>  	error = security_syslog(type);
> @@ -434,6 +428,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	}
>  out:
>  	return error;
> +warn:
> +	/* remove after 2.6.39 */
> +	if (capable(CAP_SYS_ADMIN))
> +		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
> +		  "but no CAP_SYSLOG (deprecated and denied).\n");
> +	return -EPERM;
>  }
>  
>  SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)
> -- 
> 1.7.0.4
> 

-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] syslog: check cap_syslog when dmesg_restrict

[Daniel J Walsh]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/08/2010 05:56 PM, James Morris wrote:
> On Wed, 8 Dec 2010, Serge E. Hallyn wrote:
> 
>> Eric Paris pointed out that it doesn't make sense to require
>> both CAP_SYS_ADMIN and CAP_SYSLOG for certain syslog actions.
>> So require CAP_SYSLOG, not CAP_SYS_ADMIN, when dmesg_restrict
>> is set.
>>
>> (I'm also consolidating the now common error path)
>>
>> Signed-off-by: Serge E. Hallyn <serge.hallyn@canonical.com>
> 
> Applied.
> 
> (Please cc the lsm list with security patches).
> 
>> ---
>>  Documentation/sysctl/kernel.txt |    2 +-
>>  kernel/printk.c                 |   20 ++++++++++----------
>>  2 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
>> index 209e158..5740671 100644
>> --- a/Documentation/sysctl/kernel.txt
>> +++ b/Documentation/sysctl/kernel.txt
>> @@ -219,7 +219,7 @@ dmesg_restrict:
>>  This toggle indicates whether unprivileged users are prevented from using
>>  dmesg(8) to view messages from the kernel's log buffer.  When
>>  dmesg_restrict is set to (0) there are no restrictions.  When
>> -dmesg_restrict is set set to (1), users must have CAP_SYS_ADMIN to use
>> +dmesg_restrict is set set to (1), users must have CAP_SYSLOG to use
>>  dmesg(8).
>>  
>>  The kernel config option CONFIG_SECURITY_DMESG_RESTRICT sets the default
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index 0712380..0cecba0 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -279,18 +279,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>>  	 * at open time.
>>  	 */
>>  	if (type == SYSLOG_ACTION_OPEN || !from_file) {
>> -		if (dmesg_restrict && !capable(CAP_SYS_ADMIN))
>> -			return -EPERM;
>> +		if (dmesg_restrict && !capable(CAP_SYSLOG))
>> +			goto warn; /* switch to return -EPERM after 2.6.39 */
>>  		if ((type != SYSLOG_ACTION_READ_ALL &&
>>  		     type != SYSLOG_ACTION_SIZE_BUFFER) &&
>> -		    !capable(CAP_SYSLOG)) {
>> -			/* remove after 2.6.38 */
>> -			if (capable(CAP_SYS_ADMIN))
>> -				WARN_ONCE(1, "Attempt to access syslog with "
>> -				  "CAP_SYS_ADMIN but no CAP_SYSLOG "
>> -				  "(deprecated and denied).\n");
>> -			return -EPERM;
>> -		}
>> +		    !capable(CAP_SYSLOG))
>> +			goto warn; /* switch to return -EPERM after 2.6.39 */
>>  	}
>>  
>>  	error = security_syslog(type);
>> @@ -434,6 +428,12 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>>  	}
>>  out:
>>  	return error;
>> +warn:
>> +	/* remove after 2.6.39 */
>> +	if (capable(CAP_SYS_ADMIN))
>> +		WARN_ONCE(1, "Attempt to access syslog with CAP_SYS_ADMIN "
>> +		  "but no CAP_SYSLOG (deprecated and denied).\n");
>> +	return -EPERM;
>>  }
>>  
>>  SYSCALL_DEFINE3(syslog, int, type, char __user *, buf, int, len)
>> -- 
>> 1.7.0.4
>>
> 

Does anyone have an idea of which domains are going to be effected by
this change?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk0CMvcACgkQrlYvE4MpobP++gCgyJtjhYDfgXnc0TBOGseOpF67
zHoAn3bEditZdnj/OLGInp7FeCaxNQXH
=cOvZ
-----END PGP SIGNATURE-----