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

[PATCH] printk: 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 contents of the file is different when opened as root, and displaying the contents may leak kernel pointer values.
    
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>
---

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 26559bd..b1cd14d 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1312,10 +1312,24 @@ 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. We don't want
+		 * badly written setuid binaries being able to read the real
+		 * pointers on behalf of unprivileged users.
+		 */
+		{
+			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] printk: Check real user/group id for %pK

[George Spelvin]

The basic idea is good, but I'm not sure if this is the correct permission
check to use.

After all, a setuid program might also want to give filtered access to
a /proc file with some %pK values.

The fundamental problem is that %pK is using permissions at the time
of the read(), while the general Unix rule that setuid programs expect
is that permission is checked at open() time.  pppd is an example; its
options_fom_file() function (pppd/options.c:391 in the 2.4.5 release)
does:

    euid = geteuid();
    if (check_prot && seteuid(getuid()) == -1) {
	option_error("unable to drop privileges to open %s: %m", filename);
	return 0;
    }
    f = fopen(filename, "r");
    err = errno;
    if (check_prot && seteuid(euid) == -1)
	fatal("unable to regain privileges");


Now the whole struct cred and capability system is something I don't
really understand, but it is clear from a brief look at the code
that getting the appropriate credential through the seq_file to
lib/vsprintf.c:pointer() would be tricky.

But it also seems like the Right Thing to do; other fixes seem like
ineffective kludges.

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

[Ryan Mallon]

On 30/09/13 09:15, George Spelvin wrote:
> The basic idea is good, but I'm not sure if this is the correct permission
> check to use.
> 
> After all, a setuid program might also want to give filtered access to
> a /proc file with some %pK values.

Yeah. I'm not sure if this will break some applications (e.g. perf?)
which are expecting to be able to read %pK values if invoked setuid. The
problem is that allowing that can potentially be used to leak
information too.

> 
> The fundamental problem is that %pK is using permissions at the time
> of the read(), while the general Unix rule that setuid programs expect
> is that permission is checked at open() time.  pppd is an example; its
> options_fom_file() function (pppd/options.c:391 in the 2.4.5 release)
> does:
> 
>     euid = geteuid();
>     if (check_prot && seteuid(getuid()) == -1) {
> 	option_error("unable to drop privileges to open %s: %m", filename);
> 	return 0;
>     }
>     f = fopen(filename, "r");
>     err = errno;
>     if (check_prot && seteuid(euid) == -1)
> 	fatal("unable to regain privileges");
> 

Right, so the pppd application is actually doing the correct thing.

> 
> Now the whole struct cred and capability system is something I don't
> really understand, but it is clear from a brief look at the code
> that getting the appropriate credential through the seq_file to
> lib/vsprintf.c:pointer() would be tricky.

:-).

> 
> But it also seems like the Right Thing to do; other fixes seem like
> ineffective kludges.

Will wait and see what others have to say.

(also apologies for the badly formatted initial post. The mail client on
my other machine is apparently not configured properly).

Thanks,
~Ryan

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

[George Spelvin]

> Right, so the pppd application is actually doing the correct thing.

And a CAP_SYSLOG setuid binary that *doesn't* DTRT seems like a more
immediate security hole than leaking kernel addresses.  After all
kptr_restrict is optional precisely because the benefit is marginal.

The interesting question is what credentials make sense for %pK outside
of a seq_printf().  Does it even make sense in a generic printk?  In that
case, it's the permission of the syslogd that matters rather than the
process generating the message.

> Will wait and see what others have to say.

Me, too.  Dan in particular.

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

[Dan Rosenberg]

On 09/29/2013 07:41 PM, George Spelvin wrote:
>> Right, so the pppd application is actually doing the correct thing.
> And a CAP_SYSLOG setuid binary that *doesn't* DTRT seems like a more
> immediate security hole than leaking kernel addresses.  After all
> kptr_restrict is optional precisely because the benefit is marginal.
>
> The interesting question is what credentials make sense for %pK outside
> of a seq_printf().  Does it even make sense in a generic printk?  In that
> case, it's the permission of the syslogd that matters rather than the
> process generating the message.
>
>> Will wait and see what others have to say.
> Me, too.  Dan in particular.

Firstly, I wouldn't recommend applying %pK's to printk usage. Removing
all addresses from the kernel syslog compromises its usefulness in
debugging basically anything at all. Additionally, many printk calls are
performed from a context where a capability check would yield
unpredictable (or at least meaningless) results. If you want to restrict
access to the kernel syslog by unprivileged users, that should be done
by enabling CONFIG_DMESG_RESTRICT, which was written for this purpose.

With that out of the way, I don't have a strong opinion on how to handle
this case. The proposed patch solves the problem but may break setuid
applications that expect to be able to read /proc/kallsyms contents
based on euid (and implicitly, capabilities) alone. But then again,
these mythical setuid applications are probably broken in some
situations anyway, because what happens if /proc/kallsyms is set to "2"
(unconditionally replace addresses with 0's)? I also can't think of a
better solution.

-Dan

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

[Ryan Mallon]

On 30/09/13 10:41, Dan Rosenberg wrote:
> On 09/29/2013 07:41 PM, George Spelvin wrote:
>>> Right, so the pppd application is actually doing the correct thing.
>> And a CAP_SYSLOG setuid binary that *doesn't* DTRT seems like a more
>> immediate security hole than leaking kernel addresses.  After all
>> kptr_restrict is optional precisely because the benefit is marginal.
>>
>> The interesting question is what credentials make sense for %pK outside
>> of a seq_printf().  Does it even make sense in a generic printk?  In that
>> case, it's the permission of the syslogd that matters rather than the
>> process generating the message.
>>
>>> Will wait and see what others have to say.
>> Me, too.  Dan in particular.
> 
> Firstly, I wouldn't recommend applying %pK's to printk usage.

Sorry, the patch description should say 'vsprintf: ' not 'printk: '.
Posting too early in the morning :-).

> Removing
> all addresses from the kernel syslog compromises its usefulness in
> debugging basically anything at all. Additionally, many printk calls are
> performed from a context where a capability check would yield
> unpredictable (or at least meaningless) results. If you want to restrict
> access to the kernel syslog by unprivileged users, that should be done
> by enabling CONFIG_DMESG_RESTRICT, which was written for this purpose.

Agreed.

> With that out of the way, I don't have a strong opinion on how to handle
> this case. The proposed patch solves the problem but may break setuid
> applications that expect to be able to read /proc/kallsyms contents
> based on euid (and implicitly, capabilities) alone. But then again,
> these mythical setuid applications are probably broken in some
> situations anyway, because what happens if /proc/kallsyms is set to "2"
> (unconditionally replace addresses with 0's)? I also can't think of a
> better solution.

Okay, this was just the simplest solution I could come up with that
fixed the issue for me. Is that a tentative acked/reviewed-by? :-).

~Ryan

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

[Dan Rosenberg]

On 09/29/2013 08:56 PM, Ryan Mallon wrote:
> Okay, this was just the simplest solution I could come up with that
> fixed the issue for me. Is that a tentative acked/reviewed-by? :-).
>
> ~Ryan

I'm interested to see if anyone else has alternative ideas, but for now:

Reviewed-by: Dan Rosenberg <dan.j.rosenberg@gmail.com>