[PATCH 0/9] Fix leaking of kernel heap addresses in net/

[PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Dan Rosenberg]

This patch series resolves the leakage of kernel heap addresses to
userspace via network protocol /proc interfaces and public error
messages.  Revealing this information is a bad idea from a security
perspective for a number of reasons, the most obvious of which is it
provides unprivileged users a mechanism by which to create a structure
in the kernel heap containing function pointers, obtain the address of
that structure, and overwrite those function pointers by leveraging
other vulnerabilities.  It is my hope that by eliminating this
information leakage, in conjunction with making statically-declared
function pointer tables read-only (to be done in a separate patch
series), we can at least add a small hurdle for the exploitation of a
subset of kernel vulnerabilities.

To maintain compatibility with userspace programs relying on
consistent /proc output, the output descriptions and number of fields
are not changed.  When a unique identifier for the socket is desired,
the socket address has been replaced with the socket inode number.  When
the inode number is already present in the output, the address has been
replaced with a 0.  In these cases, the format specifier has been
changed to %d, because a %p output of 0 from kernel space is written as
"(null)", while userspace %p can only parse "(nil)".

-Dan

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Eric Dumazet]

Le dimanche 07 novembre 2010 à 11:31 -0500, Dan Rosenberg a écrit :
> This patch series resolves the leakage of kernel heap addresses to
> userspace via network protocol /proc interfaces and public error
> messages.  Revealing this information is a bad idea from a security
> perspective for a number of reasons, the most obvious of which is it
> provides unprivileged users a mechanism by which to create a structure
> in the kernel heap containing function pointers, obtain the address of
> that structure, and overwrite those function pointers by leveraging
> other vulnerabilities.  It is my hope that by eliminating this
> information leakage, in conjunction with making statically-declared
> function pointer tables read-only (to be done in a separate patch
> series), we can at least add a small hurdle for the exploitation of a
> subset of kernel vulnerabilities.
> 
> To maintain compatibility with userspace programs relying on
> consistent /proc output, the output descriptions and number of fields
> are not changed.  When a unique identifier for the socket is desired,
> the socket address has been replaced with the socket inode number.  When
> the inode number is already present in the output, the address has been
> replaced with a 0.  In these cases, the format specifier has been
> changed to %d, because a %p output of 0 from kernel space is written as
> "(null)", while userspace %p can only parse "(nil)".
> 

NACK

Thats a pretty stupid patch series, sorry.

You are basically ruining a lot of debugging facilities we use every day
to find and fix _real_ bugs. The bugs that happen to crash machines of
our customers.

If you want to avoid a user reading kernel syslog, why dont you fix the
problem for non root users able to "dmesg" ? I personally dont care.

I am a root user on my machine, I _want_ to have some pretty basic
informations so that I can work on it, and I believe my work is useful.

There are pretty easy ways to not disclose "information", but your way
of using '0' for all values is the dumbest idea one could ever had.

A single XOR with a "root only visible, random value chosen at boot"
would be OK. At least we could continue our work, with litle burden.

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Dan Rosenberg]

> NACK
> 
> Thats a pretty stupid patch series, sorry.
> 

I think it might be more constructive to avoid childish name-calling and
instead try to guide the conversation in a way that produces a patch
that would better fit your needs.  Even if you don't agree with the
approach, it's certainly not "stupid".

> You are basically ruining a lot of debugging facilities we use every day
> to find and fix _real_ bugs. The bugs that happen to crash machines of
> our customers.

I'm going to give you the benefit of the doubt and assume you're not
implying that security issues aren't "real" bugs, because that would be
utterly ridiculous.

> 
> If you want to avoid a user reading kernel syslog, why dont you fix the
> problem for non root users able to "dmesg" ? I personally dont care.
> 

This is simply the reality of the current situation.  At least while the
kernel syslog is available to unprivileged users, we need to be more
careful of what is visible through there.

> I am a root user on my machine, I _want_ to have some pretty basic
> informations so that I can work on it, and I believe my work is useful.
> 
> There are pretty easy ways to not disclose "information", but your way
> of using '0' for all values is the dumbest idea one could ever had.

I'm glad I'm capable of producing "the dumbest idea one could ever had".
You seem to be quite set on convincing unpaid volunteers such as myself
to stop sending in patches.

> 
> A single XOR with a "root only visible, random value chosen at boot"
> would be OK. At least we could continue our work, with litle burden.

Finally, a useful contribution.  I'll consider this option after hearing
from a few more people on the subject.

-Dan

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Eric Dumazet]

Le dimanche 07 novembre 2010 à 12:25 -0500, Dan Rosenberg a écrit :
> > NACK
> > 
> > Thats a pretty stupid patch series, sorry.
> > 
> 
> I think it might be more constructive to avoid childish name-calling and
> instead try to guide the conversation in a way that produces a patch
> that would better fit your needs.  Even if you don't agree with the
> approach, it's certainly not "stupid".
> 

It is stupid. Really Dan. The idea is stupid, not you.

> > You are basically ruining a lot of debugging facilities we use every day
> > to find and fix _real_ bugs. The bugs that happen to crash machines of
> > our customers.
> 
> I'm going to give you the benefit of the doubt and assume you're not
> implying that security issues aren't "real" bugs, because that would be
> utterly ridiculous.
> 

