[PATCH v2] vsprintf: Check real user/group id for %pK

[PATCH v2] vsprintf: Check real user/group id for %pK

[Ryan Mallon]

Some setuid binaries will allow reading of files which have read
permission by the real user id. This is problematic with files which
use %pK because the file access permission is checked at open() time,
but the kptr_restrict setting is checked at read() time. If a setuid
binary opens a %pK file as an unprivileged user, and then elevates
permissions before reading the file, then kernel pointer values may be
leaked.
    
This happens for example with the setuid pppd application on Ubuntu 12.04:
    
  $ head -1 /proc/kallsyms
  00000000 T startup_32
    
  $ pppd file /proc/kallsyms
  pppd: In file /proc/kallsyms: unrecognized option 'c1000000'
    
This will only leak the pointer value from the first line, but other
setuid binaries may leak more information.
    
Fix this by adding a check that in addition to the current process
having CAP_SYSLOG, that effective user and group ids are equal to the
real ids. If a setuid binary reads the contents of a file which uses
%pK then the pointer values will be printed as NULL if the real user
is unprivileged.
    
Signed-off-by: Ryan Mallon <rmallon@gmail.com>
---
Changes since v1:

 * Fix the description to say 'vsprintf' instead of 'printk'.
 * Clarify the open() vs read() time checks in the patch description and
code comment.
 * Remove comment about 'badly written' setuid binaries. This occurs
with setuids binaries which correctly handle opening files.
 * Added extra people to the Cc list.
---

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..c02faf3 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1312,10 +1312,26 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				spec.field_width = default_width;
 			return string(buf, end, "pK-error", spec);
 		}
-		if (!((kptr_restrict == 0) ||
-		      (kptr_restrict == 1 &&
-		       has_capability_noaudit(current, CAP_SYSLOG))))
-			ptr = NULL;
+
+		/*
+		 * If kptr_restrict is set to 2, then %pK always prints as
+		 * NULL. If it is set to 1, then only print the real pointer
+		 * value if the current proccess has CAP_SYSLOG and is running
+		 * with the same credentials it started with. This is because
+		 * access to files is checked at open() time, but %pK checks
+		 * permission at read() time. We don't want to leak pointer
+		 * values if a binary opens a file using %pK and then elevates
+		 * privileges before reading it.
+		 */
+		{
+			const struct cred *cred = current_cred();
+
+			if (kptr_restrict == 2 || (kptr_restrict == 1 &&
+			     (!has_capability_noaudit(current, CAP_SYSLOG) ||
+			      !uid_eq(cred->euid, cred->uid) ||
+			      !gid_eq(cred->egid, cred->gid))))
+				ptr = NULL;
+		}
 		break;
 	case 'N':
 		switch (fmt[1]) {

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Joe Perches]

On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
> Some setuid binaries will allow reading of files which have read
> permission by the real user id. This is problematic with files which
> use %pK because the file access permission is checked at open() time,
> but the kptr_restrict setting is checked at read() time. If a setuid
> binary opens a %pK file as an unprivileged user, and then elevates
> permissions before reading the file, then kernel pointer values may be
> leaked.

I think it should explicitly test 0.

Dan? Might this be any problem?

Otherwise, just style notes:

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1312,10 +1312,26 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  				spec.field_width = default_width;
>  			return string(buf, end, "pK-error", spec);
>  		}
> -		if (!((kptr_restrict == 0) ||
> -		      (kptr_restrict == 1 &&
> -		       has_capability_noaudit(current, CAP_SYSLOG))))
> -			ptr = NULL;
> +
> +		/*
> +		 * If kptr_restrict is set to 2, then %pK always prints as
> +		 * NULL. If it is set to 1, then only print the real pointer
> +		 * value if the current proccess has CAP_SYSLOG and is running
> +		 * with the same credentials it started with. This is because
> +		 * access to files is checked at open() time, but %pK checks
> +		 * permission at read() time. We don't want to leak pointer
> +		 * values if a binary opens a file using %pK and then elevates
> +		 * privileges before reading it.
> +		 */
> +		{
> +			const struct cred *cred = current_cred();

Please add #include <linux/cred.h>

> +			if (kptr_restrict == 2 || (kptr_restrict == 1 &&
> +			     (!has_capability_noaudit(current, CAP_SYSLOG) ||
> +			      !uid_eq(cred->euid, cred->uid) ||
> +			      !gid_eq(cred->egid, cred->gid))))
> +				ptr = NULL;
> +		}
>  		break;

