* Re: Printk specifiers for __user pointers
2020-11-23 9:53 ` Petr Mladek
@ 2020-11-23 14:11 ` Steven Rostedt
2020-11-24 21:53 ` Kees Cook
1 sibling, 0 replies; 5+ messages in thread
From: Steven Rostedt @ 2020-11-23 14:11 UTC (permalink / raw)
To: Petr Mladek
Cc: Alan Stern, Sergey Senozhatsky, Kernel development list,
Kees Cook, Daniel Borkmann, Linus Torvalds
On Mon, 23 Nov 2020 10:53:24 +0100
Petr Mladek <pmladek@suse.com> wrote:
> On Fri 2020-11-20 13:42:42, Steven Rostedt wrote:
> > On Fri, 20 Nov 2020 11:44:12 -0500
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > > To the VSPRINTF maintainers:
> > >
> > > Documentation/core-api/printk-formats.rst lists a large number of format
> > > specifiers for pointers of various sorts. Yet as far as I can see,
> > > there is no specifier meant for use with __user pointers.
> > >
> > > The security implications of printing the true, unmangled value of a
> > > __user pointer are minimal, since doing so does not leak any kernel
> > > information. So %px would work, but tools like checkpatch.pl don't like
> > > it.
>
> Just to be sure as I am not a security expert. Is there really that
> big difference in the risk? The following scenarios come to my mind:
One of the biggest differences, is that with exposing the kernel, every
process has the same kernel address space. By leaking memory addresses of
the kernel, and knowing of some overflow bug in a system call, you can
exploit it right away.
Also, a user space application could trigger some kind of print to show
that kernel address space.
With having the kernel show the address space of another process, it is not
as easy to exploit. You would need to make that other process do something
to have the kernel show its address space.
>
> 1. The address would show a well defined location in the userspace
> application? Could it be used to attack the application?
It's possible, but the ramifications usually wont be as bad as the kernel.
Unless of course you do it for systemd or some other daemon. But then
again, you still need to have that application cause the print, as any
user space address being printed from the kernel would need to be caused by
that application.
>
> 2. The address shows a location that is being accessed by kernel.
> Could not it be used to pass a value that might be used to attack
> kernel?
I don't know what you mean here.
>
>
> > > Should a new specifier be added? If not, should we simply use %px?
> >
> > There's currently no user of '%pu' (although there is a '%pus'. Perhaps we
> > should have a '%pux'?
> >
> > I would even state that if it is used, that if makes sure that the value is
> > indeed a user space pointer (goes through the same checks as accessing user
> > space), before its printed, otherwise it shows "(fault)" or something.
>
> I have mixed feelings about this.
>
> One one hand, it might make sense to mark locations where userspace
> address is printed. We could easily decide how to print them (hash or
> value) and we could check that it is really from a userspace one.
It would definitely need to be checked that it is from user space.
>
> But I have few concerns:
>
> 1. The existing "%pus" has a kind of opposite meaning. It says what
> address space should be used when the kernel and userspace address
> space is overlapping.
>
> 2. There is the history with "%pk". It did not work because people did
> not use it.
>
> 3. I am not sure about the output when the address is not from
> userspace. Printing ("fault") is not much helpful. Printing
> hashed value might be confusing. Well, I am still not sure
> that it is really safe to print real userspace addresses
> by default.
We could have it print: "(kernel:<hash>)" if it is a kernel address space,
and the hash value wont be as confusing if it states "kernel", and by
showing the hash value, it may be possible to know what was printed there
instead (by possibly seeing another hash with the same value).
-- Steve
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: Printk specifiers for __user pointers
2020-11-23 9:53 ` Petr Mladek
2020-11-23 14:11 ` Steven Rostedt
@ 2020-11-24 21:53 ` Kees Cook
1 sibling, 0 replies; 5+ messages in thread
From: Kees Cook @ 2020-11-24 21:53 UTC (permalink / raw)
To: Petr Mladek
Cc: Steven Rostedt, Alan Stern, Sergey Senozhatsky,
Kernel development list, Daniel Borkmann, Linus Torvalds
On Mon, Nov 23, 2020 at 10:53:24AM +0100, Petr Mladek wrote:
> On Fri 2020-11-20 13:42:42, Steven Rostedt wrote:
> > On Fri, 20 Nov 2020 11:44:12 -0500
> > Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > > To the VSPRINTF maintainers:
> > >
> > > Documentation/core-api/printk-formats.rst lists a large number of format
> > > specifiers for pointers of various sorts. Yet as far as I can see,
> > > there is no specifier meant for use with __user pointers.
> > >
> > > The security implications of printing the true, unmangled value of a
> > > __user pointer are minimal, since doing so does not leak any kernel
> > > information. So %px would work, but tools like checkpatch.pl don't like
> > > it.
>
> Just to be sure as I am not a security expert. Is there really that
> big difference in the risk? The following scenarios come to my mind:
>
> 1. The address would show a well defined location in the userspace
> application? Could it be used to attack the application?
Yes -- this is the primary risk in my view. Exposing addresses of any
kind can be a risk. While an unprivileged user may not have direct
access to dmesg, there tend to be many indirect ways to see its
contents. As such, exposing a userspace address (when not then also
terminating the process, as seen with the segv reporting) poses a
potential exposure risk. I admit it's not a LARGE risk, but modern
attacks use these kind of building blocks to construct all the steps to
reaching their target.
> 2. The address shows a location that is being accessed by kernel.
> Could not it be used to pass a value that might be used to attack
> kernel?
This is also a risk: it provides feedback about where something may be
as a target within a confused-deputy style attack. (i.e. set up one
process to confuse the kernel, and exploit it from another).
> > > Should a new specifier be added? If not, should we simply use %px?
> >
> > There's currently no user of '%pu' (although there is a '%pus'. Perhaps we
> > should have a '%pux'?
> >
> > I would even state that if it is used, that if makes sure that the value is
> > indeed a user space pointer (goes through the same checks as accessing user
> > space), before its printed, otherwise it shows "(fault)" or something.
>
> I have mixed feelings about this.
>
> One one hand, it might make sense to mark locations where userspace
> address is printed. We could easily decide how to print them (hash or
> value) and we could check that it is really from a userspace one.
>
> But I have few concerns:
>
> 1. The existing "%pus" has a kind of opposite meaning. It says what
> address space should be used when the kernel and userspace address
> space is overlapping.
>
> 2. There is the history with "%pk". It did not work because people did
> not use it.
>
> 3. I am not sure about the output when the address is not from
> userspace. Printing ("fault") is not much helpful. Printing
> hashed value might be confusing. Well, I am still not sure
> that it is really safe to print real userspace addresses
> by default.
I think this should just be %px. Or better yet, not printed at all. See
Linus's prior comments:
https://www.kernel.org/doc/html/latest/process/deprecated.html#p-format-specifier
--
Kees Cook
^ permalink raw reply [flat|nested] 5+ messages in thread