Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755935Ab3JICWW (ORCPT ); Tue, 8 Oct 2013 22:22:22 -0400 Received: from mail-pa0-f41.google.com ([209.85.220.41]:46103 "EHLO mail-pa0-f41.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751288Ab3JICWV (ORCPT ); Tue, 8 Oct 2013 22:22:21 -0400 Message-ID: <5254BDD0.7040001@gmail.com> Date: Wed, 09 Oct 2013 13:22:08 +1100 From: Ryan Mallon User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.0 MIME-Version: 1.0 To: Joe Perches CC: Andrew Morton , eldad@fogrefinery.com, Jiri Kosina , jgunthorpe@obsidianresearch.com, Dan Rosenberg , Kees Cook , Alexander Viro , "Eric W. Biederman" , George Spelvin , "kernel-hardening@lists.openwall.com" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2] vsprintf: Check real user/group id for %pK References: <5254A034.5020700@gmail.com> <1381279758.23937.42.camel@joe-AO722> <1381282200.23937.45.camel@joe-AO722> <5254B787.6080700@gmail.com> <1381284056.23937.49.camel@joe-AO722> In-Reply-To: <1381284056.23937.49.camel@joe-AO722> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5999 Lines: 158 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 #include #include +#include #include #include /* 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': -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/