Skip to contentSkip to conversation

getsockopt() early argument sanity checking

PATCH In review Solar Designer 10 participants 21 messages Aug 19, 2006 – Aug 21, 2006

Subsystems (3)
Networking Network drivers Sockets
lore
Share & tools
lore

Solar Designer

author
#0 lore raw

PATCH 1/1 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 · 3 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
net/socket.c +8 -0
diff -urpPX nopatch linux-2.4.33/net/socket.c linux/net/socket.c
@@ -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
#1 lore raw
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 29 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 +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
#2 lore raw
Quoted 10 lines · Willy Tarreau
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 8 lines
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.

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

Quoted 39 lines

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

Signature · 1 line
Greetings Michael.
#3 lore raw
Quoted 16 lines · Michael Buesch
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 5 lines
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

                ^^^^^^^

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.

Quoted 1 line

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

net/socket.c +7 -1
index ac45b13..910ef88 100644
@@ -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);
Signature · 1 line
1.4.1
maintainer
#4 lore raw

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

Quoted 10 lines · Michael Buesch
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.

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.

#5 lore raw

Hi David,

Quoted 8 lines · David Miller

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 8 lines
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. :-/

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

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 1 line

No thanks.

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

Willy

#6 lore raw
Quoted 4 lines

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.

#7 lore raw
In reply to Willy Tarreau
Quoted 3 lines · Willy Tarreau
Quoted 2 lines · Michael Buesch
Quoted 1 line · Willy Tarreau

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

[snipped]

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

Quoted 9 lines
net/socket.c +1 -0
index ac45b13..910ef88 100644
@@ -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;

        ^^^^^^^^

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

Quoted 9 lines
net/socket.c +4 -1
 	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//

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

Quoted 2 lines
net/socket.c +2 -0
+			err = -EINVAL;
+		else if (level == SOL_SOCKET)

s/else//

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

Quoted 3 lines
net/socket.c +0 -0
 			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

Signature · 2 lines
eteo redhat.com  ph: +65 6490 4142  http://www.kernel.org/~eugeneteo
gpg fingerprint:  47B9 90F6 AE4A 9C51 37E0  D6E1 EA84 C6A2 58DF 8823
#8 lore raw
In reply to Solar Designer
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 5 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

#9 lore raw
Quoted 10 lines · Andi Kleen
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.

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.

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

Valid point.

Quoted 3 lines

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.

Quoted 1 line

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.

Quoted 1 line

-Andi

Regards,
Willy

#10 lore raw

Hello.

In article <[email protected]> (at Sun, 20 Aug 2006 12:15:28 +0200), Willy Tarreau <[email protected]> says:

Quoted 14 lines

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;
}
Quoted 3 lines

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.

:

Quoted 9 lines

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

maintainer
#11 lore raw
In reply to Willy Tarreau

From: Willy Tarreau <[email protected]>
Date: Sun, 20 Aug 2006 12:15:28 +0200

Quoted 3 lines

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.

author
#12 lore raw
In reply to Andi Kleen
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.

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

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.

Quoted 2 lines

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

#13 lore raw
Quoted 13 lines

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

maintainer
#14 lore raw

From: Arjan van de Ven <[email protected]>
Date: Sun, 20 Aug 2006 18:30:51 +0200

Quoted 1 line

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.

#15 lore raw
In reply to Solar Designer
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.

Quoted 13 lines

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

Quoted 2 lines

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

author
#16 lore raw
Quoted 11 lines · Andi Kleen
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.

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

maintainer
#17 lore raw
In reply to Andi Kleen

From: Andi Kleen <[email protected]>
Date: Sun, 20 Aug 2006 10:34:43 +0200

Quoted 4 lines

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.

#18 lore raw
In reply to Solar Designer

Ar Sul, 2006-08-20 am 03:05 +0400, ysgrifennodd Solar Designer:

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.

Is it in 2.6 ?

#19 lore raw
In reply to Solar Designer
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. 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?

author
#20 lore raw

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

Quoted 7 lines · David Wagner
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)

Yes it does, using Matt Bishop's classification.

Quoted 3 lines

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.

Quoted 1 line

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.