[PATCH] getsockopt() early argument sanity checking

[PATCH] getsockopt() early argument sanity checking

[Solar Designer]

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

Willy,

I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
into 2.4.34-pre.

(2.6 kernels could benefit from the same change, too, but at the moment
I am dealing with proper submission of generic changes like this that
are a part of 2.4.33-ow1.)

The patch makes getsockopt(2) sanity-check the value pointed to by
the optlen argument early on.  This is a security hardening measure
intended to prevent exploitation of certain potential vulnerabilities in
socket type specific getsockopt() code on UP systems.

This change has been a part of -ow patches for some years.

Thanks,

-- 
Alexander Peslyak <solar at openwall.com>
GPG key ID: B35D3598  fp: 6429 0D7E F130 C13E C929  6447 73C3 A290 B35D 3598
http://www.openwall.com - bringing security into open computing environments

[-- Attachment #2: linux-2.4.33-ow1-getsockopt-early-arg-sanity.diff --]
[-- Type: text/plain, Size: 686 bytes --]

diff -urpPX nopatch linux-2.4.33/net/socket.c linux/net/socket.c
--- linux-2.4.33/net/socket.c	Wed Jan 19 17:10:14 2005
+++ linux/net/socket.c	Sat Aug 12 08:51:47 2006
@@ -1307,10 +1307,18 @@ asmlinkage long sys_setsockopt(int fd, i
 asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval, int *optlen)
 {
 	int err;
+	int len;
 	struct socket *sock;
 
 	if ((sock = sockfd_lookup(fd, &err))!=NULL)
 	{
+		/* XXX: insufficient for SMP, but should be redundant anyway */
+		if (get_user(len, optlen))
+			err = -EFAULT;
+		else
+		if (len < 0)
+			err = -EINVAL;
+		else
 		if (level == SOL_SOCKET)
 			err=sock_getsockopt(sock,level,optname,optval,optlen);
 		else

Re: [PATCH] getsockopt() early argument sanity checking

[Willy Tarreau]

On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
> Willy,
> 
> I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
> into 2.4.34-pre.
> 
> (2.6 kernels could benefit from the same change, too, but at the moment
> I am dealing with proper submission of generic changes like this that
> are a part of 2.4.33-ow1.)
> 
> The patch makes getsockopt(2) sanity-check the value pointed to by
> the optlen argument early on.  This is a security hardening measure
> intended to prevent exploitation of certain potential vulnerabilities in
> socket type specific getsockopt() code on UP systems.
> 
> This change has been a part of -ow patches for some years.

looks valid to me, merged.

Thanks Alexander !
Willy


> Thanks,
> 
> -- 
> Alexander Peslyak <solar at openwall.com>
> GPG key ID: B35D3598  fp: 6429 0D7E F130 C13E C929  6447 73C3 A290 B35D 3598
> http://www.openwall.com - bringing security into open computing environments

> diff -urpPX nopatch linux-2.4.33/net/socket.c linux/net/socket.c
> --- linux-2.4.33/net/socket.c	Wed Jan 19 17:10:14 2005
> +++ linux/net/socket.c	Sat Aug 12 08:51:47 2006
> @@ -1307,10 +1307,18 @@ asmlinkage long sys_setsockopt(int fd, i
>  asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval, int *optlen)
>  {
>  	int err;
> +	int len;
>  	struct socket *sock;
>  
>  	if ((sock = sockfd_lookup(fd, &err))!=NULL)
>  	{
> +		/* XXX: insufficient for SMP, but should be redundant anyway */
> +		if (get_user(len, optlen))
> +			err = -EFAULT;
> +		else
> +		if (len < 0)
> +			err = -EINVAL;
> +		else
>  		if (level == SOL_SOCKET)
>  			err=sock_getsockopt(sock,level,optname,optval,optlen);
>  		else

Re: [PATCH] getsockopt() early argument sanity checking

[Michael Buesch]

On Sunday 20 August 2006 01:48, Willy Tarreau wrote:
> On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
> > Willy,
> > 
> > I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
> > into 2.4.34-pre.
> > 
> > (2.6 kernels could benefit from the same change, too, but at the moment
> > I am dealing with proper submission of generic changes like this that
> > are a part of 2.4.33-ow1.)
> > 
> > The patch makes getsockopt(2) sanity-check the value pointed to by
> > the optlen argument early on.  This is a security hardening measure
> > intended to prevent exploitation of certain potential vulnerabilities in
> > socket type specific getsockopt() code on UP systems.
> > 
> > This change has been a part of -ow patches for some years.
> 
> looks valid to me, merged.

Not to me. It heavily violates codingstyle and screws brains
with the non-indented else branches. Learn about goto.

> Thanks Alexander !
> Willy
> 
> 
> > Thanks,
> > 
> > -- 
> > Alexander Peslyak <solar at openwall.com>
> > GPG key ID: B35D3598  fp: 6429 0D7E F130 C13E C929  6447 73C3 A290 B35D 3598
> > http://www.openwall.com - bringing security into open computing environments
> 
> > diff -urpPX nopatch linux-2.4.33/net/socket.c linux/net/socket.c
> > --- linux-2.4.33/net/socket.c	Wed Jan 19 17:10:14 2005
> > +++ linux/net/socket.c	Sat Aug 12 08:51:47 2006
> > @@ -1307,10 +1307,18 @@ asmlinkage long sys_setsockopt(int fd, i
> >  asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval, int *optlen)
> >  {
> >  	int err;
> > +	int len;
> >  	struct socket *sock;
> >  
> >  	if ((sock = sockfd_lookup(fd, &err))!=NULL)
> >  	{
> > +		/* XXX: insufficient for SMP, but should be redundant anyway */
> > +		if (get_user(len, optlen))
> > +			err = -EFAULT;
> > +		else
> > +		if (len < 0)
> > +			err = -EINVAL;
> > +		else
> >  		if (level == SOL_SOCKET)
> >  			err=sock_getsockopt(sock,level,optname,optval,optlen);
> >  		else
> 
> -
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Greetings Michael.

Re: [PATCH] getsockopt() early argument sanity checking

[Willy Tarreau]

On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
> On Sunday 20 August 2006 01:48, Willy Tarreau wrote:
> > On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
> > > Willy,
> > > 
> > > I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
> > > into 2.4.34-pre.
> > > 
> > > (2.6 kernels could benefit from the same change, too, but at the moment
> > > I am dealing with proper submission of generic changes like this that
> > > are a part of 2.4.33-ow1.)
> > > 
> > > The patch makes getsockopt(2) sanity-check the value pointed to by
> > > the optlen argument early on.  This is a security hardening measure
> > > intended to prevent exploitation of certain potential vulnerabilities in
> > > socket type specific getsockopt() code on UP systems.
> > > 
> > > This change has been a part of -ow patches for some years.
> > 
> > looks valid to me, merged.
> 
> Not to me. It heavily violates codingstyle and screws brains
                ^^^^^^^
little exageration detected here.

> with the non-indented else branches.

while they surprized me first, they make the *patch* more readable
by clearly showing what has been inserted and where. However, I have
joined the lines for the merge.

> Learn about goto.

definitely not here. The if() expressions are all one-liners. Adding
a goto would mean two instructions, to which you add 2 braces. It will
not make the code more readable. Patch below is OK. If you have a hard
time understanding it, then it's because it's bedtime for you too :-)

Regards,
Willy


diff --git a/net/socket.c b/net/socket.c
index ac45b13..910ef88 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1307,11 +1307,17 @@ asmlinkage long sys_setsockopt(int fd, i
 asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval, int *optlen)
 {
 	int err;
+	int len;
 	struct socket *sock;
 
 	if ((sock = sockfd_lookup(fd, &err))!=NULL)
 	{
-		if (level == SOL_SOCKET)
+		/* XXX: insufficient for SMP, but should be redundant anyway */
+		if (get_user(len, optlen))
+			err = -EFAULT;
+		else if (len < 0)
+			err = -EINVAL;
+		else if (level == SOL_SOCKET)
 			err=sock_getsockopt(sock,level,optname,optval,optlen);
 		else
 			err=sock->ops->getsockopt(sock, level, optname, optval, optlen);
-- 
1.4.1

Re: [PATCH] getsockopt() early argument sanity checking

[David Miller]

From: Willy Tarreau <w@1wt.eu>
Date: Sun, 20 Aug 2006 02:43:07 +0200

> On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
> > Not to me. It heavily violates codingstyle and screws brains
>                 ^^^^^^^
> little exageration detected here.
> 
> > with the non-indented else branches.
> 
> while they surprized me first, they make the *patch* more readable
> by clearly showing what has been inserted and where. However, I have
> joined the lines for the merge.

Thanks for consulting the networking maintainer before merging
this. :-/

What if some sockopt treats a negative length specially?
Maybe some setsockopt() doesn't care about the optlen pointer?

This toplevel code has no buisness interpreting the arguments when the
downcall and argument interpretation is by definition protocol
specific.  It also means we'll touch userspace twice for this value
which is really dumb.

The only nice part about this change is that it allows us to be lazy
about auditing the individual setsockopt() implementations.  I'd
rather fix the broken cases than add a patch which just assumes they
are broken and not worth fixing, and also imposes a convention for the
optlen argument.

No thanks.

And yes the coding style was totally unacceptable too.

Re: [PATCH] getsockopt() early argument sanity checking

[Willy Tarreau]

Hi David,

On Sun, Aug 20, 2006 at 12:44:27PM -0700, David Miller wrote:
> From: Willy Tarreau <w@1wt.eu>
> Date: Sun, 20 Aug 2006 02:43:07 +0200
> 
> > On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
> > > Not to me. It heavily violates codingstyle and screws brains
> >                 ^^^^^^^
> > little exageration detected here.
> > 
> > > with the non-indented else branches.
> > 
> > while they surprized me first, they make the *patch* more readable
> > by clearly showing what has been inserted and where. However, I have
> > joined the lines for the merge.
> 
> Thanks for consulting the networking maintainer before merging
> this. :-/

I'm sorry, will try to do better next time :-(
I only queue the patches locally while they're being discussed anyway.

> What if some sockopt treats a negative length specially?
> Maybe some setsockopt() doesn't care about the optlen pointer?

Which should not be a reason for userspace to respect the calling convention.
>From man 2 getsockopt, it's clear that optlen is the size of the buffer :

       For getsockopt, optlen is a value-result
       parameter, initially containing the  size  of  the  buffer
       pointed  to  by optval, and modified on return to indicate
       the actual size of the value returned.

Where I can agree with you is on this part of the man :

       If no option value is to be supplied or returned, optval may be NULL.

Nothing is said about optlen's validity in such a case.

> This toplevel code has no buisness interpreting the arguments when the
> downcall and argument interpretation is by definition protocol
> specific.

Generally, the advantage of doing checks early is that they help enforcing
conventions and sometimes protect against a stupid bug in one of the multiple
leaves. It should in no way be an excuse to keep the remaining stupid bugs
when we spot them.

> It also means we'll touch userspace twice for this value which is really
> dumb.

If this is that dumb, then why is it done twice in tcp_getsockopt() for
TCP_INFO ? would you accept a fix which copies it in a local variable
instead ? 2.6 would also benefit from this for TCP_CONGESTION.

> The only nice part about this change is that it allows us to be lazy
> about auditing the individual setsockopt() implementations.

This is absolutely not the intent. When we merged Julien Tinnes's fixes
for NULL pointer checks, it "looked like" the callers already performed
the tests, but that was not a reason not to complete the test in the leaf
functions.

> I'd rather fix the broken cases than add a patch which just assumes they
> are broken and not worth fixing

We're not assuming they're broken. When some code is maintained by many people
and when conventions differ between similar functions (eg: setsockopt does
the check at top level and getsockopt in the leaves), it should be expected
that we populate the CVE lists from time to time when we decide that there
will always be one single check for every argument. And this is true for all
system parts.

> and also imposes a convention for the optlen argument.

I would better understand this one considering the fact that the man is
vague on this matter.

> No thanks.

OK, fine, I drop it. People who seek better security know where to find
hardening patches anyway.

Willy

Re: [PATCH] getsockopt() early argument sanity checking

[Arjan van de Ven]

> We're not assuming they're broken. When some code is maintained by many people
> and when conventions differ between similar functions (eg: setsockopt does
> the check at top level and getsockopt in the leaves),

thats not something you want to fix in 2.4 though ;)

it may be worth considering for 2.6 of course.

Re: [PATCH] getsockopt() early argument sanity checking

[Eugene Teo]

Willy Tarreau wrote:
> On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
>> On Sunday 20 August 2006 01:48, Willy Tarreau wrote:
>>> On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
[snipped]
> diff --git a/net/socket.c b/net/socket.c
> index ac45b13..910ef88 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1307,11 +1307,17 @@ asmlinkage long sys_setsockopt(int fd, i
>  asmlinkage long sys_getsockopt(int fd, int level, int optname, char *optval, int *optlen)
>  {
>  	int err;
> +	int len;
        ^^^^^^^^

>  	struct socket *sock;
>  
>  	if ((sock = sockfd_lookup(fd, &err))!=NULL)
>  	{
> -		if (level == SOL_SOCKET)
> +		/* XXX: insufficient for SMP, but should be redundant anyway */
> +		if (get_user(len, optlen))
> +			err = -EFAULT;
> +		else if (len < 0)
                ^^^^^^^^^^^^^^^^^

s/else//

> +			err = -EINVAL;
> +		else if (level == SOL_SOCKET)

s/else//

>  			err=sock_getsockopt(sock,level,optname,optval,optlen);
>  		else
>  			err=sock->ops->getsockopt(sock, level, optname, optval, optlen);

These checks are already in getsockopt(). Duplicated code?

Eugene
-- 
eteo redhat.com  ph: +65 6490 4142  http://www.kernel.org/~eugeneteo
gpg fingerprint:  47B9 90F6 AE4A 9C51 37E0  D6E1 EA84 C6A2 58DF 8823

Re: [PATCH] getsockopt() early argument sanity checking

[Andi Kleen]

On Sunday 20 August 2006 01:05, Solar Designer wrote:
> I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
> into 2.4.34-pre.
> 
> (2.6 kernels could benefit from the same change, too, but at the moment
> I am dealing with proper submission of generic changes like this that
> are a part of 2.4.33-ow1.)

In general I don't think it makes sense to submit stuff for 2.4 
that isn't in 2.6.

> 
> The patch makes getsockopt(2) sanity-check the value pointed to by
> the optlen argument early on.  This is a security hardening measure
> intended to prevent exploitation of certain potential vulnerabilities in
> socket type specific getsockopt() code on UP systems.

It's not only insufficient on SMP, but even on UP where a thread
can sleep in get_user and another one can run in this time.

Doing a check that is inherently racy everywhere doesn't seem like
a security improvement to me. If there is really a length checking bug somewhere 
it needs to be fixed in a race-free way. If not then there is no need
for a change.

-Andi

Re: [PATCH] getsockopt() early argument sanity checking

[Willy Tarreau]

On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
> On Sunday 20 August 2006 01:05, Solar Designer wrote:
> > I propose the attached patch (extracted from 2.4.33-ow1) for inclusion
> > into 2.4.34-pre.
> > 
> > (2.6 kernels could benefit from the same change, too, but at the moment
> > I am dealing with proper submission of generic changes like this that
> > are a part of 2.4.33-ow1.)
> 
> In general I don't think it makes sense to submit stuff for 2.4 
> that isn't in 2.6.

I generally agree with you on this, but I think that when they are just
preventive measures, they can be applied in whatever order, provided that
they don't get lost.

> > The patch makes getsockopt(2) sanity-check the value pointed to by
> > the optlen argument early on.  This is a security hardening measure
> > intended to prevent exploitation of certain potential vulnerabilities in
> > socket type specific getsockopt() code on UP systems.
> 
> It's not only insufficient on SMP, but even on UP where a thread
> can sleep in get_user and another one can run in this time.

Valid point.

> Doing a check that is inherently racy everywhere doesn't seem like
> a security improvement to me. If there is really a length checking bug somewhere 
> it needs to be fixed in a race-free way.

The only race-free solution would be to add an argument to all getsockopt()
functions and pass them the decoded value. There are other places where
multiple get_user() are performed on optlen, with some useless tests (eg
in net/ipv4/tcp.c, len is checked for <0 after having been compared to
sizeof(int) as unsigned int) and all those places would benefit from this.

But I don't want to induce such large changes in this kernel. The goal of
this test is a preventive measure to catch easily exploitable errors that
might have remained undetected. For instance, a quick glance shows this
portion of code in net/ipv4/raw.c (both 2.4 and 2.6) :

static int raw_seticmpfilter(struct sock *sk, char *optval, int optlen)
{
        if (optlen > sizeof(struct icmp_filter))
                optlen = sizeof(struct icmp_filter);
        if (copy_from_user(&sk->tp_pinfo.tp_raw4.filter, optval, optlen))
                return -EFAULT;
        return 0;
}

It only relies on sock_setsockopt() refusing optlen values < sizeof(int),
and this is not documented. Having part of this code being copied for use
in another code path would open a breach for optlen < 0.

> If not then there is no need for a change.

There are two tests in this patch :

  - one on the validity of the optlen address. This one is race-free and
    should be conserved anyway.

  - one on the optlen range which is valid for most cases but which is
    subject to a race condition and which might be circumvented by
    carefully written code and with some luck as in all race conditions
    issues.

Some will say that this last one offers a first level of protection by
transforming determinist attacks into racy attacks which make potential
vulnerabilities much harder to exploit by code injection. I'm one of them.

Others will consider it totally useless because it does not cover all cases,
but I think it is against the general principle of precaution we try to
apply in security.

> -Andi

Regards,
Willy

Re: [PATCH] getsockopt() early argument sanity checking

[YOSHIFUJI Hideaki / 吉藤英明]

Hello.

In article <20060820101528.GE602@1wt.eu> (at Sun, 20 Aug 2006 12:15:28 +0200), Willy Tarreau <w@1wt.eu> says:

> But I don't want to induce such large changes in this kernel. The goal of
> this test is a preventive measure to catch easily exploitable errors that
> might have remained undetected. For instance, a quick glance shows this
> portion of code in net/ipv4/raw.c (both 2.4 and 2.6) :
> 
> static int raw_seticmpfilter(struct sock *sk, char *optval, int optlen)
> {
>         if (optlen > sizeof(struct icmp_filter))
>                 optlen = sizeof(struct icmp_filter);
>         if (copy_from_user(&sk->tp_pinfo.tp_raw4.filter, optval, optlen))
>                 return -EFAULT;
>         return 0;
> }
> 
> It only relies on sock_setsockopt() refusing optlen values < sizeof(int),
> and this is not documented. Having part of this code being copied for use
> in another code path would open a breach for optlen < 0.
:
> There are two tests in this patch :
> 
>   - one on the validity of the optlen address. This one is race-free and
>     should be conserved anyway.
> 
>   - one on the optlen range which is valid for most cases but which is
>     subject to a race condition and which might be circumvented by
>     carefully written code and with some luck as in all race conditions
>     issues.

Don't mix getsockopt() and setsockopt() code paths.

For setsockopt(), optlen < 0 is checked in net/socket.c:sys_setsockopt().

For getsockopt(), optlen and *optlen < 0 is (and should be) checked
(or handled) in each getsockopt function; e.g. do_ip_getsockopt(),
raw_geticmpfilter() etc.

--yoshfuji

Re: [PATCH] getsockopt() early argument sanity checking

[David Miller]

From: Willy Tarreau <w@1wt.eu>
Date: Sun, 20 Aug 2006 12:15:28 +0200

> Others will consider it totally useless because it does not cover
> all cases, but I think it is against the general principle of
> precaution we try to apply in security.

Reading in a value from userspace twice for questionable
"security"  is just bogus.

Re: [PATCH] getsockopt() early argument sanity checking

[Solar Designer]

On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
> In general I don't think it makes sense to submit stuff for 2.4 
> that isn't in 2.6.

In general I agree, however right now I had the choice between
submitting these changes for 2.4 first and not submitting them at all
(at least for some months more).  I chose the former.

> > The patch makes getsockopt(2) sanity-check the value pointed to by
> > the optlen argument early on.  This is a security hardening measure
> > intended to prevent exploitation of certain potential vulnerabilities in
> > socket type specific getsockopt() code on UP systems.
> 
> It's not only insufficient on SMP, but even on UP where a thread
> can sleep in get_user and another one can run in this time.

Good point.  However, what about this special case? -

We're on UP.  sys_getsockopt() does get_user() (due to the patch) and
makes sure that the passed *optlen is sane.  Even if this get_user()
sleeps, the value it returns in "len" is what's currently in memory at
the time of the get_user() return (correct?)  Then an underlying
*getsockopt() function does another get_user() on optlen (same address),
without doing any other user-space data accesses or anything else that
could sleep first.  Is it possible that this second get_user()
invocation would sleep?  I think not since it's the same address that
we've just read a value from, we did not leave kernel space, and we're
on UP (so no other processor could have changed the mapping).  So the
patch appears to be sufficient for this special case (which is not
unlikely).

Of course, it is possible that I am wrong about some of the above;
please correct me if so.

> If there is really a length checking bug somewhere it needs to be
> fixed in a race-free way.

Indeed, all known bugs need to be fixed properly.

Thanks,

Alexander

Re: [PATCH] getsockopt() early argument sanity checking

[Arjan van de Ven]

> We're on UP.  sys_getsockopt() does get_user() (due to the patch) and
> makes sure that the passed *optlen is sane.  Even if this get_user()
> sleeps, the value it returns in "len" is what's currently in memory at
> the time of the get_user() return (correct?)  Then an underlying
> *getsockopt() function does another get_user() on optlen (same address),
> without doing any other user-space data accesses or anything else that
> could sleep first.  Is it possible that this second get_user()
> invocation would sleep?  I think not since it's the same address that
> we've just read a value from, we did not leave kernel space, and we're
> on UP (so no other processor could have changed the mapping).  So the
> patch appears to be sufficient for this special case (which is not
> unlikely).

this reasoning goes out the window with kernel preemption of course ;)

Re: [PATCH] getsockopt() early argument sanity checking

[David Miller]

From: Arjan van de Ven <arjan@infradead.org>
Date: Sun, 20 Aug 2006 18:30:51 +0200

> this reasoning goes out the window with kernel preemption of course ;)

Yes and even though this thing is for 2.4.x, it just shows
that this is a hack and we shouldn't eat two userspace
accesses for a hack.

Instead fix the setsockopt() implementations that aren't
checking the optlen parameter correctly.

Re: [PATCH] getsockopt() early argument sanity checking

[Andi Kleen]

On Sunday 20 August 2006 18:16, Solar Designer wrote:
> On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
> > In general I don't think it makes sense to submit stuff for 2.4 
> > that isn't in 2.6.
> 
> In general I agree, however right now I had the choice between
> submitting these changes for 2.4 first and not submitting them at all
> (at least for some months more).  I chose the former.

If there is really a length checking bug it shouldn't be that hard to fix it 
in both.


> We're on UP.  sys_getsockopt() does get_user() (due to the patch) and
> makes sure that the passed *optlen is sane.  Even if this get_user()
> sleeps, the value it returns in "len" is what's currently in memory at
> the time of the get_user() return (correct?)  Then an underlying
> *getsockopt() function does another get_user() on optlen (same address),
> without doing any other user-space data accesses or anything else that
> could sleep first.  Is it possible that this second get_user()
> invocation would sleep?  I think not since it's the same address that
> we've just read a value from, we did not leave kernel space, and we're
> on UP (so no other processor could have changed the mapping).  So the
> patch appears to be sufficient for this special case (which is not
> unlikely).
> 
> Of course, it is possible that I am wrong about some of the above;
> please correct me if so.

Nah you're right (except on a preemptible kernel which 2.4 isn't unpatched)
However if there is any other user access before the second get_user 
the race could happen again even on UP.

-Andi

Re: [PATCH] getsockopt() early argument sanity checking

[Solar Designer]

On Sun, Aug 20, 2006 at 08:38:34PM +0200, Andi Kleen wrote:
> On Sunday 20 August 2006 18:16, Solar Designer wrote:
> > On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
> > > In general I don't think it makes sense to submit stuff for 2.4 
> > > that isn't in 2.6.
> > 
> > In general I agree, however right now I had the choice between
> > submitting these changes for 2.4 first and not submitting them at all
> > (at least for some months more).  I chose the former.
> 
> If there is really a length checking bug it shouldn't be that hard to fix it 
> in both.

There were such length checking bugs being discovered and fixed in the
past.  In particular, many got fixed between 2.2.18 and 2.2.19; that was
also when I added this hardening measure to -ow patches (starting with
2.2.19-ow1 released 5 years ago).

Of course, any known bugs should be fixed ASAP, but to me that is not a
sufficient reason to not keep a hardening measure like this.  It's just
a matter of opinion.

Alexander

Re: [PATCH] getsockopt() early argument sanity checking

[David Miller]

From: Andi Kleen <ak@suse.de>
Date: Sun, 20 Aug 2006 10:34:43 +0200

> Doing a check that is inherently racy everywhere doesn't seem like a
> security improvement to me. If there is really a length checking bug
> somewhere it needs to be fixed in a race-free way. If not then there
> is no need for a change.

Totally agreed, this change makes no sense on every level.

Re: [PATCH] getsockopt() early argument sanity checking

[Alan Cox]

Ar Sul, 2006-08-20 am 03:05 +0400, ysgrifennodd Solar Designer:
> The patch makes getsockopt(2) sanity-check the value pointed to by
> the optlen argument early on.  This is a security hardening measure
> intended to prevent exploitation of certain potential vulnerabilities in
> socket type specific getsockopt() code on UP systems.
> 
> This change has been a part of -ow patches for some years.

Is it in 2.6 ?

Re: [PATCH] getsockopt() early argument sanity checking

[David Wagner]

Solar Designer  wrote:
>The patch makes getsockopt(2) sanity-check the value pointed to by
>the optlen argument early on.  This is a security hardening measure
>intended to prevent exploitation of certain potential vulnerabilities in
>socket type specific getsockopt() code on UP systems.

This looks broken to me.  It has a TOCTTOU (time-of-check-to-time-of-use)
vulnerability (i.e., race condition): you read the length value twice,
and assume that you will get the same value both times.  That assumption
is not valid.

It looks like it will be easy to bypass this check.  For instance,
think about what happens if an adversary stores the length field in a
mmaped region, for instance.  It should be easy for the value of that
length field to change between when it was first read and when it was
subsequently read.  I don't see how this provides any "hardening" if
the attacker knows how to read kernel source code.  Am I missing
something?

Re: [PATCH] getsockopt() early argument sanity checking

[Solar Designer]

(I realize that the patch has been rejected and this message is in no
way meant to affect that decision.)

On Mon, Aug 21, 2006 at 03:00:22AM +0000, David Wagner wrote:
> Solar Designer  wrote:
> >The patch makes getsockopt(2) sanity-check the value pointed to by
> >the optlen argument early on.  This is a security hardening measure
> >intended to prevent exploitation of certain potential vulnerabilities in
> >socket type specific getsockopt() code on UP systems.
> 
> This looks broken to me.  It has a TOCTTOU (time-of-check-to-time-of-use)

Yes it does, using Matt Bishop's classification.

> vulnerability (i.e., race condition): you read the length value twice,
> and assume that you will get the same value both times.  That assumption
> is not valid.

I don't assume that.  I realize that there's the race condition on SMP
and, as pointed out by Andi Kleen, in many cases also on UP systems.
That's why I had the XXX comment in there from the very beginning.

This added check is not supposed to be relied upon; rather, it is a
hardening measure in case we miss a bug in underlying *getsockopt()
functions.

> It looks like it will be easy to bypass this check.

It depends.  You might have missed this description of a special case
where it does not appear to be possible to bypass the check:

	http://lkml.org/lkml/2006/8/20/148

Yes, the patch is highly controversial and I mostly agree with its
opponents (I had much of the same thoughts myself, except for DaveM's
concern that *optlen might be uninitialized or negative on purpose),
yet I am going to keep it in -ow.

BTW, I had previously submitted a very similar check to do_sysctl(),
also with an XXX comment, which got in a few years back.

I won't be surprised if one of these checks saves a system from a
compromise one day.  This world is not perfect - neither the rest of the
Linux kernel code nor vulnerability exploit programs are perfect.

Alexander

P.S. Please CC me on your replies.

Re: [PATCH] getsockopt() early argument sanity checking

[Manfred Spraul]

Arjan wrote:

>> We're on UP.  sys_getsockopt() does get_user() (due to the patch) and
>> makes sure that the passed *optlen is sane.  Even if this get_user()
>> sleeps, the value it returns in "len" is what's currently in memory at
>> the time of the get_user() return (correct?)  Then an underlying
>> *getsockopt() function does another get_user() on optlen (same address),
>> without doing any other user-space data accesses or anything else that
>> could sleep first.  Is it possible that this second get_user()
>> invocation would sleep?  I think not since it's the same address that
>> we've just read a value from, we did not leave kernel space, and we're
>> on UP (so no other processor could have changed the mapping).  So the
>> patch appears to be sufficient for this special case (which is not
>> unlikely).
>
>this reasoning goes out the window with kernel preemption of course ;)
>  
>
Or O_DIRECT? I'm not sure what's easier to time, a kernel preemption or 
a DMA to the user address.

--
    Manfred