[PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

Originally, the addition of dmesg_restrict covered both the syslog
method of accessing dmesg, as well as /dev/kmsg itself.  This was done
indirectly by security_syslog calling cap_syslog before doing any LSM
checks.

However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
logic to fix build failure) moved the code around and pushed the checks
into the caller itself.  That seems to have inadvertently dropped the
checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
because util-linux dmesg(1) defaults to using the syslog method for
access in older versions.  With util-linux 2.22 and a kernel newer than
3.5, dmesg(1) defaults to reading directly from /dev/kmsg.

Fix this by making an explicit check in the devkmsg_open function.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

Reported-by: Christian Kujau <lists@nerdbynature.de>
CC: stable@vger.kernel.org
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 kernel/printk.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/kernel/printk.c b/kernel/printk.c
index f24633a..398ef9a 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	struct devkmsg_user *user;
 	int err;
 
+	if (dmesg_restrict && !capable(CAP_SYSLOG))
+		return -EACCES;
+
 	/* write-only does not need any file context */
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
-- 
1.8.1.2

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> Originally, the addition of dmesg_restrict covered both the syslog
> method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> indirectly by security_syslog calling cap_syslog before doing any LSM
> checks.
> 
> However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
> logic to fix build failure) moved the code around and pushed the checks
> into the caller itself.  That seems to have inadvertently dropped the
> checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
> because util-linux dmesg(1) defaults to using the syslog method for
> access in older versions.  With util-linux 2.22 and a kernel newer than
> 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
> 
> Fix this by making an explicit check in the devkmsg_open function.
> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> 
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> CC: stable@vger.kernel.org
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> ---
>  kernel/printk.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index f24633a..398ef9a 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	struct devkmsg_user *user;
>  	int err;
>  
> +	if (dmesg_restrict && !capable(CAP_SYSLOG))
> +		return -EACCES;
> +

I think this should use check_syslog_permissions() instead, as done for
/proc/kmsg and the syslog syscall.

	err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);
	if (err)
		return err;

And going forward we should probably think about dropping the CAP_SYS_ADMIN
backward-compat code in check_syslog_permissions.

>  	/* write-only does not need any file context */
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote:
> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> > Originally, the addition of dmesg_restrict covered both the syslog
> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> > indirectly by security_syslog calling cap_syslog before doing any LSM
> > checks.
> > 
> > However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
> > logic to fix build failure) moved the code around and pushed the checks
> > into the caller itself.  That seems to have inadvertently dropped the
> > checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
> > because util-linux dmesg(1) defaults to using the syslog method for
> > access in older versions.  With util-linux 2.22 and a kernel newer than
> > 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
> > 
> > Fix this by making an explicit check in the devkmsg_open function.
> > 
> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> > 
> > Reported-by: Christian Kujau <lists@nerdbynature.de>
> > CC: stable@vger.kernel.org
> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> > ---
> >  kernel/printk.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/kernel/printk.c b/kernel/printk.c
> > index f24633a..398ef9a 100644
> > --- a/kernel/printk.c
> > +++ b/kernel/printk.c
> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
> >  	struct devkmsg_user *user;
> >  	int err;
> >  
> > +	if (dmesg_restrict && !capable(CAP_SYSLOG))
> > +		return -EACCES;
> > +
> 
> I think this should use check_syslog_permissions() instead, as done for
> /proc/kmsg and the syslog syscall.
> 
> 	err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);

Did you mean SYSLOG_ACTION_OPEN?

I didn't code it that way because the comment in that function about the
capability checks already being done seem pretty off to me.  I could
have just misread the /proc code though.  I can resend with the change
you suggest if everyone thinks that's a better way.

Also, the LSM hooks aren't doing any capability checks at all that I can
see, which may or may not be a bug in and of itself but I have no idea.
I was hoping Eric would speak up about that.

> 	if (err)
> 		return err;
> 
> And going forward we should probably think about dropping the CAP_SYS_ADMIN
> backward-compat code in check_syslog_permissions.

Sure, but that's a separate commit.

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

Hi,

On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> Originally, the addition of dmesg_restrict covered both the syslog
> method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> indirectly by security_syslog calling cap_syslog before doing any LSM
> checks.

Actually, are the security_syslog() checks in /dev/kmsg correct? There is
only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't
it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to
devkmsg_read? (And should we add one for write?)

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Feb 27, 2013 at 10:05:47AM -0800, Kees Cook wrote:
> Hi,
> 
> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> > Originally, the addition of dmesg_restrict covered both the syslog
> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> > indirectly by security_syslog calling cap_syslog before doing any LSM
> > checks.
> 
> Actually, are the security_syslog() checks in /dev/kmsg correct? There is
> only one used in devkmsg_open which uses SYSLOG_ACTION_READ_ALL. Shouldn't
> it be using SYSLOG_ACTION_OPEN? And have SYSLOG_ACTION_READ_ALL added to
> devkmsg_read? (And should we add one for write?)

You ask wonderful questions!  I have no idea.  Either way, it needs to
still somehow check for capable(CAP_SYSLOG) which security_syslog
doesn't do.  So either what I have already, or your suggestion of using
the existing function with SYSLOG_ACTION_OPEN in devkmsg_open.

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote:
>> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
>> > Originally, the addition of dmesg_restrict covered both the syslog
>> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
>> > indirectly by security_syslog calling cap_syslog before doing any LSM
>> > checks.
>> >
>> > However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
>> > logic to fix build failure) moved the code around and pushed the checks
>> > into the caller itself.  That seems to have inadvertently dropped the
>> > checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
>> > because util-linux dmesg(1) defaults to using the syslog method for
>> > access in older versions.  With util-linux 2.22 and a kernel newer than
>> > 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
>> >
>> > Fix this by making an explicit check in the devkmsg_open function.
>> >
>> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>> >
>> > Reported-by: Christian Kujau <lists@nerdbynature.de>
>> > CC: stable@vger.kernel.org
>> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
>> > ---
>> >  kernel/printk.c | 3 +++
>> >  1 file changed, 3 insertions(+)
>> >
>> > diff --git a/kernel/printk.c b/kernel/printk.c
>> > index f24633a..398ef9a 100644
>> > --- a/kernel/printk.c
>> > +++ b/kernel/printk.c
>> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>> >     struct devkmsg_user *user;
>> >     int err;
>> >
>> > +   if (dmesg_restrict && !capable(CAP_SYSLOG))
>> > +           return -EACCES;
>> > +
>>
>> I think this should use check_syslog_permissions() instead, as done for
>> /proc/kmsg and the syslog syscall.
>>
>>       err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);
>
> Did you mean SYSLOG_ACTION_OPEN?

Oops, yes, typo.

> I didn't code it that way because the comment in that function about the
> capability checks already being done seem pretty off to me.  I could
> have just misread the /proc code though.  I can resend with the change
> you suggest if everyone thinks that's a better way.

Yeah, the comment is meaningful if you examine how /proc/kmsg works,
which was basically as a wrapper to the syslog syscall. The issue was
that we had to catch (and potentially block) the "open" action on
/proc/kmsg vs blocking the the syslog read action. In this case, we've
got another file-based interface, so we should use OPEN and FROM_FILE
to block the open. (Though it could be argued that we only want to
block the open if it's reading, which is exactly what Kay was trying
to do originally it looks like.)

> Also, the LSM hooks aren't doing any capability checks at all that I can
> see, which may or may not be a bug in and of itself but I have no idea.
> I was hoping Eric would speak up about that.

Eric explicitly removed the cap check since it was cluttering things
the way it was originally written. I do think security_syslog() should
pass through check_syslog_permissions(), though. Then this wouldn't
have happened. That might actually be the right way to clean this up,
but I'd like to see Eric's thoughts first.

>>       if (err)
>>               return err;
>>
>> And going forward we should probably think about dropping the CAP_SYS_ADMIN
>> backward-compat code in check_syslog_permissions.
>
> Sure, but that's a separate commit.

Yup.

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Eric Paris]

Fine Fine, I'll get off my lazy butt and look at this.

On Wed, 2013-02-27 at 10:14 -0800, Kees Cook wrote:
> On Wed, Feb 27, 2013 at 10:01 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> > On Wed, Feb 27, 2013 at 09:54:27AM -0800, Kees Cook wrote:
> >> On Fri, Feb 22, 2013 at 01:18:57PM -0500, Josh Boyer wrote:
> >> > Originally, the addition of dmesg_restrict covered both the syslog
> >> > method of accessing dmesg, as well as /dev/kmsg itself.  This was done
> >> > indirectly by security_syslog calling cap_syslog before doing any LSM
> >> > checks.
> >> >
> >> > However, commit 12b3052c3ee (capabilities/syslog: open code cap_syslog
> >> > logic to fix build failure) moved the code around and pushed the checks
> >> > into the caller itself.  That seems to have inadvertently dropped the
> >> > checks for dmesg_restrict on /dev/kmsg.

Not sure this is correct.  There was no devkmsg_open() in commit
12b3052c3ee.  That was added in e11fea92e.  Looks like before that
commit the devkmsg code was even worse.  It didn't call security_syslog
or capable().  Uggh.

>   Most people haven't noticed
> >> > because util-linux dmesg(1) defaults to using the syslog method for
> >> > access in older versions.  With util-linux 2.22 and a kernel newer than
> >> > 3.5, dmesg(1) defaults to reading directly from /dev/kmsg.
> >> >
> >> > Fix this by making an explicit check in the devkmsg_open function.
> >> >
> >> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> >> >
> >> > Reported-by: Christian Kujau <lists@nerdbynature.de>
> >> > CC: stable@vger.kernel.org
> >> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> >> > ---
> >> >  kernel/printk.c | 3 +++
> >> >  1 file changed, 3 insertions(+)
> >> >
> >> > diff --git a/kernel/printk.c b/kernel/printk.c
> >> > index f24633a..398ef9a 100644
> >> > --- a/kernel/printk.c
> >> > +++ b/kernel/printk.c
> >> > @@ -615,6 +615,9 @@ static int devkmsg_open(struct inode *inode, struct file *file)
> >> >     struct devkmsg_user *user;
> >> >     int err;
> >> >
> >> > +   if (dmesg_restrict && !capable(CAP_SYSLOG))
> >> > +           return -EACCES;
> >> > +
> >>
> >> I think this should use check_syslog_permissions() instead, as done for
> >> /proc/kmsg and the syslog syscall.
> >>
> >>       err = check_syslog_permissions(SYSLOG_ACTION_OPTION, SYSLOG_FROM_FILE);
> >
> > Did you mean SYSLOG_ACTION_OPEN?
> 
> Oops, yes, typo.
> 
> > I didn't code it that way because the comment in that function about the
> > capability checks already being done seem pretty off to me.  I could
> > have just misread the /proc code though.  I can resend with the change
> > you suggest if everyone thinks that's a better way.
> 
> Yeah, the comment is meaningful if you examine how /proc/kmsg works,
> which was basically as a wrapper to the syslog syscall. The issue was
> that we had to catch (and potentially block) the "open" action on
> /proc/kmsg vs blocking the the syslog read action. In this case, we've
> got another file-based interface, so we should use OPEN and FROM_FILE
> to block the open. (Though it could be argued that we only want to
> block the open if it's reading, which is exactly what Kay was trying
> to do originally it looks like.)

Right.  Now we have /proc/kmsg, /dev/kmsg, and the syscall.  /proc/kmsg
and the syscall both use do_syslog() which calls
check_syslog_permissions() and security_syslog().  /dev/kmsg only calls
security_syslog(), which we all agree needs fixed.

> > Also, the LSM hooks aren't doing any capability checks at all that I can
> > see, which may or may not be a bug in and of itself but I have no idea.
> > I was hoping Eric would speak up about that.

I wouldn't call it a bug.  But it sure is a pretty shitty design pattern
to have security_* sometimes the right thing to do and sometimes
capable() is the right thing to do.  It is pervasive in the kernel that
you have either/or, but I can't think of anywhere that functions are
expected to do BOTH.  So yeah, that needs fixed.

> Eric explicitly removed the cap check since it was cluttering things
> the way it was originally written. I do think security_syslog() should
> pass through check_syslog_permissions(), though. Then this wouldn't
> have happened. That might actually be the right way to clean this up,
> but I'd like to see Eric's thoughts first.

How about something like this?

diff --git a/kernel/printk.c b/kernel/printk.c
index 7c69b3e..ced2cac 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -626,7 +626,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
 
-	err = security_syslog(SYSLOG_ACTION_READ_ALL);
+	err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);
 	if (err)
 		return err;
 
@@ -840,22 +840,23 @@ static 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;
 
 	if (syslog_action_restricted(type)) {
 		if (capable(CAP_SYSLOG))
-			return 0;
+			goto ok;
 		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
 		if (capable(CAP_SYS_ADMIN)) {
 			printk_once(KERN_WARNING "%s (%d): "
 				 "Attempt to access syslog with CAP_SYS_ADMIN "
 				 "but no CAP_SYSLOG (deprecated).\n",
 				 current->comm, task_pid_nr(current));
-			return 0;
+			goto ok;
 		}
 		return -EPERM;
 	}
-	return 0;
+ok:
+	return security_syslog(type);
 }
 
 #if defined(CONFIG_PRINTK_TIME)
@@ -1133,10 +1134,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	if (error)
 		goto out;
 
-	error = security_syslog(type);
-	if (error)
-		return error;
-
 	switch (type) {
 	case SYSLOG_ACTION_CLOSE:	/* Close log */
 		break;

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
> Fine Fine, I'll get off my lazy butt and look at this.

Shock!

> Right.  Now we have /proc/kmsg, /dev/kmsg, and the syscall.  /proc/kmsg
> and the syscall both use do_syslog() which calls
> check_syslog_permissions() and security_syslog().  /dev/kmsg only calls
> security_syslog(), which we all agree needs fixed.
> 
> > > Also, the LSM hooks aren't doing any capability checks at all that I can
> > > see, which may or may not be a bug in and of itself but I have no idea.
> > > I was hoping Eric would speak up about that.
> 
> I wouldn't call it a bug.  But it sure is a pretty shitty design pattern
> to have security_* sometimes the right thing to do and sometimes
> capable() is the right thing to do.  It is pervasive in the kernel that
> you have either/or, but I can't think of anywhere that functions are
> expected to do BOTH.  So yeah, that needs fixed.

OK.

> 
> > Eric explicitly removed the cap check since it was cluttering things
> > the way it was originally written. I do think security_syslog() should
> > pass through check_syslog_permissions(), though. Then this wouldn't
> > have happened. That might actually be the right way to clean this up,
> > but I'd like to see Eric's thoughts first.
> 
> How about something like this?

I think this looks pretty good.  Much clearer overall and the
consolidation is nice.  I'll try to get it tested soon.

josh

> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index 7c69b3e..ced2cac 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -626,7 +626,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;
>  
> -	err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +	err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);
>  	if (err)
>  		return err;
>  
> @@ -840,22 +840,23 @@ static 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;
>  
>  	if (syslog_action_restricted(type)) {
>  		if (capable(CAP_SYSLOG))
> -			return 0;
> +			goto ok;
>  		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>  		if (capable(CAP_SYS_ADMIN)) {
>  			printk_once(KERN_WARNING "%s (%d): "
>  				 "Attempt to access syslog with CAP_SYS_ADMIN "
>  				 "but no CAP_SYSLOG (deprecated).\n",
>  				 current->comm, task_pid_nr(current));
> -			return 0;
> +			goto ok;
>  		}
>  		return -EPERM;
>  	}
> -	return 0;
> +ok:
> +	return security_syslog(type);
>  }
>  
>  #if defined(CONFIG_PRINTK_TIME)
> @@ -1133,10 +1134,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	if (error)
>  		goto out;
>  
> -	error = security_syslog(type);
> -	if (error)
> -		return error;
> -
>  	switch (type) {
>  	case SYSLOG_ACTION_CLOSE:	/* Close log */
>  		break;
> 
> 

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, Feb 27, 2013 at 2:19 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Wed, Feb 27, 2013 at 03:46:41PM -0500, Eric Paris wrote:
>> Fine Fine, I'll get off my lazy butt and look at this.
>
> Shock!
>
>> Right.  Now we have /proc/kmsg, /dev/kmsg, and the syscall.  /proc/kmsg
>> and the syscall both use do_syslog() which calls
>> check_syslog_permissions() and security_syslog().  /dev/kmsg only calls
>> security_syslog(), which we all agree needs fixed.
>>
>> > > Also, the LSM hooks aren't doing any capability checks at all that I can
>> > > see, which may or may not be a bug in and of itself but I have no idea.
>> > > I was hoping Eric would speak up about that.
>>
>> I wouldn't call it a bug.  But it sure is a pretty shitty design pattern
>> to have security_* sometimes the right thing to do and sometimes
>> capable() is the right thing to do.  It is pervasive in the kernel that
>> you have either/or, but I can't think of anywhere that functions are
>> expected to do BOTH.  So yeah, that needs fixed.
>
> OK.
>
>>
>> > Eric explicitly removed the cap check since it was cluttering things
>> > the way it was originally written. I do think security_syslog() should
>> > pass through check_syslog_permissions(), though. Then this wouldn't
>> > have happened. That might actually be the right way to clean this up,
>> > but I'd like to see Eric's thoughts first.
>>
>> How about something like this?
>
> I think this looks pretty good.  Much clearer overall and the
> consolidation is nice.  I'll try to get it tested soon.
>
> josh
>
>>
>> diff --git a/kernel/printk.c b/kernel/printk.c
>> index 7c69b3e..ced2cac 100644
>> --- a/kernel/printk.c
>> +++ b/kernel/printk.c
>> @@ -626,7 +626,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>>       if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>>               return 0;
>>
>> -     err = security_syslog(SYSLOG_ACTION_READ_ALL);
>> +     err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);

Yes, this looks correct with the consolidation below. Nice!

>>       if (err)
>>               return err;
>>
>> @@ -840,22 +840,23 @@ static 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;
>>
>>       if (syslog_action_restricted(type)) {
>>               if (capable(CAP_SYSLOG))
>> -                     return 0;
>> +                     goto ok;
>>               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>>               if (capable(CAP_SYS_ADMIN)) {
>>                       printk_once(KERN_WARNING "%s (%d): "
>>                                "Attempt to access syslog with CAP_SYS_ADMIN "
>>                                "but no CAP_SYSLOG (deprecated).\n",
>>                                current->comm, task_pid_nr(current));
>> -                     return 0;
>> +                     goto ok;
>>               }
>>               return -EPERM;
>>       }
>> -     return 0;
>> +ok:
>> +     return security_syslog(type);
>>  }
>>
>>  #if defined(CONFIG_PRINTK_TIME)
>> @@ -1133,10 +1134,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>>       if (error)
>>               goto out;
>>
>> -     error = security_syslog(type);
>> -     if (error)
>> -             return error;
>> -
>>       switch (type) {
>>       case SYSLOG_ACTION_CLOSE:       /* Close log */
>>               break;
>>
>>

I think for completeness, we need to add a
check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE)
call to devkmsg_read().

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Andrew Morton]

poke.  Nothing got applied.  I'll drop
kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
effect ;) 


From: Josh Boyer <jwboyer@redhat.com>
Subject: kmsg: honor dmesg_restrict sysctl on /dev/kmsg

Originally, the addition of dmesg_restrict covered both the syslog
method of accessing dmesg, as well as /dev/kmsg itself.  This was done
indirectly by security_syslog calling cap_syslog before doing any LSM
checks.

However, commit 12b3052c3ee ("capabilities/syslog: open code cap_syslog
logic to fix build failure") moved the code around and pushed the checks
into the caller itself.  That seems to have inadvertently dropped the
checks for dmesg_restrict on /dev/kmsg.  Most people haven't noticed
because util-linux dmesg(1) defaults to using the syslog method for access
in older versions.  With util-linux 2.22 and a kernel newer than 3.5,
dmesg(1) defaults to reading directly from /dev/kmsg.

Fix this by making an explicit check in the devkmsg_open function.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

Signed-off-by: Josh Boyer <jwboyer@redhat.com>
Reported-by: Christian Kujau <lists@nerdbynature.de>
Cc: Eric Paris <eparis@redhat.com>
Cc: James Morris <jmorris@namei.org>
Cc: <stable@vger.kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/printk.c |    3 +++
 1 file changed, 3 insertions(+)

diff -puN kernel/printk.c~kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg kernel/printk.c
--- a/kernel/printk.c~kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg
+++ a/kernel/printk.c
@@ -620,6 +620,9 @@ static int devkmsg_open(struct inode *in
 	struct devkmsg_user *user;
 	int err;
 
+	if (dmesg_restrict && !capable(CAP_SYSLOG))
+		return -EACCES;
+
 	/* write-only does not need any file context */
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
_

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> 
> poke.  Nothing got applied.  I'll drop
> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> effect ;) 

Oh dear.

Eric, were you going to cleanup your suggestion and send it out?

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
>>
>> poke.  Nothing got applied.  I'll drop
>> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
>> effect ;)
>
> Oh dear.
>
> Eric, were you going to cleanup your suggestion and send it out?

Ping? What state is this in?

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

----- Original Message -----
> From: "Kees Cook" <keescook@chromium.org>
> To: "Josh Boyer" <jwboyer@redhat.com>
> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Eric Paris" <eparis@redhat.com>, "Linus Torvalds"
> <torvalds@linux-foundation.org>, "Christian Kujau" <lists@nerdbynature.de>, "# 3.4.x" <stable@vger.kernel.org>,
> "LKML" <linux-kernel@vger.kernel.org>
> Sent: Monday, April 1, 2013 7:51:57 PM
> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
> 
> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> >>
> >> poke.  Nothing got applied.  I'll drop
> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> >> effect ;)
> >
> > Oh dear.
> >
> > Eric, were you going to cleanup your suggestion and send it out?
> 
> Ping? What state is this in?

If Eric doesn't send a patch tomorrow, I will.  Fedora is carrying my
original patch, but I'd rather get this fixed everywhere.

josh

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> ----- Original Message -----
>> From: "Kees Cook" <keescook@chromium.org>
>> To: "Josh Boyer" <jwboyer@redhat.com>
>> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Eric Paris" <eparis@redhat.com>, "Linus Torvalds"
>> <torvalds@linux-foundation.org>, "Christian Kujau" <lists@nerdbynature.de>, "# 3.4.x" <stable@vger.kernel.org>,
>> "LKML" <linux-kernel@vger.kernel.org>
>> Sent: Monday, April 1, 2013 7:51:57 PM
>> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
>>
>> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
>> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
>> >>
>> >> poke.  Nothing got applied.  I'll drop
>> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
>> >> effect ;)
>> >
>> > Oh dear.
>> >
>> > Eric, were you going to cleanup your suggestion and send it out?
>>
>> Ping? What state is this in?
>
> If Eric doesn't send a patch tomorrow, I will.  Fedora is carrying my
> original patch, but I'd rather get this fixed everywhere.

Pinging again here. Any news on this?

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Mon, Apr 08, 2013 at 02:34:38PM -0700, Kees Cook wrote:
> On Mon, Apr 1, 2013 at 6:05 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> > ----- Original Message -----
> >> From: "Kees Cook" <keescook@chromium.org>
> >> To: "Josh Boyer" <jwboyer@redhat.com>
> >> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Eric Paris" <eparis@redhat.com>, "Linus Torvalds"
> >> <torvalds@linux-foundation.org>, "Christian Kujau" <lists@nerdbynature.de>, "# 3.4.x" <stable@vger.kernel.org>,
> >> "LKML" <linux-kernel@vger.kernel.org>
> >> Sent: Monday, April 1, 2013 7:51:57 PM
> >> Subject: Re: [PATCH] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
> >>
> >> On Fri, Mar 22, 2013 at 3:14 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> >> > On Fri, Mar 22, 2013 at 02:54:48PM -0700, Andrew Morton wrote:
> >> >>
> >> >> poke.  Nothing got applied.  I'll drop
> >> >> kmsg-honor-dmesg_restrict-sysctl-on-dev-kmsg.patch, see if that has any
> >> >> effect ;)
> >> >
> >> > Oh dear.
> >> >
> >> > Eric, were you going to cleanup your suggestion and send it out?
> >>
> >> Ping? What state is this in?
> >
> > If Eric doesn't send a patch tomorrow, I will.  Fedora is carrying my
> > original patch, but I'd rather get this fixed everywhere.
> 
> Pinging again here. Any news on this?

I am now almost as delinquent as Eric.  I owe you one beer of your
choice.  You can add one beer to that tally every day until I send out a
cleaned up patch.  Maybe that will motivate me to send one.

More seriously, I was on PTO at the end of last week and this fell off
my plate.  I will clean it up and test it tomorrow, with the aim of
actually sending something tomorrow afternoon.

josh

[PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

The dmesg_restrict sysctl currently covers the syslog method for access
dmesg, however /dev/kmsg isn't covered by the same protections.  Most
people haven't noticed because util-linux dmesg(1) defaults to using the
syslog method for access in older versions.  With util-linux dmesg(1)
defaults to reading directly from /dev/kmsg.

Fix this by reworking all of the access methods to use the
check_syslog_permissions function and adding checks to devkmsg_open and
devkmsg_read.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

Reported-by: Christian Kujau <lists@nerdbynature.de>
CC: stable@vger.kernel.org
Signed-off-by: Eric Paris <eparis@redhat.com>
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---

 v2: Rework patch based on code from Eric Paris, add check in devkmsg_read as
     suggested by Kees Cook.

 kernel/printk.c | 91 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 47 insertions(+), 44 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index abbdd9e..5541095 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -368,6 +368,46 @@ static void log_store(int facility, int level,
 	log_next_seq++;
 }
 
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+int dmesg_restrict = 1;
+#else
+int dmesg_restrict;
+#endif
+
+static int syslog_action_restricted(int type)
+{
+	if (dmesg_restrict)
+		return 1;
+	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
+	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
+}
+
+static int check_syslog_permissions(int type, bool from_file)
+{
+	/*
+	 * 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)
+		goto ok;
+
+	if (syslog_action_restricted(type)) {
+		if (capable(CAP_SYSLOG))
+			goto ok;
+		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
+		if (capable(CAP_SYS_ADMIN)) {
+			printk_once(KERN_WARNING "%s (%d): "
+				 "Attempt to access syslog with CAP_SYS_ADMIN "
+				 "but no CAP_SYSLOG (deprecated).\n",
+				 current->comm, task_pid_nr(current));
+			goto ok;
+		}
+		return -EPERM;
+	}
+ok:
+	return security_syslog(type);
+}
+
 /* /dev/kmsg - userspace message inject/listen interface */
 struct devkmsg_user {
 	u64 seq;
@@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
 	char cont = '-';
 	size_t len;
 	ssize_t ret;
+	int err;
 
 	if (!user)
 		return -EBADF;
 
+	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
+		SYSLOG_FROM_FILE);
+	if (err)
+		return err;
+
 	ret = mutex_lock_interruptible(&user->lock);
 	if (ret)
 		return ret;
@@ -624,7 +670,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
 
-	err = security_syslog(SYSLOG_ACTION_READ_ALL);
+	err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);
 	if (err)
 		return err;
 
@@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level)
 }
 #endif
 