Also, it might be easier to read as:

		if (kptr_restrict == 0)
			break;
		else if (kptr_restrict == 1) {
			const struct cred *cred = current_cred();

			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
			    !uid_eq(cred->euid, cred->uid) ||
			    !gid_eq(cred->egid, cred->gid))
				ptr = NULL;
		} else {
			ptr = NULL;
		}
		break;

>  	case 'N':
>  		switch (fmt[1]) {

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Joe Perches]

On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote:
> On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
> > Some setuid binaries will allow reading of files which have read
> > permission by the real user id. This is problematic with files which
> > use %pK because the file access permission is checked at open() time,
> > but the kptr_restrict setting is checked at read() time. If a setuid
> > binary opens a %pK file as an unprivileged user, and then elevates
> > permissions before reading the file, then kernel pointer values may be
> > leaked.
> 
> I think it should explicitly test 0.

Also, Documentation/sysctl/kernel.txt should be updated too.

Here's a suggested patch:

---
 Documentation/sysctl/kernel.txt | 14 ++++++++------
 lib/vsprintf.c                  | 38 ++++++++++++++++++++++++++------------
 2 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..eac53d5 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -290,13 +290,15 @@ Default value is "/sbin/hotplug".
 kptr_restrict:
 
 This toggle indicates whether restrictions are placed on
-exposing kernel addresses via /proc and other interfaces.  When
-kptr_restrict is set to (0), there are no restrictions.  When
-kptr_restrict is set to (1), the default, kernel pointers
+exposing kernel addresses via /proc and other interfaces.
+
+When kptr_restrict is set to (0), there are no restrictions.
+When kptr_restrict is set to (1), the default, kernel pointers
 printed using the %pK format specifier will be replaced with 0's
-unless the user has CAP_SYSLOG.  When kptr_restrict is set to
-(2), kernel pointers printed using %pK will be replaced with 0's
-regardless of privileges.
+unless the user has CAP_SYSLOG and effective user and group ids
+are equal to the real ids.
+When kptr_restrict is set to (2), kernel pointers printed using
+%pK will be replaced with 0's regardless of privileges.
 
 ==============================================================
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..986fdbe 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
 #include <linux/dcache.h>
+#include <linux/cred.h>
 #include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
@@ -1302,20 +1303,33 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 			return buf;
 		}
 	case 'K':
-		/*
-		 * %pK cannot be used in IRQ context because its test
-		 * for CAP_SYSLOG would be meaningless.
-		 */
-		if (kptr_restrict && (in_irq() || in_serving_softirq() ||
-				      in_nmi())) {
-			if (spec.field_width == -1)
-				spec.field_width = default_width;
-			return string(buf, end, "pK-error", spec);
+		switch (kptr_restrict) {
+		case 0:			/* None */
+			break;
+		case 1: {		/* Restricted (the default) */
+			const struct cred *cred;
+
+			if (in_irq() || in_serving_softirq() || in_nmi()) {
+				/*
+				 * This cannot be used in IRQ context because
+				 * the test for CAP_SYSLOG would be meaningless
+				 */
+				if (spec.field_width == -1)
+					spec.field_width = default_width;
+				return string(buf, end, "pK-error", spec);
+			}
+			cred = current_cred();
+			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+			    !uid_eq(cred->euid, cred->uid) ||
+			    !gid_eq(cred->egid, cred->gid))
+				ptr = NULL;
+			break;
 		}
-		if (!((kptr_restrict == 0) ||
-		      (kptr_restrict == 1 &&
-		       has_capability_noaudit(current, CAP_SYSLOG))))
+		case 2:			/* Forbidden - Always 0 */
+		default:
 			ptr = NULL;
+			break;
+		}
 		break;
 	case 'N':
 		switch (fmt[1]) {

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Ryan Mallon]

