[PATCH] vfsprintf: only hash addresses in security environment

[PATCH] vfsprintf: only hash addresses in security environment

[Jason Yan]

When I am implementing KASLR for powerpc, Scott Wood argued that format
specifier "%p" always hashes the addresses that people do not have a
choice to shut it down: https://patchwork.kernel.org/cover/11367547/

It's true that if in a debug environment or security is not concerned,
such as KASLR is absent or kptr_restrict = 0,  there is no way to shut
the hashing down except changing the code and build the kernel again
to use a different format specifier like "%px". And when we want to
turn to security environment, the format specifier has to be changed
back and rebuild the kernel.

As KASLR is available on most popular platforms and enabled by default,
print the raw value of address while KASLR is absent and kptr_restrict
is zero. Those who concerns about security must have KASLR enabled or
kptr_restrict set properly.

Cc: Scott Wood <oss@buserror.net>
Cc: Kees Cook <keescook@chromium.org>
Cc: "Tobin C . Harding" <tobin@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Daniel Axtens <dja@axtens.net>
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
 lib/vsprintf.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..f74131b152a1 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -2253,8 +2253,15 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return err_ptr(buf, end, ptr, spec);
 	}
 
-	/* default is to _not_ leak addresses, hash before printing */
-	return ptr_to_id(buf, end, ptr, spec);
+	/*
+	 * In security environment, while kaslr is enabled or kptr_restrict is
+	 * not zero, hash before printing so that addresses will not be
+	 * leaked. And if not in a security environment, print the raw value
+	 */
+	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) || kptr_restrict)
+		return ptr_to_id(buf, end, ptr, spec);
+	else
+		return pointer_string(buf, end, ptr, spec);
 }
 
 /*
-- 
2.17.2

Re: [PATCH] vfsprintf: only hash addresses in security environment

[Andy Shevchenko]

On Wed, Mar 04, 2020 at 08:47:07PM +0800, Jason Yan wrote:
> When I am implementing KASLR for powerpc, Scott Wood argued that format
> specifier "%p" always hashes the addresses that people do not have a
> choice to shut it down: https://patchwork.kernel.org/cover/11367547/
> 
> It's true that if in a debug environment or security is not concerned,
> such as KASLR is absent or kptr_restrict = 0,  there is no way to shut
> the hashing down except changing the code and build the kernel again
> to use a different format specifier like "%px". And when we want to
> turn to security environment, the format specifier has to be changed
> back and rebuild the kernel.
> 
> As KASLR is available on most popular platforms and enabled by default,
> print the raw value of address while KASLR is absent and kptr_restrict
> is zero. Those who concerns about security must have KASLR enabled or
> kptr_restrict set properly.

Even w/o KASLR the kernel address is sensitive material.
However, as a developer, I would like to have means to shut the hashing down.

Btw, when pass 'nokaslr' to the kernel it should turned off as well.

> +	/*
> +	 * In security environment, while kaslr is enabled or kptr_restrict is

kaslr -> KASLR

> +	 * not zero, hash before printing so that addresses will not be
> +	 * leaked. And if not in a security environment, print the raw value

Missed period at the end of sentence.

> +	 */
> +	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) || kptr_restrict)
> +		return ptr_to_id(buf, end, ptr, spec);
> +	else
> +		return pointer_string(buf, end, ptr, spec);
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] vfsprintf: only hash addresses in security environment

[Kees Cook]

On Wed, Mar 04, 2020 at 08:47:07PM +0800, Jason Yan wrote:
> When I am implementing KASLR for powerpc, Scott Wood argued that format
> specifier "%p" always hashes the addresses that people do not have a
> choice to shut it down: https://patchwork.kernel.org/cover/11367547/
> 
> It's true that if in a debug environment or security is not concerned,
> such as KASLR is absent or kptr_restrict = 0,  there is no way to shut
> the hashing down except changing the code and build the kernel again
> to use a different format specifier like "%px". And when we want to
> turn to security environment, the format specifier has to be changed
> back and rebuild the kernel.
> 
> As KASLR is available on most popular platforms and enabled by default,
> print the raw value of address while KASLR is absent and kptr_restrict
> is zero. Those who concerns about security must have KASLR enabled or
> kptr_restrict set properly.