-#ifdef CONFIG_SECURITY_DMESG_RESTRICT
-int dmesg_restrict = 1;
-#else
-int dmesg_restrict;
-#endif
-
-static int syslog_action_restricted(int type)
-{
-	if (dmesg_restrict)
-		return 1;
-	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
-	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
-}
-
-static int check_syslog_permissions(int type, bool from_file)
-{
-	/*
-	 * 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)
-		return 0;
-
-	if (syslog_action_restricted(type)) {
-		if (capable(CAP_SYSLOG))
-			return 0;
-		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
-		if (capable(CAP_SYS_ADMIN)) {
-			printk_once(KERN_WARNING "%s (%d): "
-				 "Attempt to access syslog with CAP_SYS_ADMIN "
-				 "but no CAP_SYSLOG (deprecated).\n",
-				 current->comm, task_pid_nr(current));
-			return 0;
-		}
-		return -EPERM;
-	}
-	return 0;
-}
-
 #if defined(CONFIG_PRINTK_TIME)
 static bool printk_time = 1;
 #else
@@ -1131,10 +1138,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	if (error)
 		goto out;
 
-	error = security_syslog(type);
-	if (error)
-		return error;
-
 	switch (type) {
 	case SYSLOG_ACTION_CLOSE:	/* Close log */
 		break;
-- 
1.8.1.4

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> The dmesg_restrict sysctl currently covers the syslog method for access
> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> people haven't noticed because util-linux dmesg(1) defaults to using the
> syslog method for access in older versions.  With util-linux dmesg(1)
> defaults to reading directly from /dev/kmsg.
>
> Fix this by reworking all of the access methods to use the
> check_syslog_permissions function and adding checks to devkmsg_open and
> devkmsg_read.
>
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> CC: stable@vger.kernel.org
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>

Thanks!

Acked-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook
Chrome OS Security

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Tue, Apr 09, 2013 at 11:48:20AM -0400, Josh Boyer wrote:
> The dmesg_restrict sysctl currently covers the syslog method for access
> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> people haven't noticed because util-linux dmesg(1) defaults to using the
> syslog method for access in older versions.  With util-linux dmesg(1)
> defaults to reading directly from /dev/kmsg.
> 
> Fix this by reworking all of the access methods to use the
> check_syslog_permissions function and adding checks to devkmsg_open and
> devkmsg_read.
> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

So this does fix that bug.  But then it introduced this one:

https://bugzilla.redhat.com/show_bug.cgi?id=952655

Basically, dmesg(1) now always falls back to using the syslog interface
instead of /dev/kmsg because nothing is granting CAP_SYSLOG for normal
users.  That was somewhat intentional based on the feedback from Kees
and Eric, but it does present a problem.

If we want to keep the existing open behavior for /dev/kmsg, and still
honor dmesg_restrict, we basically need it to fail in devkmsg_read.
With the current functions we have, that won't work so we'll either need
to hack that up or just have devkmsg_read call syslog_action_restricted
instead.

josh

> Reported-by: Christian Kujau <lists@nerdbynature.de>
> CC: stable@vger.kernel.org
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> ---
> 
>  v2: Rework patch based on code from Eric Paris, add check in devkmsg_read as
>      suggested by Kees Cook.
> 
>  kernel/printk.c | 91 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 47 insertions(+), 44 deletions(-)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index abbdd9e..5541095 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -368,6 +368,46 @@ static void log_store(int facility, int level,
>  	log_next_seq++;
>  }
>  
> +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> +int dmesg_restrict = 1;
> +#else
> +int dmesg_restrict;
> +#endif
> +
> +static int syslog_action_restricted(int type)
> +{
> +	if (dmesg_restrict)
> +		return 1;
> +	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> +	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> +}
> +
> +static int check_syslog_permissions(int type, bool from_file)
> +{
> +	/*
> +	 * 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)
> +		goto ok;
> +
> +	if (syslog_action_restricted(type)) {
> +		if (capable(CAP_SYSLOG))
> +			goto ok;
> +		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> +		if (capable(CAP_SYS_ADMIN)) {
> +			printk_once(KERN_WARNING "%s (%d): "
> +				 "Attempt to access syslog with CAP_SYS_ADMIN "
> +				 "but no CAP_SYSLOG (deprecated).\n",
> +				 current->comm, task_pid_nr(current));
> +			goto ok;
> +		}
> +		return -EPERM;
> +	}
> +ok:
> +	return security_syslog(type);
> +}
> +
>  /* /dev/kmsg - userspace message inject/listen interface */
>  struct devkmsg_user {
>  	u64 seq;
> @@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>  	char cont = '-';
>  	size_t len;
>  	ssize_t ret;
> +	int err;
>  
>  	if (!user)
>  		return -EBADF;
>  
> +	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +		SYSLOG_FROM_FILE);
> +	if (err)
> +		return err;
> +
>  	ret = mutex_lock_interruptible(&user->lock);
>  	if (ret)
>  		return ret;
> @@ -624,7 +670,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;
>  
> -	err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +	err = check_syslog_permissions(SYSLOG_ACTION_OPEN, SYSLOG_FROM_FILE);
>  	if (err)
>  		return err;
>  
> @@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level)
>  }
>  #endif
>  
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> -
> -static int syslog_action_restricted(int type)
> -{
> -	if (dmesg_restrict)
> -		return 1;
> -	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> -	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> -}
> -
> -static int check_syslog_permissions(int type, bool from_file)
> -{
> -	/*
> -	 * 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)
> -		return 0;
> -
> -	if (syslog_action_restricted(type)) {
> -		if (capable(CAP_SYSLOG))
> -			return 0;
> -		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> -		if (capable(CAP_SYS_ADMIN)) {
> -			printk_once(KERN_WARNING "%s (%d): "
> -				 "Attempt to access syslog with CAP_SYS_ADMIN "
> -				 "but no CAP_SYSLOG (deprecated).\n",
> -				 current->comm, task_pid_nr(current));
> -			return 0;
> -		}
> -		return -EPERM;
> -	}
> -	return 0;
> -}
> -
>  #if defined(CONFIG_PRINTK_TIME)
>  static bool printk_time = 1;
>  #else
> @@ -1131,10 +1138,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	if (error)
>  		goto out;
>  
> -	error = security_syslog(type);
> -	if (error)
> -		return error;
> -
>  	switch (type) {
>  	case SYSLOG_ACTION_CLOSE:	/* Close log */
>  		break;
> -- 
> 1.8.1.4
> 

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kay Sievers]

On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <keescook@chromium.org> wrote:
> On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@redhat.com> wrote:
>> The dmesg_restrict sysctl currently covers the syslog method for access
>> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
>> people haven't noticed because util-linux dmesg(1) defaults to using the
>> syslog method for access in older versions.  With util-linux dmesg(1)
>> defaults to reading directly from /dev/kmsg.
>>
>> Fix this by reworking all of the access methods to use the
>> check_syslog_permissions function and adding checks to devkmsg_open and
>> devkmsg_read.
>>
>> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>>
>> Reported-by: Christian Kujau <lists@nerdbynature.de>
>> CC: stable@vger.kernel.org
>> Signed-off-by: Eric Paris <eparis@redhat.com>
>> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
>
> Thanks!
>
> Acked-by: Kees Cook <keescook@chromium.org>

If that's the version currently in Fedora, we just cannot do this.
   https://bugzilla.redhat.com/show_bug.cgi?id=952655

/dev/kmsg is supposed, and was added, to be a sane alternative to
syslog(). It is already used in dmesg(1) which is now broken with this
patch.

The access rules for /dev/kmsg should follow the access rules of
syslog(), and not be any stricter.

Thanks,
Kay

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Apr 24, 2013 at 07:44:33PM +0200, Kay Sievers wrote:
> On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <keescook@chromium.org> wrote:
> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> >> The dmesg_restrict sysctl currently covers the syslog method for access
> >> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> >> people haven't noticed because util-linux dmesg(1) defaults to using the
> >> syslog method for access in older versions.  With util-linux dmesg(1)
> >> defaults to reading directly from /dev/kmsg.
> >>
> >> Fix this by reworking all of the access methods to use the
> >> check_syslog_permissions function and adding checks to devkmsg_open and
> >> devkmsg_read.
> >>
> >> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> >>
> >> Reported-by: Christian Kujau <lists@nerdbynature.de>
> >> CC: stable@vger.kernel.org
> >> Signed-off-by: Eric Paris <eparis@redhat.com>
> >> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> >
> > Thanks!
> >
> > Acked-by: Kees Cook <keescook@chromium.org>
> 
> If that's the version currently in Fedora, we just cannot do this.
>    https://bugzilla.redhat.com/show_bug.cgi?id=952655
> 
> /dev/kmsg is supposed, and was added, to be a sane alternative to
> syslog(). It is already used in dmesg(1) which is now broken with this
> patch.
> 
> The access rules for /dev/kmsg should follow the access rules of
> syslog(), and not be any stricter.

I haven't tested it yet, but I think something like this should work
while still honoring dmesg_restrict.  I'll test it out while the rest
of you debate things.

josh

From: Josh Boyer <jwboyer@redhat.com>
Date: Tue, 9 Apr 2013 11:08:13 -0400
Subject: [PATCH v3] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

The dmesg_restrict sysctl currently covers the syslog method for access
dmesg, however /dev/kmsg isn't covered by the same protections.  Most
people haven't noticed because util-linux dmesg(1) defaults to using the
syslog method for access in older versions.  With util-linux dmesg(1)
defaults to reading directly from /dev/kmsg.

Fix this by reworking all of the access methods to use the
check_syslog_permissions function and adding checks to devkmsg_open and
devkmsg_read.

This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192

Reported-by: Christian Kujau <lists@nerdbynature.de>
CC: stable@vger.kernel.org
Signed-off-by: Eric Paris <eparis@redhat.com>
Signed-off-by: Josh Boyer <jwboyer@redhat.com>
---
 v3: Allow devkmsg_open to work without CAP_SYSLOG, but still make
     devkmsg_read honor dmesg_restrict

 kernel/printk.c | 91 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 47 insertions(+), 44 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index abbdd9e..2d7be05 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -368,6 +368,46 @@ static void log_store(int facility, int level,
 	log_next_seq++;
 }
 
+#ifdef CONFIG_SECURITY_DMESG_RESTRICT
+int dmesg_restrict = 1;
+#else
+int dmesg_restrict;
+#endif
+
+static int syslog_action_restricted(int type)
+{
+	if (dmesg_restrict)
+		return 1;
+	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
+	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
+}
+
+static int check_syslog_permissions(int type, bool from_file)
+{
+	/*
+	 * 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)
+		goto ok;
+
+	if (syslog_action_restricted(type)) {
+		if (capable(CAP_SYSLOG))
+			goto ok;
+		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
+		if (capable(CAP_SYS_ADMIN)) {
+			printk_once(KERN_WARNING "%s (%d): "
+				 "Attempt to access syslog with CAP_SYS_ADMIN "
+				 "but no CAP_SYSLOG (deprecated).\n",
+				 current->comm, task_pid_nr(current));
+			goto ok;
+		}
+		return -EPERM;
+	}
+ok:
+	return security_syslog(type);
+}
+
 /* /dev/kmsg - userspace message inject/listen interface */
 struct devkmsg_user {
 	u64 seq;
@@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
 	char cont = '-';
 	size_t len;
 	ssize_t ret;
+	int err;
 
 	if (!user)
 		return -EBADF;
 
+	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
+		SYSLOG_FROM_CALL);
+	if (err)
+		return err;
+
 	ret = mutex_lock_interruptible(&user->lock);
 	if (ret)
 		return ret;
@@ -624,7 +670,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
 	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
 		return 0;
 
-	err = security_syslog(SYSLOG_ACTION_READ_ALL);
+	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE);
 	if (err)
 		return err;
 
@@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level)
 }
 #endif
 
