[PATCH v4] kptr_restrict for hiding kernel pointers

[PATCH v4] kptr_restrict for hiding kernel pointers

[Dan Rosenberg]

Add the %pK printk format specifier and
the /proc/sys/kernel/kptr_restrict sysctl.

The %pK format specifier is designed to hide exposed kernel pointers,
specifically via /proc interfaces.  Exposing these pointers provides an
easy target for kernel write vulnerabilities, since they reveal the
locations of writable structures containing easily triggerable function
pointers.  The behavior of %pK depends on the kptr_restrict sysctl.

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.  If kptr_restrict is set to 2, kernel pointers using %pK are
printed as 0's regardless of privileges.  Replacing with 0's was chosen
over the default "(null)", which cannot be parsed by userland %p, which
expects "(nil)".


v4 incorporates Eric Paris' suggestion of using
has_capability_noaudit(), since failing this capability check is not a
policy violation but rather a code path choice and shouldn't generate
potentially excessive log noise.  Adjusted IRQ comment for clarity.

v3 adds the "2" setting, cleans up documentation, removes the CONFIG,
and incorporates changes and suggestions from Andrew Morton.

v2 improves checking for inappropriate context, on suggestion by Peter
Zijlstra.  Thanks to Thomas Graf for suggesting use of a centralized
format specifier.


Signed-off-by: Dan Rosenberg <drosenberg@vsecurity.com>
Cc: James Morris <jmorris@namei.org>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Thomas Graf <tgraf@infradead.org>
Cc: Eugene Teo <eugeneteo@kernel.org>
Cc: Kees Cook <kees.cook@canonical.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: David S. Miller <davem@davemloft.net>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Paris <eparis@parisplace.org>
---

 Documentation/sysctl/kernel.txt |   14 ++++++++++++++
 include/linux/kernel.h          |    2 ++
 kernel/sysctl.c                 |    9 +++++++++
 lib/vsprintf.c                  |   24 ++++++++++++++++++++++++
 4 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
index 209e158..3cff47e 100644
--- a/Documentation/sysctl/kernel.txt
+++ b/Documentation/sysctl/kernel.txt
@@ -34,6 +34,7 @@ show up in /proc/sys/kernel:
 - hotplug
 - java-appletviewer           [ binfmt_java, obsolete ]
 - java-interpreter            [ binfmt_java, obsolete ]
+- kptr_restrict
 - kstack_depth_to_print       [ X86 only ]
 - l2cr                        [ PPC only ]
 - modprobe                    ==> Documentation/debugging-modules.txt
@@ -261,6 +262,19 @@ This flag controls the L2 cache of G3 processor boards. If
 
 ==============================================================
 
+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), the default, there are no
+restrictions.  When kptr_restrict is set to (1), 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.
+
+==============================================================
+
 kstack_depth_to_print: (X86 only)
 
 Controls the number of words to print when dumping the raw
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index b6de9a6..b4f4863 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -201,6 +201,8 @@ extern int sscanf(const char *, const char *, ...)
 extern int vsscanf(const char *, const char *, va_list)
 	__attribute__ ((format (scanf, 2, 0)));
 
+extern int kptr_restrict;	/* for sysctl */
+
 extern int get_option(char **str, int *pint);
 extern char *get_options(const char *str, int nints, int *ints);
 extern unsigned long long memparse(const char *ptr, char **retptr);
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 5abfa15..236fa91 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -713,6 +713,15 @@ static struct ctl_table kern_table[] = {
 	},
 #endif
 	{
+		.procname	= "kptr_restrict",
+		.data		= &kptr_restrict,
+		.maxlen		= sizeof(int),
+		.mode		= 0644,
+		.proc_handler	= proc_dointvec_minmax,
+		.extra1		= &zero,
+		.extra2		= &two,
+	},
+	{
 		.procname	= "ngroups_max",
 		.data		= &ngroups_max,
 		.maxlen		= sizeof (int),
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index c150d3d..313f767 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;
+
 /*
  * 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,27 @@ 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':
+		/*
+		 * %pK cannot be used in IRQ context because its test
+		 * for CAP_SYSLOG would be meaningless.
+		 */
+		if (in_irq() || in_serving_softirq() || in_nmi())
+			WARN_ONCE(1, "%%pK used in interrupt context.\n");
+
+		if (!kptr_restrict)
+			break;		/* %pK does not obscure pointers */
+
+		if ((kptr_restrict != 2) &&
+		    has_capability_noaudit(current, CAP_SYSLOG))
+			break;		/* privileged apps expose pointers,
+					   unless kptr_restrict is 2 */
+
+		if (spec.field_width == -1) {
+			spec.field_width = 2 * sizeof(void *);
+			spec.flags |= ZEROPAD;
+		}
+		return number(buf, end, 0, spec);
 	}
 	spec.flags |= SMALL;
 	if (spec.field_width == -1) {

Re: [PATCH v4] kptr_restrict for hiding kernel pointers

[Ingo Molnar]

* Dan Rosenberg <drosenberg@vsecurity.com> wrote:

> +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), the default, there are no
> +restrictions.  When kptr_restrict is set to (1), 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.