Sorry, but no: %p censoring is there to stem the flood of endless pointer
leaks into logs, sysfs, proc, etc. These are used for attacks to build
reliable kernel memory targets, regardless of KASLR. (KASLR can be
bypassed with all kinds of sampling methods, side-channels, etc.)

Linus has rejected past suggestions to provide a flag for it[1],
wanting %p to stay unconditionally censored.

-Kees

[1] https://lore.kernel.org/lkml/CA+55aFwieC1-nAs+NFq9RTwaR8ef9hWa4MjNBWL41F-8wM49eA@mail.gmail.com/

> 
> Cc: Scott Wood <oss@buserror.net>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: "Tobin C . Harding" <tobin@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Daniel Axtens <dja@axtens.net>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
>  lib/vsprintf.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7c488a1ce318..f74131b152a1 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2253,8 +2253,15 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		return err_ptr(buf, end, ptr, spec);
>  	}
>  
> -	/* default is to _not_ leak addresses, hash before printing */
> -	return ptr_to_id(buf, end, ptr, spec);
> +	/*
> +	 * In security environment, while kaslr is enabled or kptr_restrict is
> +	 * not zero, hash before printing so that addresses will not be
> +	 * leaked. And if not in a security environment, print the raw value
> +	 */
> +	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) || kptr_restrict)
> +		return ptr_to_id(buf, end, ptr, spec);
> +	else
> +		return pointer_string(buf, end, ptr, spec);
>  }
>  
>  /*
> -- 
> 2.17.2
> 

-- 
Kees Cook

Re: [PATCH v3 0/6] implement KASLR for powerpc/fsl_booke/64

[Scott Wood]

On Wed, 2020-03-04 at 10:34 -0800, Kees Cook wrote:
> On Wed, Mar 04, 2020 at 08:47:07PM +0800, Jason Yan wrote:
> > When I am implementing KASLR for powerpc, Scott Wood argued that format
> > specifier "%p" always hashes the addresses that people do not have a
> > choice to shut it down: https://patchwork.kernel.org/cover/11367547/
> > 
> > It's true that if in a debug environment or security is not concerned,
> > such as KASLR is absent or kptr_restrict = 0,  there is no way to shut
> > the hashing down except changing the code and build the kernel again
> > to use a different format specifier like "%px". And when we want to
> > turn to security environment, the format specifier has to be changed
> > back and rebuild the kernel.
> > 
> > As KASLR is available on most popular platforms and enabled by default,
> > print the raw value of address while KASLR is absent and kptr_restrict
> > is zero. Those who concerns about security must have KASLR enabled or
> > kptr_restrict set properly.
> 
> Sorry, but no: %p censoring is there to stem the flood of endless pointer
> leaks into logs, sysfs, proc, etc. These are used for attacks to build
> reliable kernel memory targets, regardless of KASLR. (KASLR can be
> bypassed with all kinds of sampling methods, side-channels, etc.)
> 
> Linus has rejected past suggestions to provide a flag for it[1],
> wanting %p to stay unconditionally censored.

The frustration is with the inability to set a flag to say, "I'm debugging and
don't care about leaks... in fact I'd like as much information as possible to
leak to me."  Hashed addresses only allow comparison to other hashes which
doesn't help when looking at (or trying to find) data structures in kernel
memory, etc.  I get what Linus is saying about "you can't have nice things
because people won't test the normal config" but it's still annoying. :-P

In any case, this came up now due to a question about what to use when
printing crash dumps.  PowerPC currently prints stack and return addresses
with %lx (in addition to %pS in the latter case) and someone proposed
converting them to %p and/or removing them altogether.  Is there a consensus
on whether crash dumps need to be sanitized of this stuff as well?  It seems
like you'd have the addresses in the register dump as well (please don't take
that away too...).  Maybe crash dumps would be a less problematic place to
make the hashing conditional (i.e. less likely to break something in userspace
that wasn't expecting a hash)?

-Scott

Re: [PATCH v3 0/6] implement KASLR for powerpc/fsl_booke/64

[Kees Cook]

On Wed, Mar 04, 2020 at 03:11:39PM -0600, Scott Wood wrote:
> In any case, this came up now due to a question about what to use when
> printing crash dumps.  PowerPC currently prints stack and return addresses
> with %lx (in addition to %pS in the latter case) and someone proposed

Right -- I think other archs moved entirely to %pS and just removed %lx
and %p uses.

> converting them to %p and/or removing them altogether.  Is there a consensus
> on whether crash dumps need to be sanitized of this stuff as well?  It seems
> like you'd have the addresses in the register dump as well (please don't take
> that away too...).  Maybe crash dumps would be a less problematic place to
> make the hashing conditional (i.e. less likely to break something in userspace
> that wasn't expecting a hash)?

Actual _crash_ dumps print all kinds of stuff, even the KASLR offset,
but for generic stack traces, it's been mainly %pS, with things like
registers using %lx.

I defer to Linus, obviously. I just wanted to repeat what he'd said
before.

-- 
Kees Cook

Re: [PATCH v3 0/6] implement KASLR for powerpc/fsl_booke/64

[Linus Torvalds]

On Wed, Mar 4, 2020 at 3:16 PM Scott Wood <oss@buserror.net> wrote:
>
> The frustration is with the inability to set a flag to say, "I'm debugging and
> don't care about leaks... in fact I'd like as much information as possible to
> leak to me."

Well, I definitely don't want to tie it to "I turned off kaslr in
order to help debugging". That just means that now you're debugging a
kernel that is fundamentally different from what people are running.

So I'd much rather have people just set a really magic flag, perhaps
when kgdb is in use or something.

> In any case, this came up now due to a question about what to use when
> printing crash dumps.  PowerPC currently prints stack and return addresses
> with %lx (in addition to %pS in the latter case) and someone proposed
> converting them to %p and/or removing them altogether.

Please just use '%pS'.

The symbol and offset is what is useful when users send crash-dumps.
The hex value is entirely pointless with kaslr - which should
basically be the default.

Note that this isn't about security at that point - crash dumps are
something that shouldn't happen, but if they do happen, we want the
pointers. But the random hex value just isn't _useful_, so it's just
making things less legible.

                  Linus

Re: [PATCH v3 0/6] implement KASLR for powerpc/fsl_booke/64

[Scott Wood]

On Thu, 2020-03-05 at 12:51 -0600, Linus Torvalds wrote:
> On Wed, Mar 4, 2020 at 3:16 PM Scott Wood <oss@buserror.net> wrote:
> > 
> > The frustration is with the inability to set a flag to say, "I'm debugging
> > and
> > don't care about leaks... in fact I'd like as much information as possible
> > to
> > leak to me."
> 
> Well, I definitely don't want to tie it to "I turned off kaslr in
> order to help debugging". That just means that now you're debugging a
> kernel that is fundamentally different from what people are running.

One shouldn't *test* with something different from what people are running but
once a problem has been identified I don't see the problem with changing the
kernel to make diagnosis easier (assuming the problem is reproduceable). 
Though I suppose one could just locally apply a "no pointer hashing" patch
when debugging...

> So I'd much rather have people just set a really magic flag, perhaps
> when kgdb is in use or something.
> 
> > In any case, this came up now due to a question about what to use when
> > printing crash dumps.  PowerPC currently prints stack and return addresses
> > with %lx (in addition to %pS in the latter case) and someone proposed
> > converting them to %p and/or removing them altogether.
> 
> Please just use '%pS'.
> 
> The symbol and offset is what is useful when users send crash-dumps.
> The hex value is entirely pointless with kaslr - which should
> basically be the default.
> 
> Note that this isn't about security at that point - crash dumps are
> something that shouldn't happen, but if they do happen, we want the
> pointers. But the random hex value just isn't _useful_, so it's just
> making things less legible.

Losing %lx on the return address would be a minor annoyance (harder to verify
that you're looking at the right stack frame in a dump, more steps to look up
the line number when modules and kaslr aren't involved, etc), but %pS doesn't
help with stack addresses themselves -- and yes, digging into the actual stack
data (via kdump, external debugger, etc.) is sometimes useful.  Maybe
condition it on it being an actual crash dump and not some other caller of
show_stack()?

-Scott