So what ? Because of security, we must accept even stupid patches ?

> > 
> > If you want to avoid a user reading kernel syslog, why dont you fix the
> > problem for non root users able to "dmesg" ? I personally dont care.
> > 
> 
> This is simply the reality of the current situation.  At least while the
> kernel syslog is available to unprivileged users, we need to be more
> careful of what is visible through there.
> 

So instead of fixing the problem, you are going to change thousand of
kernel printk() ?

> > I am a root user on my machine, I _want_ to have some pretty basic
> > informations so that I can work on it, and I believe my work is useful.
> > 
> > There are pretty easy ways to not disclose "information", but your way
> > of using '0' for all values is the dumbest idea one could ever had.
> 
> I'm glad I'm capable of producing "the dumbest idea one could ever had".
> You seem to be quite set on convincing unpaid volunteers such as myself
> to stop sending in patches.
> 

I am unpaid volunteer too.

I also had stupid ideas, and other guys said so.

So what ? Should I continue contributing to Linux, or assume I am stupid
and stop ?

> > 
> > A single XOR with a "root only visible, random value chosen at boot"
> > would be OK. At least we could continue our work, with litle burden.
> 
> Finally, a useful contribution.  I'll consider this option after hearing
> from a few more people on the subject.

I am glad you like it. But it also may a _very_ stupid idea. You really
want to have a _lot_ of agreement before even considering it.

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Urs Thuermann]

Eric Dumazet <eric.dumazet@gmail.com> writes:

> You are basically ruining a lot of debugging facilities we use every
> day to find and fix _real_ bugs. The bugs that happen to crash
> machines of our customers.
> 
> If you want to avoid a user reading kernel syslog, why dont you fix
> the problem for non root users able to "dmesg" ? I personally dont
> care.

dmesg and syslog aren't the only problem.  There are files in
/proc/net/ that must be readable by all users in order for a number of
tools to work, e.g. netstat(8), and those files contain kernel
addresses of sock structs.

> There are pretty easy ways to not disclose "information", but your
> way of using '0' for all values is the dumbest idea one could ever
> had.
> 
> A single XOR with a "root only visible, random value chosen at boot"
> would be OK. At least we could continue our work, with litle burden.

Simple XOR with a secret constant doesn't help very much.  Many bits
of that hidden constant can be revealed quite easily.  On x86-32 for
example all addresses are in the high 1 GB address space so the
highest two address bits are 1.  Many structs of type sock are aligned
to 64 or higher powers of two so you can easily infer the lower 6 or
more bits of that hidden XOR constant.  If you know two structs of
alignment 2^n are contiguous in memory you can infer the lower n+1
bits.

I suggest either one of the following two solutions:

1. Make the contents of the proc-files in question dependent on some
   capability, e.g. CAP_SYS_ADMIN or some new CAP_KERNEL_DEBUG.  If
   the process opening the file owns that capability it will see the
   kernel address, otherwise it will see 0.

2. Replace the kernel address by the socket inode number or some other
   identifying value (as our patch for CAN does) and provide some
   mechanism for root only to translate that value to the
   corresponding kernel address.


urs

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Rémi Denis-Courmont]

On Sunday 07 November 2010 18:31:32 ext Dan Rosenberg, you wrote:
> This patch series resolves the leakage of kernel heap addresses to
> userspace via network protocol /proc interfaces and public error
> messages.  Revealing this information is a bad idea from a security
> perspective for a number of reasons, the most obvious of which is it
> provides unprivileged users a mechanism by which to create a structure
> in the kernel heap containing function pointers, obtain the address of
> that structure, and overwrite those function pointers by leveraging
> other vulnerabilities.  It is my hope that by eliminating this
> information leakage, in conjunction with making statically-declared
> function pointer tables read-only (to be done in a separate patch
> series), we can at least add a small hurdle for the exploitation of a
> subset of kernel vulnerabilities.

Seems like this patch series is incomplete to me as far as /proc/net is 
concerned.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Dan Rosenberg]

> Seems like this patch series is incomplete to me as far as /proc/net is 
> concerned.
> 

I'm aware that I missed SCTP, which I will fix in the next attempt.
Other than that (and sticking only to /proc leaks, since I realize there
are many printk examples), care to share what was missed?

-Dan

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Rémi Denis-Courmont]

On Monday 08 November 2010 15:13:19 ext Dan Rosenberg, you wrote:
> > Seems like this patch series is incomplete to me as far as /proc/net is
> > concerned.
> 
> I'm aware that I missed SCTP, which I will fix in the next attempt.
> Other than that (and sticking only to /proc leaks, since I realize there
> are many printk examples), care to share what was missed?

At least Phonet seems to be missing too... which makes me wonder why I am in 
the Cc list.

-- 
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki

Re: [PATCH 0/9] Fix leaking of kernel heap addresses in net/

[Dan Rosenberg]

> 
> At least Phonet seems to be missing too... which makes me wonder why I am in 
> the Cc list.

I have a patch prepared for Phonet - it seems to have been lost in the
shuffle when I broke up the patch set.  I'll be sure to include it in
the next revision.  Sorry about that.

-Dan