On 09/10/13 12:30, Joe Perches wrote:
> On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote:
>> On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
>>> Some setuid binaries will allow reading of files which have read
>>> permission by the real user id. This is problematic with files which
>>> use %pK because the file access permission is checked at open() time,
>>> but the kptr_restrict setting is checked at read() time. If a setuid
>>> binary opens a %pK file as an unprivileged user, and then elevates
>>> permissions before reading the file, then kernel pointer values may be
>>> leaked.
>>
>> I think it should explicitly test 0.
> 
> Also, Documentation/sysctl/kernel.txt should be updated too.
> 
> Here's a suggested patch:
> 
> ---
>  Documentation/sysctl/kernel.txt | 14 ++++++++------
>  lib/vsprintf.c                  | 38 ++++++++++++++++++++++++++------------
>  2 files changed, 34 insertions(+), 18 deletions(-)
> 
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 9d4c1d1..eac53d5 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -290,13 +290,15 @@ Default value is "/sbin/hotplug".
>  kptr_restrict:
>  
>  This toggle indicates whether restrictions are placed on
> -exposing kernel addresses via /proc and other interfaces.  When
> -kptr_restrict is set to (0), there are no restrictions.  When
> -kptr_restrict is set to (1), the default, kernel pointers
> +exposing kernel addresses via /proc and other interfaces.
> +
> +When kptr_restrict is set to (0), there are no restrictions.
> +When kptr_restrict is set to (1), the default, kernel pointers
>  printed using the %pK format specifier will be replaced with 0's
> -unless the user has CAP_SYSLOG.  When kptr_restrict is set to
> -(2), kernel pointers printed using %pK will be replaced with 0's
> -regardless of privileges.
> +unless the user has CAP_SYSLOG and effective user and group ids
> +are equal to the real ids.
> +When kptr_restrict is set to (2), kernel pointers printed using
> +%pK will be replaced with 0's regardless of privileges.

I'll add this, thanks.

I'm less fussed about the suggestions for the logic. The current test is
small and concise. The original also does the in_irq tests regardless of
the kptr_restrict setting since they are mostly intended to catch
internal kernel bugs.

Anyway, I am mostly interested to hear if the solution is acceptable, or
if a more involved open() vs read() time check is required.

~Ryan

>  
>  ==============================================================
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 26559bd..986fdbe 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -27,6 +27,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/ioport.h>
>  #include <linux/dcache.h>
> +#include <linux/cred.h>
>  #include <net/addrconf.h>
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
> @@ -1302,20 +1303,33 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  			return buf;
>  		}
>  	case 'K':
> -		/*
> -		 * %pK cannot be used in IRQ context because its test
> -		 * for CAP_SYSLOG would be meaningless.
> -		 */
> -		if (kptr_restrict && (in_irq() || in_serving_softirq() ||
> -				      in_nmi())) {
> -			if (spec.field_width == -1)
> -				spec.field_width = default_width;
> -			return string(buf, end, "pK-error", spec);
> +		switch (kptr_restrict) {
> +		case 0:			/* None */
> +			break;
> +		case 1: {		/* Restricted (the default) */
> +			const struct cred *cred;
> +
> +			if (in_irq() || in_serving_softirq() || in_nmi()) {
> +				/*
> +				 * This cannot be used in IRQ context because
> +				 * the test for CAP_SYSLOG would be meaningless
> +				 */
> +				if (spec.field_width == -1)
> +					spec.field_width = default_width;
> +				return string(buf, end, "pK-error", spec);
> +			}
> +			cred = current_cred();
> +			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
> +			    !uid_eq(cred->euid, cred->uid) ||
> +			    !gid_eq(cred->egid, cred->gid))
> +				ptr = NULL;
> +			break;
>  		}
> -		if (!((kptr_restrict == 0) ||
> -		      (kptr_restrict == 1 &&
> -		       has_capability_noaudit(current, CAP_SYSLOG))))
> +		case 2:			/* Forbidden - Always 0 */
> +		default:
>  			ptr = NULL;
> +			break;
> +		}
>  		break;
>  	case 'N':
>  		switch (fmt[1]) {
> 
> 

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Joe Perches]

