Re: [PATCH] getsockopt() early argument sanity checking
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
-
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
Re: [PATCH] getsockopt() early argument sanity checking
> 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. - 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
Re: [PATCH] getsockopt() early argument sanity checking
Hi David, On Sun, Aug 20, 2006 at 12:44:27PM -0700, David Miller wrote: > From: Willy Tarreau <[EMAIL PROTECTED]> > 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 - 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
Re: [PATCH] getsockopt() early argument sanity checking
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 - 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
Re: [PATCH] getsockopt() early argument sanity checking
From: Willy Tarreau <[EMAIL PROTECTED]> 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. - 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
Re: [PATCH] getsockopt() early argument sanity checking
From: Arjan van de Ven <[EMAIL PROTECTED]> 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. - 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
Re: [PATCH] getsockopt() early argument sanity checking
From: Andi Kleen <[EMAIL PROTECTED]> 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. - 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
Re: [PATCH] getsockopt() early argument sanity checking
From: Willy Tarreau <[EMAIL PROTECTED]> 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. - 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
Re: [PATCH] getsockopt() early argument sanity checking
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 - 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
Re: [PATCH] getsockopt() early argument sanity checking
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 ? - 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
Re: [PATCH] getsockopt() early argument sanity checking
> 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 ;) - 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
Re: [PATCH] getsockopt() early argument sanity checking
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 - 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
Re: [PATCH] getsockopt() early argument sanity checking
Hello.
In article <[EMAIL PROTECTED]> (at Sun, 20 Aug 2006 12:15:28 +0200), Willy
Tarreau <[EMAIL PROTECTED]> 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
-
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
Re: [PATCH] getsockopt() early argument sanity checking
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
-
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
Re: [PATCH] getsockopt() early argument sanity checking
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 - 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
Re: [PATCH] getsockopt() early argument sanity checking
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
-
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
Re: [PATCH] getsockopt() early argument sanity checking
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
> > 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 [EMAIL PROTECTED]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Greetings Michael.
-
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
Re: [PATCH] getsockopt() early argument sanity checking
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
> 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.cSat 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] 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,
--
Alexander Peslyak
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
