Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

[Eric Dumazet]

Le mercredi 08 décembre 2010 à 22:05 -0500, Dan Rosenberg a écrit :
> The below patch adds the %pK format specifier, the
> CONFIG_SECURITY_KPTR_RESTRICT configuration option, and the
> kptr_restrict sysctl.
> 
> The %pK format specifier is designed to hide exposed kernel pointers
> from unprivileged users, specifically via /proc interfaces.  Its
> behavior depends on the kptr_restrict sysctl, whose default value
> depends on CONFIG_SECURITY_KPTR_RESTRICT.  If kptr_restrict is set to 0,
> no deviation from the standard %p behavior occurs.  If kptr_restrict is
> set to 1, if the current user (intended to be a reader via seq_printf(),
> etc.) does not have CAP_SYSLOG (which is currently in the LSM tree),
> kernel pointers using %pK are printed as 0's.  This was chosen over the
> default "(null)", which cannot be parsed by userland %p, which expects
> "(nil)".
> 

Thanks for not giving credits to people suggesting this idea to you
(Thomas if I remember well), and not Ccing netdev where original
discussion took place. 


> Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
> --- 
> Documentation/sysctl/kernel.txt |   14 ++++++++++++++
>  include/linux/kernel.h          |    2 ++
>  kernel/sysctl.c                 |    9 +++++++++
>  lib/vsprintf.c                  |   18 ++++++++++++++++++
>  security/Kconfig                |   12 ++++++++++++
>  5 files changed, 55 insertions(+), 0 deletions(-)

...

> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index c150d3d..c011249 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -936,6 +936,8 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>  	return string(buf, end, uuid, spec);
>  }
>  
> +int kptr_restrict = CONFIG_SECURITY_KPTR_RESTRICT;
> +
>  /*
>   * Show a '%p' thing.  A kernel extension is that the '%p' is followed
>   * by an extra set of alphanumeric characters that are extended format
> @@ -979,6 +981,7 @@ char *uuid_string(char *buf, char *end, const u8 *addr,
>   *       Implements a "recursive vsnprintf".
>   *       Do not use this feature without some mechanism to verify the
>   *       correctness of the format string and va_list arguments.
> + * - 'K' For a kernel pointer that should be hidden from unprivileged users
>   *
>   * Note: The difference between 'S' and 'F' is that on ia64 and ppc64
>   * function pointers are really function descriptors, which contain a
> @@ -1035,6 +1038,21 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		return buf + vsnprintf(buf, end - buf,
>  				       ((struct va_format *)ptr)->fmt,
>  				       *(((struct va_format *)ptr)->va));
> +	case 'K':
> +		if (kptr_restrict) {
> +			if (in_interrupt())
> +				WARN(1, "%%pK used in interrupt context.\n");

So caller can not block BH ?

This seems wrong to me, please consider :

normal process context :

spin_lock_bh() ...

for (...)  
	{xxx}printf( ...   "%pK"   ...)

spin_unlock_bh();


> +
> +			else if (capable(CAP_SYSLOG))
> +				break;
> +
> +			if (spec.field_width == -1) {
> +				spec.field_width = 2 * sizeof(void *);
> +				spec.flags |= ZEROPAD;
> +			}
> +			return number(buf, end, 0, spec);
> +		}
> +		break;
>  	}
>  	spec.flags |= SMALL;
>  	if (spec.field_width == -1) {
> diff --git a/security/Kconfig b/security/Kconfig
> index e80da95..944fc73 100644
> --- a/security/Kconfig
> +++ b/security/Kconfig
> @@ -51,6 +51,18 @@ config SECURITY_DMESG_RESTRICT
>  
>  	  If you are unsure how to answer this question, answer N.
>  
> +config SECURITY_KPTR_RESTRICT
> +	bool "Hide kernel pointers from unprivileged users"
> +	default n
> +	help
> +	  This enforces restrictions on unprivileged users reading kernel
> +	  addresses via various interfaces, e.g. /proc.
> +
> +	  If this option is not selected, no restrictions will be enforced
> +	  unless the kptr_restrict sysctl is explicitly set to (1).
> +
> +	  If you are unsure how to answer this question, answer N.
> +
>  config SECURITY
>  	bool "Enable different security models"
>  	depends on SYSFS
> 

Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

[Eric Dumazet]

Le jeudi 09 décembre 2010 à 04:23 +0100, Eric Dumazet a écrit :

> Thanks for not giving credits to people suggesting this idea to you
> (Thomas if I remember well), and not Ccing netdev where original
> discussion took place. 

Yes, credits should be given to Thomas Graf

http://www.spinics.net/lists/netdev/msg146606.html

Thanks

Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

[Dan Rosenberg]

> 
> Thanks for not giving credits to people suggesting this idea to you
> (Thomas if I remember well), and not Ccing netdev where original
> discussion took place. 
> 

I am happy to credit Thomas, even though he is far from the first person
to have suggested this approach to me.  Thanks for the suggestion.

> 
> So caller can not block BH ?
> 
> This seems wrong to me, please consider :
> 
> normal process context :
> 
> spin_lock_bh() ...
> 
> for (...)  
> 	{xxx}printf( ...   "%pK"   ...)
> 
> spin_unlock_bh();
> 

I will think about this and address it.

-Dan

Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

[Dan Rosenberg]

> > So caller can not block BH ?
> > 
> > This seems wrong to me, please consider :
> > 
> > normal process context :
> > 
> > spin_lock_bh() ...
> > 
> > for (...)  
> > 	{xxx}printf( ...   "%pK"   ...)
> > 
> > spin_unlock_bh();
> > 
> 
> I will think about this and address it.

Would you be happier if I omitted the in_interrupt() check entirely?

-Dan

Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

[Eric Dumazet]

Le jeudi 09 décembre 2010 à 07:46 -0500, Dan Rosenberg a écrit :
> > > So caller can not block BH ?
> > > 
> > > This seems wrong to me, please consider :
> > > 
> > > normal process context :
> > > 
> > > spin_lock_bh() ...
> > > 
> > > for (...)  
> > > 	{xxx}printf( ...   "%pK"   ...)
> > > 
> > > spin_unlock_bh();
> > > 
> > 
> > I will think about this and address it.
> 
> Would you be happier if I omitted the in_interrupt() check entirely?
> 

Well, it seems difficult to make a check here, its a generic function
that happens to be used from different contexts.

Even using in_irq() might be a problem.



--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

[Dan Rosenberg]

> 
> Well, it seems difficult to make a check here, its a generic function
> that happens to be used from different contexts.
> 
> Even using in_irq() might be a problem.

I agree it seems difficult - my only goal was to prevent subsequent
breakage with the capability check.  Does anyone have any suggestions
for a better approach here?

-Dan

Re: [PATCH] kptr_restrict for hiding kernel pointers from unprivileged users

[Peter Zijlstra]

On Thu, 2010-12-09 at 04:23 +0100, Eric Dumazet wrote:
> > +             if (kptr_restrict) {
> > +                     if (in_interrupt())
> > +                             WARN(1, "%%pK used in interrupt context.\n");
> 
> So caller can not block BH ?
> 
> This seems wrong to me, please consider :
> 
> normal process context :
> 
> spin_lock_bh() ...
> 
> for (...)  
>         {xxx}printf( ...   "%pK"   ...)
> 
> spin_unlock_bh(); 

That's a bug in in_interrupt(), one I've been pointing out for a long
while. Luckily we recently grew the infrastructure to deal with it.

If you write it as: if (in_irq() || in_serving_softirq() || in_nmi())
you'll not trigger for the above example.

Ideally in_serving_softirq() wouldn't exist and in_softirq() would do
what in_server_softirq() does -- which would make it symmetric with the
hardirq functions -- but nobody has found time to audit all in_softirq()
users.