-#ifdef CONFIG_SECURITY_DMESG_RESTRICT
-int dmesg_restrict = 1;
-#else
-int dmesg_restrict;
-#endif
-
-static int syslog_action_restricted(int type)
-{
-	if (dmesg_restrict)
-		return 1;
-	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
-	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
-}
-
-static int check_syslog_permissions(int type, bool from_file)
-{
-	/*
-	 * 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)
-		return 0;
-
-	if (syslog_action_restricted(type)) {
-		if (capable(CAP_SYSLOG))
-			return 0;
-		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
-		if (capable(CAP_SYS_ADMIN)) {
-			printk_once(KERN_WARNING "%s (%d): "
-				 "Attempt to access syslog with CAP_SYS_ADMIN "
-				 "but no CAP_SYSLOG (deprecated).\n",
-				 current->comm, task_pid_nr(current));
-			return 0;
-		}
-		return -EPERM;
-	}
-	return 0;
-}
-
 #if defined(CONFIG_PRINTK_TIME)
 static bool printk_time = 1;
 #else
@@ -1131,10 +1138,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
 	if (error)
 		goto out;
 
-	error = security_syslog(type);
-	if (error)
-		return error;
-
 	switch (type) {
 	case SYSLOG_ACTION_CLOSE:	/* Close log */
 		break;
-- 
1.8.1.4

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Apr 24, 2013 at 01:58:35PM -0400, Josh Boyer wrote:
> On Wed, Apr 24, 2013 at 07:44:33PM +0200, Kay Sievers wrote:
> > On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <keescook@chromium.org> wrote:
> > > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> > >> The dmesg_restrict sysctl currently covers the syslog method for access
> > >> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> > >> people haven't noticed because util-linux dmesg(1) defaults to using the
> > >> syslog method for access in older versions.  With util-linux dmesg(1)
> > >> defaults to reading directly from /dev/kmsg.
> > >>
> > >> Fix this by reworking all of the access methods to use the
> > >> check_syslog_permissions function and adding checks to devkmsg_open and
> > >> devkmsg_read.
> > >>
> > >> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> > >>
> > >> Reported-by: Christian Kujau <lists@nerdbynature.de>
> > >> CC: stable@vger.kernel.org
> > >> Signed-off-by: Eric Paris <eparis@redhat.com>
> > >> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> > >
> > > Thanks!
> > >
> > > Acked-by: Kees Cook <keescook@chromium.org>
> > 
> > If that's the version currently in Fedora, we just cannot do this.
> >    https://bugzilla.redhat.com/show_bug.cgi?id=952655
> > 
> > /dev/kmsg is supposed, and was added, to be a sane alternative to
> > syslog(). It is already used in dmesg(1) which is now broken with this
> > patch.
> > 
> > The access rules for /dev/kmsg should follow the access rules of
> > syslog(), and not be any stricter.
> 
> I haven't tested it yet, but I think something like this should work
> while still honoring dmesg_restrict.  I'll test it out while the rest
> of you debate things.

Yeah, that seems to work.  So, comments or Reviewed-by/Acked-by on it
would be welcome.

josh

> From: Josh Boyer <jwboyer@redhat.com>
> Date: Tue, 9 Apr 2013 11:08:13 -0400
> Subject: [PATCH v3] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
> 
> The dmesg_restrict sysctl currently covers the syslog method for access
> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> people haven't noticed because util-linux dmesg(1) defaults to using the
> syslog method for access in older versions.  With util-linux dmesg(1)
> defaults to reading directly from /dev/kmsg.
> 
> Fix this by reworking all of the access methods to use the
> check_syslog_permissions function and adding checks to devkmsg_open and
> devkmsg_read.
> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> 
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> CC: stable@vger.kernel.org
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> ---
>  v3: Allow devkmsg_open to work without CAP_SYSLOG, but still make
>      devkmsg_read honor dmesg_restrict
> 
>  kernel/printk.c | 91 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 47 insertions(+), 44 deletions(-)
> 
> diff --git a/kernel/printk.c b/kernel/printk.c
> index abbdd9e..2d7be05 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -368,6 +368,46 @@ static void log_store(int facility, int level,
>  	log_next_seq++;
>  }
>  
> +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> +int dmesg_restrict = 1;
> +#else
> +int dmesg_restrict;
> +#endif
> +
> +static int syslog_action_restricted(int type)
> +{
> +	if (dmesg_restrict)
> +		return 1;
> +	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> +	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> +}
> +
> +static int check_syslog_permissions(int type, bool from_file)
> +{
> +	/*
> +	 * 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)
> +		goto ok;
> +
> +	if (syslog_action_restricted(type)) {
> +		if (capable(CAP_SYSLOG))
> +			goto ok;
> +		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> +		if (capable(CAP_SYS_ADMIN)) {
> +			printk_once(KERN_WARNING "%s (%d): "
> +				 "Attempt to access syslog with CAP_SYS_ADMIN "
> +				 "but no CAP_SYSLOG (deprecated).\n",
> +				 current->comm, task_pid_nr(current));
> +			goto ok;
> +		}
> +		return -EPERM;
> +	}
> +ok:
> +	return security_syslog(type);
> +}
> +
>  /* /dev/kmsg - userspace message inject/listen interface */
>  struct devkmsg_user {
>  	u64 seq;
> @@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>  	char cont = '-';
>  	size_t len;
>  	ssize_t ret;
> +	int err;
>  
>  	if (!user)
>  		return -EBADF;
>  
> +	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +		SYSLOG_FROM_CALL);
> +	if (err)
> +		return err;
> +
>  	ret = mutex_lock_interruptible(&user->lock);
>  	if (ret)
>  		return ret;
> @@ -624,7 +670,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>  	if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>  		return 0;
>  
> -	err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +	err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE);
>  	if (err)
>  		return err;
>  
> @@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level)
>  }
>  #endif
>  
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> -
> -static int syslog_action_restricted(int type)
> -{
> -	if (dmesg_restrict)
> -		return 1;
> -	/* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> -	return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> -}
> -
> -static int check_syslog_permissions(int type, bool from_file)
> -{
> -	/*
> -	 * 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)
> -		return 0;
> -
> -	if (syslog_action_restricted(type)) {
> -		if (capable(CAP_SYSLOG))
> -			return 0;
> -		/* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> -		if (capable(CAP_SYS_ADMIN)) {
> -			printk_once(KERN_WARNING "%s (%d): "
> -				 "Attempt to access syslog with CAP_SYS_ADMIN "
> -				 "but no CAP_SYSLOG (deprecated).\n",
> -				 current->comm, task_pid_nr(current));
> -			return 0;
> -		}
> -		return -EPERM;
> -	}
> -	return 0;
> -}
> -
>  #if defined(CONFIG_PRINTK_TIME)
>  static bool printk_time = 1;
>  #else
> @@ -1131,10 +1138,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>  	if (error)
>  		goto out;
>  
> -	error = security_syslog(type);
> -	if (error)
> -		return error;
> -
>  	switch (type) {
>  	case SYSLOG_ACTION_CLOSE:	/* Close log */
>  		break;
> -- 
> 1.8.1.4
> 

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, Apr 24, 2013 at 10:58 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Wed, Apr 24, 2013 at 07:44:33PM +0200, Kay Sievers wrote:
>> On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <keescook@chromium.org> wrote:
>> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@redhat.com> wrote:
>> >> The dmesg_restrict sysctl currently covers the syslog method for access
>> >> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
>> >> people haven't noticed because util-linux dmesg(1) defaults to using the
>> >> syslog method for access in older versions.  With util-linux dmesg(1)
>> >> defaults to reading directly from /dev/kmsg.
>> >>
>> >> Fix this by reworking all of the access methods to use the
>> >> check_syslog_permissions function and adding checks to devkmsg_open and
>> >> devkmsg_read.
>> >>
>> >> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>> >>
>> >> Reported-by: Christian Kujau <lists@nerdbynature.de>
>> >> CC: stable@vger.kernel.org
>> >> Signed-off-by: Eric Paris <eparis@redhat.com>
>> >> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
>> >
>> > Thanks!
>> >
>> > Acked-by: Kees Cook <keescook@chromium.org>
>>
>> If that's the version currently in Fedora, we just cannot do this.
>>    https://bugzilla.redhat.com/show_bug.cgi?id=952655
>>
>> /dev/kmsg is supposed, and was added, to be a sane alternative to
>> syslog(). It is already used in dmesg(1) which is now broken with this
>> patch.
>>
>> The access rules for /dev/kmsg should follow the access rules of
>> syslog(), and not be any stricter.
>
> I haven't tested it yet, but I think something like this should work
> while still honoring dmesg_restrict.  I'll test it out while the rest
> of you debate things.
>
> josh
>
> From: Josh Boyer <jwboyer@redhat.com>
> Date: Tue, 9 Apr 2013 11:08:13 -0400
> Subject: [PATCH v3] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
>
> The dmesg_restrict sysctl currently covers the syslog method for access
> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> people haven't noticed because util-linux dmesg(1) defaults to using the
> syslog method for access in older versions.  With util-linux dmesg(1)
> defaults to reading directly from /dev/kmsg.
>
> Fix this by reworking all of the access methods to use the
> check_syslog_permissions function and adding checks to devkmsg_open and
> devkmsg_read.
>
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>
> Reported-by: Christian Kujau <lists@nerdbynature.de>
> CC: stable@vger.kernel.org
> Signed-off-by: Eric Paris <eparis@redhat.com>
> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> ---
>  v3: Allow devkmsg_open to work without CAP_SYSLOG, but still make
>      devkmsg_read honor dmesg_restrict
>
>  kernel/printk.c | 91 +++++++++++++++++++++++++++++----------------------------
>  1 file changed, 47 insertions(+), 44 deletions(-)
>
> diff --git a/kernel/printk.c b/kernel/printk.c
> index abbdd9e..2d7be05 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -368,6 +368,46 @@ static void log_store(int facility, int level,
>         log_next_seq++;
>  }
>
> +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> +int dmesg_restrict = 1;
> +#else
> +int dmesg_restrict;
> +#endif
> +
> +static int syslog_action_restricted(int type)
> +{
> +       if (dmesg_restrict)
> +               return 1;
> +       /* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> +       return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> +}
> +
> +static int check_syslog_permissions(int type, bool from_file)
> +{
> +       /*
> +        * 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)
> +               goto ok;
> +
> +       if (syslog_action_restricted(type)) {
> +               if (capable(CAP_SYSLOG))
> +                       goto ok;
> +               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> +               if (capable(CAP_SYS_ADMIN)) {
> +                       printk_once(KERN_WARNING "%s (%d): "
> +                                "Attempt to access syslog with CAP_SYS_ADMIN "
> +                                "but no CAP_SYSLOG (deprecated).\n",
> +                                current->comm, task_pid_nr(current));
> +                       goto ok;
> +               }
> +               return -EPERM;
> +       }
> +ok:
> +       return security_syslog(type);
> +}
> +
>  /* /dev/kmsg - userspace message inject/listen interface */
>  struct devkmsg_user {
>         u64 seq;
> @@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>         char cont = '-';
>         size_t len;
>         ssize_t ret;
> +       int err;
>
>         if (!user)
>                 return -EBADF;
>
> +       err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> +               SYSLOG_FROM_CALL);
> +       if (err)
> +               return err;
> +
>         ret = mutex_lock_interruptible(&user->lock);
>         if (ret)
>                 return ret;
> @@ -624,7 +670,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>         if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>                 return 0;
>
> -       err = security_syslog(SYSLOG_ACTION_READ_ALL);
> +       err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE);
>         if (err)
>                 return err;
>
> @@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level)
>  }
>  #endif
>
> -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> -int dmesg_restrict = 1;
> -#else
> -int dmesg_restrict;
> -#endif
> -
> -static int syslog_action_restricted(int type)
> -{
> -       if (dmesg_restrict)
> -               return 1;
> -       /* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> -       return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> -}
> -
> -static int check_syslog_permissions(int type, bool from_file)
> -{
> -       /*
> -        * 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)
> -               return 0;
> -
> -       if (syslog_action_restricted(type)) {
> -               if (capable(CAP_SYSLOG))
> -                       return 0;
> -               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> -               if (capable(CAP_SYS_ADMIN)) {
> -                       printk_once(KERN_WARNING "%s (%d): "
> -                                "Attempt to access syslog with CAP_SYS_ADMIN "
> -                                "but no CAP_SYSLOG (deprecated).\n",
> -                                current->comm, task_pid_nr(current));
> -                       return 0;
> -               }
> -               return -EPERM;
> -       }
> -       return 0;
> -}
> -
>  #if defined(CONFIG_PRINTK_TIME)
>  static bool printk_time = 1;
>  #else
> @@ -1131,10 +1138,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>         if (error)
>                 goto out;
>
> -       error = security_syslog(type);
> -       if (error)
> -               return error;
> -
>         switch (type) {
>         case SYSLOG_ACTION_CLOSE:       /* Close log */
>                 break;

So, the problem here is the expectation of privileges. The /proc/kmsg
usage pattern was:

open /proc/kmsg with CAP_SYSLOG
drop CAP_SYSLOG
read /proc/kmsg forever

If we change the FILE vs CALL and OPEN vs READ stuff here, we're lying
to the API about what's happening. If we use this patch, then we can't
use /dev/kmsg in the same way (i.e. running without privileges).

That said, I much prefer doing the privilege test at read time since
that means passing a file descriptor to another process doesn't mean
the new process can just continue reading. If we're going to be
defining the new behavior for /dev/kmsg, then I think we should
explicitly drop the fall-back to CAP_SYS_ADMIN. Perhaps introduce a
"legacy_privs" option to check_syslog_permissions() and leave it set
for do_syslog and cleared for devkmsg_*. This means we can run a
/dev/kmsg reader with _only_ CAP_SYSLOG. (A while back, the original
intent that reworked the permissions here was to avoid needing
CAP_SYS_ADMIN.)

And then maybe we need to rename the from_file to be from_proc (and
similarly SYSLOG_FROM_PROC) too?

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Apr 24, 2013 at 01:35:17PM -0700, Kees Cook wrote:
> On Wed, Apr 24, 2013 at 10:58 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> > On Wed, Apr 24, 2013 at 07:44:33PM +0200, Kay Sievers wrote:
> >> On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <keescook@chromium.org> wrote:
> >> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@redhat.com> wrote:
> >> >> The dmesg_restrict sysctl currently covers the syslog method for access
> >> >> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> >> >> people haven't noticed because util-linux dmesg(1) defaults to using the
> >> >> syslog method for access in older versions.  With util-linux dmesg(1)
> >> >> defaults to reading directly from /dev/kmsg.
> >> >>
> >> >> Fix this by reworking all of the access methods to use the
> >> >> check_syslog_permissions function and adding checks to devkmsg_open and
> >> >> devkmsg_read.
> >> >>
> >> >> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> >> >>
> >> >> Reported-by: Christian Kujau <lists@nerdbynature.de>
> >> >> CC: stable@vger.kernel.org
> >> >> Signed-off-by: Eric Paris <eparis@redhat.com>
> >> >> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> >> >
> >> > Thanks!
> >> >
> >> > Acked-by: Kees Cook <keescook@chromium.org>
> >>
> >> If that's the version currently in Fedora, we just cannot do this.
> >>    https://bugzilla.redhat.com/show_bug.cgi?id=952655
> >>
> >> /dev/kmsg is supposed, and was added, to be a sane alternative to
> >> syslog(). It is already used in dmesg(1) which is now broken with this
> >> patch.
> >>
> >> The access rules for /dev/kmsg should follow the access rules of
> >> syslog(), and not be any stricter.
> >
> > I haven't tested it yet, but I think something like this should work
> > while still honoring dmesg_restrict.  I'll test it out while the rest
> > of you debate things.
> >
> > josh
> >
> > From: Josh Boyer <jwboyer@redhat.com>
> > Date: Tue, 9 Apr 2013 11:08:13 -0400
> > Subject: [PATCH v3] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
> >
> > The dmesg_restrict sysctl currently covers the syslog method for access
> > dmesg, however /dev/kmsg isn't covered by the same protections.  Most
> > people haven't noticed because util-linux dmesg(1) defaults to using the
> > syslog method for access in older versions.  With util-linux dmesg(1)
> > defaults to reading directly from /dev/kmsg.
> >
> > Fix this by reworking all of the access methods to use the
> > check_syslog_permissions function and adding checks to devkmsg_open and
> > devkmsg_read.
> >
> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
> >
> > Reported-by: Christian Kujau <lists@nerdbynature.de>
> > CC: stable@vger.kernel.org
> > Signed-off-by: Eric Paris <eparis@redhat.com>
> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
> > ---
> >  v3: Allow devkmsg_open to work without CAP_SYSLOG, but still make
> >      devkmsg_read honor dmesg_restrict
> >
> >  kernel/printk.c | 91 +++++++++++++++++++++++++++++----------------------------
> >  1 file changed, 47 insertions(+), 44 deletions(-)
> >
> > diff --git a/kernel/printk.c b/kernel/printk.c
> > index abbdd9e..2d7be05 100644
> > --- a/kernel/printk.c
> > +++ b/kernel/printk.c
> > @@ -368,6 +368,46 @@ static void log_store(int facility, int level,
> >         log_next_seq++;
> >  }
> >
> > +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> > +int dmesg_restrict = 1;
> > +#else
> > +int dmesg_restrict;
> > +#endif
> > +
> > +static int syslog_action_restricted(int type)
> > +{
> > +       if (dmesg_restrict)
> > +               return 1;
> > +       /* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> > +       return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> > +}
> > +
> > +static int check_syslog_permissions(int type, bool from_file)
> > +{
> > +       /*
> > +        * 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)
> > +               goto ok;
> > +
> > +       if (syslog_action_restricted(type)) {
> > +               if (capable(CAP_SYSLOG))
> > +                       goto ok;
> > +               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> > +               if (capable(CAP_SYS_ADMIN)) {
> > +                       printk_once(KERN_WARNING "%s (%d): "
> > +                                "Attempt to access syslog with CAP_SYS_ADMIN "
> > +                                "but no CAP_SYSLOG (deprecated).\n",
> > +                                current->comm, task_pid_nr(current));
> > +                       goto ok;
> > +               }
> > +               return -EPERM;
> > +       }
> > +ok:
> > +       return security_syslog(type);
> > +}
> > +
> >  /* /dev/kmsg - userspace message inject/listen interface */
> >  struct devkmsg_user {
> >         u64 seq;
> > @@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
> >         char cont = '-';
> >         size_t len;
> >         ssize_t ret;
> > +       int err;
> >
> >         if (!user)
> >                 return -EBADF;
> >
> > +       err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
> > +               SYSLOG_FROM_CALL);
> > +       if (err)
> > +               return err;
> > +
> >         ret = mutex_lock_interruptible(&user->lock);
> >         if (ret)
> >                 return ret;
> > @@ -624,7 +670,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
> >         if ((file->f_flags & O_ACCMODE) == O_WRONLY)
> >                 return 0;
> >
> > -       err = security_syslog(SYSLOG_ACTION_READ_ALL);
> > +       err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE);
> >         if (err)
> >                 return err;
> >
> > @@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level)
> >  }
> >  #endif
> >
> > -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
> > -int dmesg_restrict = 1;
> > -#else
> > -int dmesg_restrict;
> > -#endif
> > -
> > -static int syslog_action_restricted(int type)
> > -{
> > -       if (dmesg_restrict)
> > -               return 1;
> > -       /* Unless restricted, we allow "read all" and "get buffer size" for everybody */
> > -       return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
> > -}
> > -
> > -static int check_syslog_permissions(int type, bool from_file)
> > -{
> > -       /*
> > -        * 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)
> > -               return 0;
> > -
> > -       if (syslog_action_restricted(type)) {
> > -               if (capable(CAP_SYSLOG))
> > -                       return 0;
> > -               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
> > -               if (capable(CAP_SYS_ADMIN)) {
> > -                       printk_once(KERN_WARNING "%s (%d): "
> > -                                "Attempt to access syslog with CAP_SYS_ADMIN "
> > -                                "but no CAP_SYSLOG (deprecated).\n",
> > -                                current->comm, task_pid_nr(current));
> > -                       return 0;
> > -               }
> > -               return -EPERM;
> > -       }
> > -       return 0;
> > -}
> > -
> >  #if defined(CONFIG_PRINTK_TIME)
> >  static bool printk_time = 1;
> >  #else
> > @@ -1131,10 +1138,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
> >         if (error)
> >                 goto out;
> >
> > -       error = security_syslog(type);
> > -       if (error)
> > -               return error;
> > -
> >         switch (type) {
> >         case SYSLOG_ACTION_CLOSE:       /* Close log */
> >                 break;
> 
> So, the problem here is the expectation of privileges. The /proc/kmsg
> usage pattern was:
> 
> open /proc/kmsg with CAP_SYSLOG
> drop CAP_SYSLOG
> read /proc/kmsg forever

This doesn't change the /proc interface at all.

> If we change the FILE vs CALL and OPEN vs READ stuff here, we're lying
> to the API about what's happening. If we use this patch, then we can't
> use /dev/kmsg in the same way (i.e. running without privileges).

Uh... Yes we can.  I tested it as a normal user.  It works just fine
running without privs and without CAP_SYSLOG, like it did before there
was a patch at all.  It also honors dmesg_restrict in devkmsg_read.
I'm confused why you think this doesn't work?


> That said, I much prefer doing the privilege test at read time since
> that means passing a file descriptor to another process doesn't mean
> the new process can just continue reading. If we're going to be
> defining the new behavior for /dev/kmsg, then I think we should
> explicitly drop the fall-back to CAP_SYS_ADMIN. Perhaps introduce a

I think Karel and Kay's entire point was /dev/kmsg shouldn't be getting
new behavior.  Aside from honoring dmesg_restrict, they see any behavior
change as a regression.

> "legacy_privs" option to check_syslog_permissions() and leave it set
> for do_syslog and cleared for devkmsg_*. This means we can run a
> /dev/kmsg reader with _only_ CAP_SYSLOG. (A while back, the original
> intent that reworked the permissions here was to avoid needing
> CAP_SYS_ADMIN.)

Right, but /dev/kmsg didn't require CAP_SYSLOG before any version of
this patch existed.  That's the bug Karel and Kay are reporting.  As
far as I can see, that should be just fine for reading unless
dmesg_restrict is set.

> And then maybe we need to rename the from_file to be from_proc (and
> similarly SYSLOG_FROM_PROC) too?

Well, that can be done regardless of what falls out from above and would
probably make things clearer.

josh

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Linus Torvalds]

On Wed, Apr 24, 2013 at 1:35 PM, Kees Cook <keescook@chromium.org> wrote:
>
> That said, I much prefer doing the privilege test at read time since
> that means passing a file descriptor to another process doesn't mean
> the new process can just continue reading.

Bullshit.

That's exactly the wrong kind of thinking. If you had privileges to
open something, and you pass it off, it's *your* choice.

In contrast, the "anybody can open, but some people can read/write"
has several times resulted in real security issues. Notably the whole
"open something, then fool a suid program to write its error message
to it".

This whole discussion has been f*cking moronic. The "security"
arguments have been utter shite with clearly no thinking behind it,
the feature is total crap (people need dmesg to do basic bug
reporting), and I'm seriously considering just getting rid of this
idiotic dmesg_restrict thing entirely. Your comment is the very
epitome of bad security thinking.

                Linus

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, Apr 24, 2013 at 2:21 PM, Josh Boyer <jwboyer@redhat.com> wrote:
> On Wed, Apr 24, 2013 at 01:35:17PM -0700, Kees Cook wrote:
>> On Wed, Apr 24, 2013 at 10:58 AM, Josh Boyer <jwboyer@redhat.com> wrote:
>> > On Wed, Apr 24, 2013 at 07:44:33PM +0200, Kay Sievers wrote:
>> >> On Tue, Apr 9, 2013 at 6:33 PM, Kees Cook <keescook@chromium.org> wrote:
>> >> > On Tue, Apr 9, 2013 at 8:48 AM, Josh Boyer <jwboyer@redhat.com> wrote:
>> >> >> The dmesg_restrict sysctl currently covers the syslog method for access
>> >> >> dmesg, however /dev/kmsg isn't covered by the same protections.  Most
>> >> >> people haven't noticed because util-linux dmesg(1) defaults to using the
>> >> >> syslog method for access in older versions.  With util-linux dmesg(1)
>> >> >> defaults to reading directly from /dev/kmsg.
>> >> >>
>> >> >> Fix this by reworking all of the access methods to use the
>> >> >> check_syslog_permissions function and adding checks to devkmsg_open and
>> >> >> devkmsg_read.
>> >> >>
>> >> >> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>> >> >>
>> >> >> Reported-by: Christian Kujau <lists@nerdbynature.de>
>> >> >> CC: stable@vger.kernel.org
>> >> >> Signed-off-by: Eric Paris <eparis@redhat.com>
>> >> >> Signed-off-by: Josh Boyer <jwboyer@redhat.com>
>> >> >
>> >> > Thanks!
>> >> >
>> >> > Acked-by: Kees Cook <keescook@chromium.org>
>> >>
>> >> If that's the version currently in Fedora, we just cannot do this.
>> >>    https://bugzilla.redhat.com/show_bug.cgi?id=952655
>> >>
>> >> /dev/kmsg is supposed, and was added, to be a sane alternative to
>> >> syslog(). It is already used in dmesg(1) which is now broken with this
>> >> patch.
>> >>
>> >> The access rules for /dev/kmsg should follow the access rules of
>> >> syslog(), and not be any stricter.
>> >
>> > I haven't tested it yet, but I think something like this should work
>> > while still honoring dmesg_restrict.  I'll test it out while the rest
>> > of you debate things.
>> >
>> > josh
>> >
>> > From: Josh Boyer <jwboyer@redhat.com>
>> > Date: Tue, 9 Apr 2013 11:08:13 -0400
>> > Subject: [PATCH v3] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg
>> >
>> > The dmesg_restrict sysctl currently covers the syslog method for access
>> > dmesg, however /dev/kmsg isn't covered by the same protections.  Most
>> > people haven't noticed because util-linux dmesg(1) defaults to using the
>> > syslog method for access in older versions.  With util-linux dmesg(1)
>> > defaults to reading directly from /dev/kmsg.
>> >
>> > Fix this by reworking all of the access methods to use the
>> > check_syslog_permissions function and adding checks to devkmsg_open and
>> > devkmsg_read.
>> >
>> > This fixes https://bugzilla.redhat.com/show_bug.cgi?id=903192
>> >
>> > Reported-by: Christian Kujau <lists@nerdbynature.de>
>> > CC: stable@vger.kernel.org
>> > Signed-off-by: Eric Paris <eparis@redhat.com>
>> > Signed-off-by: Josh Boyer <jwboyer@redhat.com>
>> > ---
>> >  v3: Allow devkmsg_open to work without CAP_SYSLOG, but still make
>> >      devkmsg_read honor dmesg_restrict
>> >
>> >  kernel/printk.c | 91 +++++++++++++++++++++++++++++----------------------------
>> >  1 file changed, 47 insertions(+), 44 deletions(-)
>> >
>> > diff --git a/kernel/printk.c b/kernel/printk.c
>> > index abbdd9e..2d7be05 100644
>> > --- a/kernel/printk.c
>> > +++ b/kernel/printk.c
>> > @@ -368,6 +368,46 @@ static void log_store(int facility, int level,
>> >         log_next_seq++;
>> >  }
>> >
>> > +#ifdef CONFIG_SECURITY_DMESG_RESTRICT
>> > +int dmesg_restrict = 1;
>> > +#else
>> > +int dmesg_restrict;
>> > +#endif
>> > +
>> > +static int syslog_action_restricted(int type)
>> > +{
>> > +       if (dmesg_restrict)
>> > +               return 1;
>> > +       /* Unless restricted, we allow "read all" and "get buffer size" for everybody */
>> > +       return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
>> > +}
>> > +
>> > +static int check_syslog_permissions(int type, bool from_file)
>> > +{
>> > +       /*
>> > +        * 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)
>> > +               goto ok;
>> > +
>> > +       if (syslog_action_restricted(type)) {
>> > +               if (capable(CAP_SYSLOG))
>> > +                       goto ok;
>> > +               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>> > +               if (capable(CAP_SYS_ADMIN)) {
>> > +                       printk_once(KERN_WARNING "%s (%d): "
>> > +                                "Attempt to access syslog with CAP_SYS_ADMIN "
>> > +                                "but no CAP_SYSLOG (deprecated).\n",
>> > +                                current->comm, task_pid_nr(current));
>> > +                       goto ok;
>> > +               }
>> > +               return -EPERM;
>> > +       }
>> > +ok:
>> > +       return security_syslog(type);
>> > +}
>> > +
>> >  /* /dev/kmsg - userspace message inject/listen interface */
>> >  struct devkmsg_user {
>> >         u64 seq;
>> > @@ -443,10 +483,16 @@ static ssize_t devkmsg_read(struct file *file, char __user *buf,
>> >         char cont = '-';
>> >         size_t len;
>> >         ssize_t ret;
>> > +       int err;
>> >
>> >         if (!user)
>> >                 return -EBADF;
>> >
>> > +       err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL,
>> > +               SYSLOG_FROM_CALL);
>> > +       if (err)
>> > +               return err;
>> > +
>> >         ret = mutex_lock_interruptible(&user->lock);
>> >         if (ret)
>> >                 return ret;
>> > @@ -624,7 +670,7 @@ static int devkmsg_open(struct inode *inode, struct file *file)
>> >         if ((file->f_flags & O_ACCMODE) == O_WRONLY)
>> >                 return 0;
>> >
>> > -       err = security_syslog(SYSLOG_ACTION_READ_ALL);
>> > +       err = check_syslog_permissions(SYSLOG_ACTION_READ_ALL, SYSLOG_FROM_FILE);
>> >         if (err)
>> >                 return err;
>> >
>> > @@ -817,45 +863,6 @@ static inline void boot_delay_msec(int level)
>> >  }
>> >  #endif
>> >
>> > -#ifdef CONFIG_SECURITY_DMESG_RESTRICT
>> > -int dmesg_restrict = 1;
>> > -#else
>> > -int dmesg_restrict;
>> > -#endif
>> > -
>> > -static int syslog_action_restricted(int type)
>> > -{
>> > -       if (dmesg_restrict)
>> > -               return 1;
>> > -       /* Unless restricted, we allow "read all" and "get buffer size" for everybody */
>> > -       return type != SYSLOG_ACTION_READ_ALL && type != SYSLOG_ACTION_SIZE_BUFFER;
>> > -}
>> > -
>> > -static int check_syslog_permissions(int type, bool from_file)
>> > -{
>> > -       /*
>> > -        * 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)
>> > -               return 0;
>> > -
>> > -       if (syslog_action_restricted(type)) {
>> > -               if (capable(CAP_SYSLOG))
>> > -                       return 0;
>> > -               /* For historical reasons, accept CAP_SYS_ADMIN too, with a warning */
>> > -               if (capable(CAP_SYS_ADMIN)) {
>> > -                       printk_once(KERN_WARNING "%s (%d): "
>> > -                                "Attempt to access syslog with CAP_SYS_ADMIN "
>> > -                                "but no CAP_SYSLOG (deprecated).\n",
>> > -                                current->comm, task_pid_nr(current));
>> > -                       return 0;
>> > -               }
>> > -               return -EPERM;
>> > -       }
>> > -       return 0;
>> > -}
>> > -
>> >  #if defined(CONFIG_PRINTK_TIME)
>> >  static bool printk_time = 1;
>> >  #else
>> > @@ -1131,10 +1138,6 @@ int do_syslog(int type, char __user *buf, int len, bool from_file)
>> >         if (error)
>> >                 goto out;
>> >
>> > -       error = security_syslog(type);
>> > -       if (error)
>> > -               return error;
>> > -
>> >         switch (type) {
>> >         case SYSLOG_ACTION_CLOSE:       /* Close log */
>> >                 break;
>>
>> So, the problem here is the expectation of privileges. The /proc/kmsg
>> usage pattern was:
>>
>> open /proc/kmsg with CAP_SYSLOG
>> drop CAP_SYSLOG
>> read /proc/kmsg forever
>
> This doesn't change the /proc interface at all.

Right, I meant that Kay's assertion that /proc/kmsg is "legacy" means
he expects syslog daemons to switch to using /dev/kmsg.

>> If we change the FILE vs CALL and OPEN vs READ stuff here, we're lying
>> to the API about what's happening. If we use this patch, then we can't
>> use /dev/kmsg in the same way (i.e. running without privileges).
>
> Uh... Yes we can.  I tested it as a normal user.  It works just fine
> running without privs and without CAP_SYSLOG, like it did before there
> was a patch at all.  It also honors dmesg_restrict in devkmsg_read.
> I'm confused why you think this doesn't work?

I don't think I was clear. There are two use-cases, as I see it:

- normal user running dmesg(1)
- system daemon pulling kernel syslog and putting into userspace (e.g.
/var/log/kern.log).

In the dmesg(1) case, we're fine. It was using syscalls, now it can
use /dev/kmsg since open isn't checked, just the read action. That's
all cool by me.

In the daemon case, it's nice to be able to drop privileges after
setting up resources. The past was open /proc/kmsg with CAP_SYS_ADMIN,
then drop CAP_SYS_ADMIN and keep reading. Then later CAP_SYS_LOG was
introduced. So if a daemon switched from /proc/kmsg to /dev/kmsg they
wouldn't be able to drop the capability. But, it's much saner to carry
CAP_SYS_LOG than CAP_SYS_ADMIN on a long-running daemon.

>> That said, I much prefer doing the privilege test at read time since
>> that means passing a file descriptor to another process doesn't mean
>> the new process can just continue reading. If we're going to be
>> defining the new behavior for /dev/kmsg, then I think we should
>> explicitly drop the fall-back to CAP_SYS_ADMIN. Perhaps introduce a
>
> I think Karel and Kay's entire point was /dev/kmsg shouldn't be getting
> new behavior.  Aside from honoring dmesg_restrict, they see any behavior
> change as a regression.

Is there an intention to use /dev/kmsg for the syslog management daemon?

>> "legacy_privs" option to check_syslog_permissions() and leave it set
>> for do_syslog and cleared for devkmsg_*. This means we can run a
>> /dev/kmsg reader with _only_ CAP_SYSLOG. (A while back, the original
>> intent that reworked the permissions here was to avoid needing
>> CAP_SYS_ADMIN.)
>
> Right, but /dev/kmsg didn't require CAP_SYSLOG before any version of
> this patch existed.  That's the bug Karel and Kay are reporting.  As
> far as I can see, that should be just fine for reading unless
> dmesg_restrict is set.

Right, your patch fixes things fine. I was pointing out how the
semantics for switching from /proc/kmsg to /dev/kmsg are different,
and why we might want to consider changing the requirements here in an
intentional and clear manner.

>> And then maybe we need to rename the from_file to be from_proc (and
>> similarly SYSLOG_FROM_PROC) too?
>
> Well, that can be done regardless of what falls out from above and would
> probably make things clearer.

Yeah, cool.

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kees Cook]

