* [PATCH] printk: Check real user/group id for %pK
@ 2013-09-29 22:35 Ryan Mallon
2013-09-29 23:15 ` George Spelvin
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Mallon @ 2013-09-29 22:35 UTC (permalink / raw
To: LKML, linux, Jiri Kosina, eldad, Andrew Morton, Dan Rosenberg; +Cc: LKML
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]) {
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] printk: Check real user/group id for %pK
2013-09-29 22:35 [PATCH] printk: Check real user/group id for %pK Ryan Mallon
@ 2013-09-29 23:15 ` George Spelvin
2013-09-29 23:26 ` Ryan Mallon
0 siblings, 1 reply; 7+ messages in thread
From: George Spelvin @ 2013-09-29 23:15 UTC (permalink / raw
To: akpm, dan.j.rosenberg, eldad, jkosina, linux-kernel, linux,
rmallon
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] printk: Check real user/group id for %pK
2013-09-29 23:15 ` George Spelvin
@ 2013-09-29 23:26 ` Ryan Mallon
2013-09-29 23:41 ` George Spelvin
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Mallon @ 2013-09-29 23:26 UTC (permalink / raw
To: George Spelvin; +Cc: akpm, dan.j.rosenberg, eldad, jkosina, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] printk: Check real user/group id for %pK
2013-09-29 23:26 ` Ryan Mallon
@ 2013-09-29 23:41 ` George Spelvin
2013-09-30 0:41 ` Dan Rosenberg
0 siblings, 1 reply; 7+ messages in thread
From: George Spelvin @ 2013-09-29 23:41 UTC (permalink / raw
To: linux, rmallon; +Cc: akpm, dan.j.rosenberg, eldad, jkosina, linux-kernel
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] printk: Check real user/group id for %pK
2013-09-29 23:41 ` George Spelvin
@ 2013-09-30 0:41 ` Dan Rosenberg
2013-09-30 0:56 ` Ryan Mallon
0 siblings, 1 reply; 7+ messages in thread
From: Dan Rosenberg @ 2013-09-30 0:41 UTC (permalink / raw
To: George Spelvin; +Cc: rmallon, akpm, eldad, jkosina, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] printk: Check real user/group id for %pK
2013-09-30 0:41 ` Dan Rosenberg
@ 2013-09-30 0:56 ` Ryan Mallon
2013-09-30 0:59 ` Dan Rosenberg
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Mallon @ 2013-09-30 0:56 UTC (permalink / raw
To: Dan Rosenberg; +Cc: George Spelvin, akpm, eldad, jkosina, linux-kernel
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] printk: Check real user/group id for %pK
2013-09-30 0:56 ` Ryan Mallon
@ 2013-09-30 0:59 ` Dan Rosenberg
0 siblings, 0 replies; 7+ messages in thread
From: Dan Rosenberg @ 2013-09-30 0:59 UTC (permalink / raw
To: Ryan Mallon; +Cc: George Spelvin, akpm, eldad, jkosina, linux-kernel
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2013-09-30 1:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-29 22:35 [PATCH] printk: Check real user/group id for %pK Ryan Mallon
2013-09-29 23:15 ` George Spelvin
2013-09-29 23:26 ` Ryan Mallon
2013-09-29 23:41 ` George Spelvin
2013-09-30 0:41 ` Dan Rosenberg
2013-09-30 0:56 ` Ryan Mallon
2013-09-30 0:59 ` Dan Rosenberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.