| Message ID | 1506816410-10230-5-git-send-email-me@tobin.cc (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Oct 01, 2017 at 11:06:48AM +1100, Tobin C. Harding wrote: > Set the initial value of kptr_restrict to the maximum > setting rather than the minimum setting, to ensure that > early boot logging is not leaking information. > > Signed-off-by: Tobin C. Harding <me@tobin.cc> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote: > Set the initial value of kptr_restrict to the maximum > setting rather than the minimum setting, to ensure that > early boot logging is not leaking information. > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > --- > lib/vsprintf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 0271223..e009325 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -396,7 +396,7 @@ struct printf_spec { > #define FIELD_WIDTH_MAX ((1 << 23) - 1) > #define PRECISION_MAX ((1 << 15) - 1) > > -int kptr_restrict __read_mostly; > +int kptr_restrict __read_mostly = 4; /* maximum setting */ > > /* > * return non-zero if we should cleanse pointers for %p and %pK specifiers. I share Linus's concern that making this the unconditional default will break some people. The intention here (as stated in the changelog) is to cover anything leaked during early boot before sysctl settings can change this value. That shouldn't break perf (which should happen after sysctl settings), but it _may_ break debugging of early boot. I would hope that nothing would be needed there, but if we want to make this more configurable, we may want to consider a Kconfig for the default. Perhaps: -int kptr_restrict __read_mostly; +int kptr_restrict __read_mostly = CONFIG_KPTR_RESTRICT_DEFAULT; ... +config KPTR_RESTRICT_DEFAULT + bool "Avoid leaking kernel pointers to userspace" + default 0 + range 0 4 + help + This specifies the initial value of the kptr_restrict sysctl, which + controls the level of kernel pointers removed from display + to userspace. 0 = off, 1 = %pK not visible to non-root, 2 = %pK + not visible for any user, 3 = %p also not visible, 4 = physical and + resource addresses also not visible. I'd argue that a default of "1" would be a sensible starting place, but that can be a separate patch, IMO. -Kees
> -----Original Message----- > From: keescook@google.com [mailto:keescook@google.com] On Behalf Of Kees > Cook > Sent: Wednesday, October 4, 2017 9:43 AM > To: Tobin C. Harding <me@tobin.cc> > Cc: Greg KH <gregkh@linuxfoundation.org>; Petr Mladek <pmladek@suse.com>; > Joe Perches <joe@perches.com>; Ian Campbell <ijc@hellion.org.uk>; Sergey > Senozhatsky <sergey.senozhatsky@gmail.com>; kernel- > hardening@lists.openwall.com; LKML <linux-kernel@vger.kernel.org>; Catalin > Marinas <catalin.marinas@arm.com>; Will Deacon <will.deacon@arm.com>; > Steven Rostedt <rostedt@goodmis.org>; Roberts, William C > <william.c.roberts@intel.com>; Chris Fries <cfries@google.com>; Dave Weinstein > <olorin@google.com>; Linus Torvalds <torvalds@linux-foundation.org> > Subject: Re: [kernel-hardening] [RFC V2 4/6] lib: vsprintf: default kptr_restrict to > the maximum value > > On Sat, Sep 30, 2017 at 5:06 PM, Tobin C. Harding <me@tobin.cc> wrote: > > Set the initial value of kptr_restrict to the maximum setting rather > > than the minimum setting, to ensure that early boot logging is not > > leaking information. > > > > Signed-off-by: Tobin C. Harding <me@tobin.cc> > > --- > > lib/vsprintf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0271223..e009325 > > 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -396,7 +396,7 @@ struct printf_spec { #define FIELD_WIDTH_MAX ((1 > > << 23) - 1) #define PRECISION_MAX ((1 << 15) - 1) > > > > -int kptr_restrict __read_mostly; > > +int kptr_restrict __read_mostly = 4; /* maximum setting */ > > > > /* > > * return non-zero if we should cleanse pointers for %p and %pK specifiers. > > I share Linus's concern that making this the unconditional default will break some > people. The intention here (as stated in the > changelog) is to cover anything leaked during early boot before sysctl settings can > change this value. That shouldn't break perf (which should happen after sysctl > settings), but it _may_ break debugging of early boot. I would hope that nothing > would be needed there, but if we want to make this more configurable, we may > want to consider a Kconfig for the default. Perhaps: > > -int kptr_restrict __read_mostly; > +int kptr_restrict __read_mostly = CONFIG_KPTR_RESTRICT_DEFAULT; > > ... > > +config KPTR_RESTRICT_DEFAULT > + bool "Avoid leaking kernel pointers to userspace" > + default 0 > + range 0 4 > + help > + This specifies the initial value of the kptr_restrict sysctl, which > + controls the level of kernel pointers removed from display > + to userspace. 0 = off, 1 = %pK not visible to non-root, 2 = %pK > + not visible for any user, 3 = %p also not visible, 4 = physical and > + resource addresses also not visible. > > > I'd argue that a default of "1" would be a sensible starting place, but that can be a > separate patch, IMO. > I might be crazy, as often the case, but a command line option might be useful here so you can boot the same kernel with Kptr < 4 if you really need to get at any information hidden by a configure time value of 4. > -Kees > > -- > Kees Cook > Pixel Security
On Wed, Oct 4, 2017 at 9:42 AM, Kees Cook <keescook@chromium.org> wrote: > > I'd argue that a default of "1" would be a sensible starting place, > but that can be a separate patch, IMO. I agree that '1' is a much saner default for _some_ uses, in that it still gives root access to /proc file data etc. However, the sad fact is that kptr_restrict just has bad semantics for that case too, in that you do want to give root access to /proc files, but the whole "is the current thread root" is a horrible horrible test. Partly it's horrible for the reasons mentioned in the source code (ie the whole IRQ context thing etc), but that's actually the smallest reason it's not great: the more fundamental issue is that even for /proc files, it should use the cred for the file opener, not the current user. And for anything *but* /proc files, it's almost always the wrong thing (ie random printk's aren't generally really associated with any user). It might almost by mistake do the right thing (ie some kernel printk that can be triggered by a normal user doing something odd), so it's not like it always does the wrong thing, but it really is almost entirely random. So I seriously dislike kptr_restrict. The whole thing is mis-designed for very very fundamental reasons. And no, I also refuse to believe that the fix is "just make it have a value of 4, and just hide all pointers". Because that will just mean that people won't be using '%p' at all for debug messages etc, they'll just use %x or whatever. So I honestly doubt the value of kptr_restrict. Any *sane* policy pretty much has to be in the caller, and by thinking about what you print out. IOW, things like proc_pid_wchan(). Linus
On Wed, Oct 4, 2017 at 10:08 AM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > > So I honestly doubt the value of kptr_restrict. Any *sane* policy > pretty much has to be in the caller, and by thinking about what you > print out. IOW, things like proc_pid_wchan(). Looking at /proc/kallsyms is actually a prime example of this. IOW, the old "open /proc/kallsyms as a normal user, then make it stdin for some suid-root program that can be fooled to output it probably works on it. So kptr_restrict ends up being entirely the wrong thing to do there. The only value in kptr_restrict ends up being as a complete hack, where you say "I trust nobody" and make %p almost entirely useless. And as mentioned, that will just make people use %x instead, or randomly sprinkle the new "I didn't really mean this" modifiers like the already discussed pr_debug() case. So even when kptr_restrict "works", it ends up just fighting itself. And most of the time it just doesn't work. Linus
On Wed, Oct 4, 2017 at 7:28 PM, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Wed, Oct 4, 2017 at 10:08 AM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> So I honestly doubt the value of kptr_restrict. Any *sane* policy >> pretty much has to be in the caller, and by thinking about what you >> print out. IOW, things like proc_pid_wchan(). > > Looking at /proc/kallsyms is actually a prime example of this. > > IOW, the old "open /proc/kallsyms as a normal user, then make it stdin > for some suid-root program that can be fooled to output it probably > works on it. Actually, /proc/kallsyms uses %pK, which hacks around this issue by checking for `euid != uid` in addition to the capability check - so this isn't exploitable through a typical setuid program.
On Wed, Oct 4, 2017 at 12:13 PM, Jann Horn <jannh@google.com> wrote: > > Actually, /proc/kallsyms uses %pK, which hacks around this issue > by checking for `euid != uid` in addition to the capability check - so this > isn't exploitable through a typical setuid program. Fair enough, you'd have to be a pretty broken suid program to have set uid to euid before reading some untrusted file descriptor. I could still imagine happening (hey, the X server used to sendmsg file descriptors back and forth), but hopefully it's not really realistic. Linus
diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 0271223..e009325 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -396,7 +396,7 @@ struct printf_spec { #define FIELD_WIDTH_MAX ((1 << 23) - 1) #define PRECISION_MAX ((1 << 15) - 1) -int kptr_restrict __read_mostly; +int kptr_restrict __read_mostly = 4; /* maximum setting */ /* * return non-zero if we should cleanse pointers for %p and %pK specifiers.
Set the initial value of kptr_restrict to the maximum setting rather than the minimum setting, to ensure that early boot logging is not leaking information. Signed-off-by: Tobin C. Harding <me@tobin.cc> --- lib/vsprintf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)