On Wed, Apr 24, 2013 at 2:30 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Wed, Apr 24, 2013 at 1:35 PM, Kees Cook <keescook@chromium.org> wrote:
>>
>> That said, I much prefer doing the privilege test at read time since
>> that means passing a file descriptor to another process doesn't mean
>> the new process can just continue reading.
>
> Bullshit.
>
> That's exactly the wrong kind of thinking. If you had privileges to
> open something, and you pass it off, it's *your* choice.

Yes, this is what I was pointing out originally. The semantics of
/proc/kmsg do exactly that: check at open time, which is much cleaner.

Solving the permissions checking delta between the syslog via syscall
and syslog via /proc/kmsg was the original intent of the code so that
capabilities could be dropped after open. And when /dev/kmsg came
along, it didn't follow either convention. I just want to see the
behavior standardized.

-Kees

--
Kees Cook
Chrome OS Security

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Apr 24, 2013 at 02:36:39PM -0700, Kees Cook wrote:
> >>
> >> So, the problem here is the expectation of privileges. The /proc/kmsg
> >> usage pattern was:
> >>
> >> open /proc/kmsg with CAP_SYSLOG
> >> drop CAP_SYSLOG
> >> read /proc/kmsg forever
> >
> > This doesn't change the /proc interface at all.
> 
> Right, I meant that Kay's assertion that /proc/kmsg is "legacy" means
> he expects syslog daemons to switch to using /dev/kmsg.
> 
> >> If we change the FILE vs CALL and OPEN vs READ stuff here, we're lying
> >> to the API about what's happening. If we use this patch, then we can't
> >> use /dev/kmsg in the same way (i.e. running without privileges).
> >
> > Uh... Yes we can.  I tested it as a normal user.  It works just fine
> > running without privs and without CAP_SYSLOG, like it did before there
> > was a patch at all.  It also honors dmesg_restrict in devkmsg_read.
> > I'm confused why you think this doesn't work?
> 
> I don't think I was clear. There are two use-cases, as I see it:
> 
> - normal user running dmesg(1)
> - system daemon pulling kernel syslog and putting into userspace (e.g.
> /var/log/kern.log).