On Wed, 2013-10-09 at 12:55 +1100, Ryan Mallon wrote:
> On 09/10/13 12:30, Joe Perches wrote:
> > On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote:
> >> On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
> >>> Some setuid binaries will allow reading of files which have read
> >>> permission by the real user id. This is problematic with files which
> >>> use %pK because the file access permission is checked at open() time,
> >>> but the kptr_restrict setting is checked at read() time. If a setuid
> >>> binary opens a %pK file as an unprivileged user, and then elevates
> >>> permissions before reading the file, then kernel pointer values may be
> >>> leaked.
> >>
> >> I think it should explicitly test 0.
> > 
> > Also, Documentation/sysctl/kernel.txt should be updated too.
> > 
> > Here's a suggested patch:
> > 
> > ---
> >  Documentation/sysctl/kernel.txt | 14 ++++++++------
> >  lib/vsprintf.c                  | 38 ++++++++++++++++++++++++++------------
> >  2 files changed, 34 insertions(+), 18 deletions(-)
> > 
> > diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> > index 9d4c1d1..eac53d5 100644
> > --- a/Documentation/sysctl/kernel.txt
> > +++ b/Documentation/sysctl/kernel.txt
> > @@ -290,13 +290,15 @@ Default value is "/sbin/hotplug".
> >  kptr_restrict:
> >  
> >  This toggle indicates whether restrictions are placed on
> > -exposing kernel addresses via /proc and other interfaces.  When
> > -kptr_restrict is set to (0), there are no restrictions.  When
> > -kptr_restrict is set to (1), the default, kernel pointers
> > +exposing kernel addresses via /proc and other interfaces.
> > +
> > +When kptr_restrict is set to (0), there are no restrictions.
> > +When kptr_restrict is set to (1), the default, kernel pointers
> >  printed using the %pK format specifier will be replaced with 0's
> > -unless the user has CAP_SYSLOG.  When kptr_restrict is set to
> > -(2), kernel pointers printed using %pK will be replaced with 0's
> > -regardless of privileges.
> > +unless the user has CAP_SYSLOG and effective user and group ids
> > +are equal to the real ids.
> > +When kptr_restrict is set to (2), kernel pointers printed using
> > +%pK will be replaced with 0's regardless of privileges.
> 
> I'll add this, thanks.
> 
> I'm less fussed about the suggestions for the logic. The current test is
> small and concise.

The logic ends up the same to the compiler, but it's
human readers that want simple and clear.

> The original also does the in_irq tests regardless of
> the kptr_restrict setting since they are mostly intended to catch
> internal kernel bugs.

Not so.

http://marc.info/?l=linux-security-module&m=129303800912245&w=4
https://lkml.org/lkml/2012/7/13/428

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Ryan Mallon]