Hm, why is it off by default? Is there some user-space regression that is caused by 
this?

We really want good security measures to be active by default (and to work by 
default) - they are not worth much if they are not.

Thanks,

	Ingo

Re: [PATCH v4] kptr_restrict for hiding kernel pointers

[Dan Rosenberg]

> Hm, why is it off by default? Is there some user-space regression that is caused by 
> this?
> 
> We really want good security measures to be active by default (and to work by 
> default) - they are not worth much if they are not.
> 

I agree entirely, but I've received a lot of resistance to these types
of changes in net.  I'm afraid that if it's enabled by default, no one
will actually allow use of the %pK specifier where it should be used.

As far as I know, there's no actual breakage of anything in userspace,
but there's a general "it might make it harder to debug things in
certain limited circumstances" sentiment among some.  I never understood
why it is necessary for unprivileged users to be able to debug the
kernel.

Does anyone else have thoughts on this?

> Thanks,
> 
> 	Ingo

Re: [PATCH v4] kptr_restrict for hiding kernel pointers

[Eric Dumazet]

Le mercredi 22 décembre 2010 à 08:13 -0500, Dan Rosenberg a écrit :
> > Hm, why is it off by default? Is there some user-space regression that is caused by 
> > this?
> > 
> > We really want good security measures to be active by default (and to work by 
> > default) - they are not worth much if they are not.
> > 
> 
> I agree entirely, but I've received a lot of resistance to these types
> of changes in net.  I'm afraid that if it's enabled by default, no one
> will actually allow use of the %pK specifier where it should be used.
> 

Actually, "net resistance" was against your first patches, using quick
and dirty techniques (Should I remind you some of them ?)

Now you have a helper, it should be easier to integrate the changes.

At least, if a mission critical legacy app want to see real pointers
values and a 2.6.38 kernel, it is a matter of sysadmin tweaks.



--
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 v4] kptr_restrict for hiding kernel pointers

[Ingo Molnar]

* Dan Rosenberg <drosenberg@vsecurity.com> wrote:

> 
> > Hm, why is it off by default? Is there some user-space regression that is caused 
> > by this?
> > 
> > We really want good security measures to be active by default (and to work by 
> > default) - they are not worth much if they are not.
> 
> I agree entirely, but I've received a lot of resistance to these types
> of changes in net.  I'm afraid that if it's enabled by default, no one
> will actually allow use of the %pK specifier where it should be used.

Some specific objections would be needed - which might arrive if the default is 
changed to on.

> As far as I know, there's no actual breakage of anything in userspace,
> but there's a general "it might make it harder to debug things in
> certain limited circumstances" sentiment among some.  I never understood
> why it is necessary for unprivileged users to be able to debug the
> kernel.
> 
> Does anyone else have thoughts on this?

Well, lets just enable it by default and let others argue for less security, hm?

Thanks,

	Ingo

Re: [PATCH v4] kptr_restrict for hiding kernel pointers

[Ingo Molnar]

* Eric Dumazet <eric.dumazet@gmail.com> wrote:

> Le mercredi 22 décembre 2010 à 08:13 -0500, Dan Rosenberg a écrit :
> > > Hm, why is it off by default? Is there some user-space regression that is caused by 
> > > this?
> > > 
> > > We really want good security measures to be active by default (and to work by 
> > > default) - they are not worth much if they are not.
> > > 
> > 
> > I agree entirely, but I've received a lot of resistance to these types
> > of changes in net.  I'm afraid that if it's enabled by default, no one
> > will actually allow use of the %pK specifier where it should be used.
> > 
> 
> Actually, "net resistance" was against your first patches, using quick
> and dirty techniques (Should I remind you some of them ?)
> 
> Now you have a helper, it should be easier to integrate the changes.

Great - if the concept itself wasnt objected to then i think we should flip the 
default to on.

Thanks,

	Ingo

Re: [PATCH v4] kptr_restrict for hiding kernel pointers

[Andrew Morton]

On Wed, 22 Dec 2010 17:21:18 +0100
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > Le mercredi 22 d__cembre 2010 __ 08:13 -0500, Dan Rosenberg a __crit :
> > > > Hm, why is it off by default? Is there some user-space regression that is caused by 
> > > > this?
> > > > 
> > > > We really want good security measures to be active by default (and to work by 
> > > > default) - they are not worth much if they are not.
> > > > 
> > > 
> > > I agree entirely, but I've received a lot of resistance to these types
> > > of changes in net.  I'm afraid that if it's enabled by default, no one
> > > will actually allow use of the %pK specifier where it should be used.
> > > 
> > 
> > Actually, "net resistance" was against your first patches, using quick
> > and dirty techniques (Should I remind you some of them ?)
> > 
> > Now you have a helper, it should be easier to integrate the changes.
> 
> Great - if the concept itself wasnt objected to then i think we should flip the 
> default to on.
> 

Yes, I'll make that change.  If we get reports of breakage during
2.6.38-rcX then we can reconsider.  But if we leave the feature
disabled, we will never know...