Ah.

> In the dmesg(1) case, we're fine. It was using syscalls, now it can
> use /dev/kmsg since open isn't checked, just the read action. That's
> all cool by me.

OK.

> In the daemon case, it's nice to be able to drop privileges after
> setting up resources. The past was open /proc/kmsg with CAP_SYS_ADMIN,
> then drop CAP_SYS_ADMIN and keep reading. Then later CAP_SYS_LOG was
> introduced. So if a daemon switched from /proc/kmsg to /dev/kmsg they
> wouldn't be able to drop the capability. But, it's much saner to carry
> CAP_SYS_LOG than CAP_SYS_ADMIN on a long-running daemon.

I have no idea on this front.  I'll let Kay speak to that.  On my
currently running Fedora 18 system, I actually have systemd-journald
using /dev/kmsg, and rsyslog using /proc/kmsg.  Why I have both, I have
no friggin idea.
 
> >> That said, I much prefer doing the privilege test at read time since
> >> that means passing a file descriptor to another process doesn't mean
> >> the new process can just continue reading. If we're going to be
> >> defining the new behavior for /dev/kmsg, then I think we should
> >> explicitly drop the fall-back to CAP_SYS_ADMIN. Perhaps introduce a
> >
> > I think Karel and Kay's entire point was /dev/kmsg shouldn't be getting
> > new behavior.  Aside from honoring dmesg_restrict, they see any behavior
> > change as a regression.
> 
> Is there an intention to use /dev/kmsg for the syslog management daemon?

Maybe?  I mean, systemd-journald seems to be using it for something.
Kay?

josh

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Josh Boyer]

On Wed, Apr 24, 2013 at 02:30:53PM -0700, Linus Torvalds wrote:
> On Wed, Apr 24, 2013 at 1:35 PM, Kees Cook <keescook@chromium.org> wrote:
> >
> > That said, I much prefer doing the privilege test at read time since
> > that means passing a file descriptor to another process doesn't mean
> > the new process can just continue reading.
> 
> Bullshit.
> 
> That's exactly the wrong kind of thinking. If you had privileges to
> open something, and you pass it off, it's *your* choice.
> 
> In contrast, the "anybody can open, but some people can read/write"
> has several times resulted in real security issues. Notably the whole
> "open something, then fool a suid program to write its error message
> to it".
> 
> This whole discussion has been f*cking moronic. The "security"
> arguments have been utter shite with clearly no thinking behind it,
> the feature is total crap (people need dmesg to do basic bug
> reporting), and I'm seriously considering just getting rid of this
> idiotic dmesg_restrict thing entirely. Your comment is the very
> epitome of bad security thinking.

I was just trying to get the 3 interfaces all honoring the same thing.

Let this be a lesson to you all: I am the harbinger of security
features removal.  If you see me sending patches, run away or I might
accidentally cross the streams and make your feature undergo total
protonic reversal.

Now if only I could use this power for good, like somehow getting Linus
to remove capabilities entirely...

josh

Re: [PATCH v2] kmsg: Honor dmesg_restrict sysctl on /dev/kmsg

[Kay Sievers]

On Wed, Apr 24, 2013 at 11:51 PM, Josh Boyer <jwboyer@redhat.com> wrote:

>> In the daemon case, it's nice to be able to drop privileges after
>> setting up resources. The past was open /proc/kmsg with CAP_SYS_ADMIN,
>> then drop CAP_SYS_ADMIN and keep reading. Then later CAP_SYS_LOG was
>> introduced. So if a daemon switched from /proc/kmsg to /dev/kmsg they
>> wouldn't be able to drop the capability. But, it's much saner to carry
>> CAP_SYS_LOG than CAP_SYS_ADMIN on a long-running daemon.
>
> I have no idea on this front.  I'll let Kay speak to that.

The original code checks once at open() only, which would allow to do
do all that privilege dropping. It is how I would expect it to work,
instead of checking the permissions at every read().

> On my
> currently running Fedora 18 system, I actually have systemd-journald
> using /dev/kmsg

That's the recent structured logging stuff.

> and rsyslog using /proc/kmsg.

That's the old plain text syslog daemon stuff.

> Why I have both, I have no friggin idea.

Nobody removed the old syslog dameon by default from the distro. If
you don't want or need the plain text files in /var/log/ anymore, just
uninstall it and use journalctl(1) to see the system logs from then
on.

>> Is there an intention to use /dev/kmsg for the syslog management daemon?

Not that I know.

> Maybe?  I mean, systemd-journald seems to be using it for something.
> Kay?

I doubt that old syslog implementations will be ported to a new kernel
interface. They work just fine the way they are, and the structured
data that is additionally put out on the new interface, they cannot
really store away anyway in their plain text files, so they do not
gain anything really.

What we can probably expect though, is that in the future the default
systems will not install any old syslog daemon, which uses that
interface anymore.

Kay