On 09/10/13 13:00, Joe Perches wrote:
> On Wed, 2013-10-09 at 12:55 +1100, Ryan Mallon wrote:
>> On 09/10/13 12:30, Joe Perches wrote:
>>> On Tue, 2013-10-08 at 17:49 -0700, Joe Perches wrote:
>>>> On Wed, 2013-10-09 at 11:15 +1100, Ryan Mallon wrote:
>>>>> Some setuid binaries will allow reading of files which have read
>>>>> permission by the real user id. This is problematic with files which
>>>>> use %pK because the file access permission is checked at open() time,
>>>>> but the kptr_restrict setting is checked at read() time. If a setuid
>>>>> binary opens a %pK file as an unprivileged user, and then elevates
>>>>> permissions before reading the file, then kernel pointer values may be
>>>>> leaked.
>>>> I think it should explicitly test 0.
>>> Also, Documentation/sysctl/kernel.txt should be updated too.
>>>
>>> Here's a suggested patch:
>>>
>>> ---
>>>  Documentation/sysctl/kernel.txt | 14 ++++++++------
>>>  lib/vsprintf.c                  | 38 ++++++++++++++++++++++++++------------
>>>  2 files changed, 34 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
>>> index 9d4c1d1..eac53d5 100644
>>> --- a/Documentation/sysctl/kernel.txt
>>> +++ b/Documentation/sysctl/kernel.txt
>>> @@ -290,13 +290,15 @@ Default value is "/sbin/hotplug".
>>>  kptr_restrict:
>>>  
>>>  This toggle indicates whether restrictions are placed on
>>> -exposing kernel addresses via /proc and other interfaces.  When
>>> -kptr_restrict is set to (0), there are no restrictions.  When
>>> -kptr_restrict is set to (1), the default, kernel pointers
>>> +exposing kernel addresses via /proc and other interfaces.
>>> +
>>> +When kptr_restrict is set to (0), there are no restrictions.
>>> +When kptr_restrict is set to (1), the default, kernel pointers
>>>  printed using the %pK format specifier will be replaced with 0's
>>> -unless the user has CAP_SYSLOG.  When kptr_restrict is set to
>>> -(2), kernel pointers printed using %pK will be replaced with 0's
>>> -regardless of privileges.
>>> +unless the user has CAP_SYSLOG and effective user and group ids
>>> +are equal to the real ids.
>>> +When kptr_restrict is set to (2), kernel pointers printed using
>>> +%pK will be replaced with 0's regardless of privileges.
>> I'll add this, thanks.
>>
>> I'm less fussed about the suggestions for the logic. The current test is
>> small and concise.
> The logic ends up the same to the compiler, but it's
> human readers that want simple and clear.
>
>> The original also does the in_irq tests regardless of
>> the kptr_restrict setting since they are mostly intended to catch
>> internal kernel bugs.
> Not so.
>
> http://marc.info/?l=linux-security-module&m=129303800912245&w=4
> https://lkml.org/lkml/2012/7/13/428
>

Ah, I misread it. It does however check when kptr_restrict != 0, not
just when kptr_restrict is 1. I've left the in_irq test as-is, but used
a switch as suggested. I don't really care either way, I think the
original check is quite readable. Anyway, updated patch below:

~Ryan

---

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..eac53d5 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -290,13 +290,15 @@ Default value is "/sbin/hotplug".
 kptr_restrict:
 
 This toggle indicates whether restrictions are placed on
-exposing kernel addresses via /proc and other interfaces.  When
-kptr_restrict is set to (0), there are no restrictions.  When
-kptr_restrict is set to (1), the default, kernel pointers
+exposing kernel addresses via /proc and other interfaces.
+
+When kptr_restrict is set to (0), there are no restrictions.
+When kptr_restrict is set to (1), the default, kernel pointers
 printed using the %pK format specifier will be replaced with 0's
-unless the user has CAP_SYSLOG.  When kptr_restrict is set to
-(2), kernel pointers printed using %pK will be replaced with 0's
-regardless of privileges.
+unless the user has CAP_SYSLOG and effective user and group ids
+are equal to the real ids.
+When kptr_restrict is set to (2), kernel pointers printed using
+%pK will be replaced with 0's regardless of privileges.
 
 ==============================================================
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..6dd8c5d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -27,6 +27,7 @@
 #include <linux/uaccess.h>
 #include <linux/ioport.h>
 #include <linux/dcache.h>
