getsockopt() early argument sanity checking

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,

Signature · 5 lines

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

net/socket.c +8 -0

@@ -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

Quoted 9 lines · Solar Designer

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.)

Quoted 6 lines

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

Quoted 19 lines

Thanks,

Signature · 5 lines

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

net/socket.c +1 -0

@@ -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)
 	{

net/socket.c asmlinkage long sys_setsockopt(int fd, i · line 1324

Quoted 10 lines

net/socket.c +7 -0

+		/* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 15 lines · Willy Tarreau

Quoted 14 lines · Solar Designer

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.

Quoted 3 lines

Quoted 1 line

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.

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.

Quoted 6 lines · Solar Designer

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.

Quoted 4 lines

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

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.

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

Quoted 4 lines

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.

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

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).

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.

Quoted 2 lines · Andi Kleen

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.

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.

Quoted 1 line

Not to me. It heavily violates codingstyle and screws brains

                ^^^^^^^

little exageration detected here.

Quoted 1 line

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.

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.

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.

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

Quoted 7 lines · Solar Designer

Quoted 2 lines · Andi Kleen

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.

From: Willy Tarreau <[email protected]>
Date: Sun, 20 Aug 2006 02:43:07 +0200

Quoted 5 lines · Michael Buesch

Quoted 1 line

Not to me. It heavily violates codingstyle and screws brains

                ^^^^^^^

little exageration detected here.

Quoted 5 lines

Quoted 1 line

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?

From man 2 getsockopt, it's clear that optlen is the size of the buffer :

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.

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),

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.

Quoted 4 lines · 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 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.

Quoted 2 lines · Michael Buesch

Quoted 1 line · Willy Tarreau

On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote: