From: Solar Designer <hidden> Date: 2006-08-19 23:09:53
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
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
--- 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, iasmlinkagelongsys_getsockopt(intfd,intlevel,intoptname,char*optval,int*optlen){interr;+intlen;structsocket*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;+elseif(level==SOL_SOCKET)err=sock_getsockopt(sock,level,optname,optval,optlen);else
From: Michael Buesch <hidden> Date: 2006-08-20 00:06:29
On Sunday 20 August 2006 01:48, Willy Tarreau wrote:
On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
quoted
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
quoted
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
--- 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, iasmlinkagelongsys_getsockopt(intfd,intlevel,intoptname,char*optval,int*optlen){interr;+intlen;structsocket*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;+elseif(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
On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
On Sunday 20 August 2006 01:48, Willy Tarreau wrote:
quoted
On Sun, Aug 20, 2006 at 03:05:32AM +0400, Solar Designer wrote:
quoted
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
@@ -1307,11 +1307,17 @@ asmlinkage long sys_setsockopt(int fd, iasmlinkagelongsys_getsockopt(intfd,intlevel,intoptname,char*optval,int*optlen){interr;+intlen;structsocket*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;+elseif(len<0)+err=-EINVAL;+elseif(level==SOL_SOCKET)err=sock_getsockopt(sock,level,optname,optval,optlen);elseerr=sock->ops->getsockopt(sock,level,optname,optval,optlen);
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
On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
On Sunday 20 August 2006 01:05, Solar Designer wrote:
quoted
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
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.
Hello.
In article [off-list ref] (at Sun, 20 Aug 2006 12:15:28 +0200), Willy Tarreau [off-list ref] 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
From: Solar Designer <hidden> Date: 2006-08-20 16:20:06
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.
quoted
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
From: Arjan van de Ven <hidden> Date: 2006-08-20 16:31:46
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 ;)
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.
On Sunday 20 August 2006 18:16, Solar Designer wrote:
On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
quoted
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
On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
quoted
Not to me. It heavily violates codingstyle and screws brains
^^^^^^^
little exageration detected here.
quoted
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.
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.
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.
From: David Miller <davem@davemloft.net> Date: 2006-08-20 19:47:42
From: Arjan van de Ven <redacted>
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.
From: Solar Designer <hidden> Date: 2006-08-20 20:00:33
On Sun, Aug 20, 2006 at 08:38:34PM +0200, Andi Kleen wrote:
On Sunday 20 August 2006 18:16, Solar Designer wrote:
quoted
On Sun, Aug 20, 2006 at 10:34:43AM +0200, Andi Kleen wrote:
quoted
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
On Sun, Aug 20, 2006 at 02:05:20AM +0200, Michael Buesch wrote:
quoted
Not to me. It heavily violates codingstyle and screws brains
^^^^^^^
little exageration detected here.
quoted
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
From: Arjan van de Ven <hidden> Date: 2006-08-20 21:13:04
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.
@@ -1307,11 +1307,17 @@ asmlinkage long sys_setsockopt(int fd, iasmlinkagelongsys_getsockopt(intfd,intlevel,intoptname,char*optval,int*optlen){interr;+intlen;
^^^^^^^^
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)