+#include <linux/cred.h>
 #include <net/addrconf.h>
 
 #include <asm/page.h>		/* for PAGE_SIZE */
@@ -1312,11 +1313,36 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 				spec.field_width = default_width;
 			return string(buf, end, "pK-error", spec);
 		}
-		if (!((kptr_restrict == 0) ||
-		      (kptr_restrict == 1 &&
-		       has_capability_noaudit(current, CAP_SYSLOG))))
+
+		switch (kptr_restrict) {
+		case 0:
+			/* Always print %pK values */
+			break;
+		case 1: {
+			/*
+			 * Only print the real pointer value if the current
+			 * proccess has CAP_SYSLOG and is running with the
+			 * same credentials it started with. This is because
+			 * access to files is checked at open() time, but %pK
+			 * checks permission at read() time. We don't want to
+			 * leak pointer values if a binary opens a file using
+			 * %pK and then elevates privileges before reading it.
+			 */
+			const struct cred *cred = current_cred();
+
+			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
+			    !uid_eq(cred->euid, cred->uid) ||
+			    !gid_eq(cred->egid, cred->gid))
+				ptr = NULL;
+			break;
+		}
+		default:
+			/* Always print 0's for %pK */
 			ptr = NULL;
+			break;
+		}
 		break;
+
 	case 'N':
 		switch (fmt[1]) {
 		case 'F':

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Joe Perches]

On Wed, 2013-10-09 at 13:22 +1100, Ryan Mallon wrote:

> Anyway, updated patch below:

nit:

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> @@ -1312,11 +1313,36 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  				spec.field_width = default_width;
>  			return string(buf, end, "pK-error", spec);
>  		}
> -		if (!((kptr_restrict == 0) ||
> -		      (kptr_restrict == 1 &&
> -		       has_capability_noaudit(current, CAP_SYSLOG))))
> +
> +		switch (kptr_restrict) {
> +		case 0:
> +			/* Always print %pK values */
> +			break;
> +		case 1: {
> +			/*
> +			 * Only print the real pointer value if the current
> +			 * proccess has CAP_SYSLOG and is running with the

s/proccess/process/ typo

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Dan Rosenberg]

On 10/08/2013 10:22 PM, Ryan Mallon wrote:
> Ah, I misread it. It does however check when kptr_restrict != 0, not
> just when kptr_restrict is 1. I've left the in_irq test as-is, but used
> a switch as suggested. I don't really care either way, I think the
> original check is quite readable. Anyway, updated patch below:
>
> ~Ryan

This seems mostly fine to me, except the "proccess" -> "process" nit Joe
already identified.

I think I also prefer Joe's style of having an explicit "case 2" in the
switch statement in addition to the default case for clarity.

Also, isn't the default value of kptr_restrict 0 now, unless I'm missing
something? If I recall it was 1 when originally written, and then
changed to 0 at some point. Could the documentation be updated to
reflect that?

-Dan

> ---
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index 9d4c1d1..eac53d5 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -290,13 +290,15 @@ Default value is "/sbin/hotplug".
>  kptr_restrict:
>  
>  This toggle indicates whether restrictions are placed on
> -exposing kernel addresses via /proc and other interfaces.  When
> -kptr_restrict is set to (0), there are no restrictions.  When
> -kptr_restrict is set to (1), the default, kernel pointers
> +exposing kernel addresses via /proc and other interfaces.
> +
> +When kptr_restrict is set to (0), there are no restrictions.
> +When kptr_restrict is set to (1), the default, kernel pointers
>  printed using the %pK format specifier will be replaced with 0's
> -unless the user has CAP_SYSLOG.  When kptr_restrict is set to
> -(2), kernel pointers printed using %pK will be replaced with 0's
> -regardless of privileges.
> +unless the user has CAP_SYSLOG and effective user and group ids
> +are equal to the real ids.
> +When kptr_restrict is set to (2), kernel pointers printed using
> +%pK will be replaced with 0's regardless of privileges.
>  
>  ==============================================================
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 26559bd..6dd8c5d 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -27,6 +27,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/ioport.h>
>  #include <linux/dcache.h>
> +#include <linux/cred.h>
>  #include <net/addrconf.h>
>  
>  #include <asm/page.h>		/* for PAGE_SIZE */
> @@ -1312,11 +1313,36 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  				spec.field_width = default_width;
>  			return string(buf, end, "pK-error", spec);
>  		}
> -		if (!((kptr_restrict == 0) ||
> -		      (kptr_restrict == 1 &&
> -		       has_capability_noaudit(current, CAP_SYSLOG))))
> +
> +		switch (kptr_restrict) {
> +		case 0:
> +			/* Always print %pK values */
> +			break;
> +		case 1: {
> +			/*
> +			 * Only print the real pointer value if the current
> +			 * proccess has CAP_SYSLOG and is running with the
> +			 * same credentials it started with. This is because
> +			 * access to files is checked at open() time, but %pK
> +			 * checks permission at read() time. We don't want to
> +			 * leak pointer values if a binary opens a file using
> +			 * %pK and then elevates privileges before reading it.
> +			 */
> +			const struct cred *cred = current_cred();
> +
> +			if (!has_capability_noaudit(current, CAP_SYSLOG) ||
> +			    !uid_eq(cred->euid, cred->uid) ||
> +			    !gid_eq(cred->egid, cred->gid))
> +				ptr = NULL;
> +			break;
> +		}
> +		default:
> +			/* Always print 0's for %pK */
>  			ptr = NULL;
> +			break;
> +		}
>  		break;
> +
>  	case 'N':
>  		switch (fmt[1]) {
>  		case 'F':
>
>
>
>
>

Re: [PATCH v2] vsprintf: Check real user/group id for %pK

[Joe Perches]

On Wed, 2013-10-09 at 07:14 -0400, Dan Rosenberg wrote:
> isn't the default value of kptr_restrict 0 now, unless I'm missing
> something? If I recall it was 1 when originally written, and then
> changed to 0 at some point. Could the documentation be updated to
> reflect that?

Yeah, the default got changed by
---------------------------
commit 411f05f123cbd7f8aa1edcae86970755a6e2a9d9
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu May 12 23:00:28 2011 +0200

kptr_restrict has been triggering bugs in apps such as perf, and it also makes
the system less useful by default, so turn it off by default.
---------------------------

Maybe this:
---
 Documentation/sysctl/kernel.txt | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 9d4c1d1..c17d5ca 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -289,14 +289,15 @@ Default value is "/sbin/hotplug".
 
 kptr_restrict:
 
-This toggle indicates whether restrictions are placed on
-exposing kernel addresses via /proc and other interfaces.  When
-kptr_restrict is set to (0), there are no restrictions.  When
-kptr_restrict is set to (1), the default, kernel pointers
-printed using the %pK format specifier will be replaced with 0's
-unless the user has CAP_SYSLOG.  When kptr_restrict is set to
-(2), kernel pointers printed using %pK will be replaced with 0's
-regardless of privileges.
+This toggle indicates whether restrictions are placed on exposing kernel
+addresses via /proc and other interfaces.
+
+When kptr_restrict is set to (0), the default, there are no restrictions.
+When kptr_restrict is set to (1), kernel pointers printed using the %pK
+format specifier will be replaced with 0's unless the user has CAP_SYSLOG
+and effective user and group ids are equal to the real ids.
+When kptr_restrict is set to (2), kernel pointers printed using %pK will
+be replaced with 0's regardless of privileges.
 
 ==============================================================