mbox

Dapper CVE-2010-3880, inet_diag: Make sure we actually run the same bytecode we audited

Message ID 20110202190454.7F466F89F8@sepang.rtg.net
State Accepted
Delegated to: Stefan Bader
Headers show

Pull-request

git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3880

Message

Tim Gardner Feb. 2, 2011, 7:04 p.m. UTC 
The following changes since commit 2677c2506a252a7a8f322434ce3e3ae7acffdd39:
  Thomas Gleixner (1):
        x86: replace LOCK_PREFIX in futex.h, CVE-2010-3086

are available in the git repository at:

  git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3880

Tim Gardner (1):
      inet_diag: Make sure we actually run the same bytecode we audited, CVE-2010-3880

 net/ipv4/inet_diag.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

From e89143e20d2e91b55d7b1f78b57304dae7648160 Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Wed, 2 Feb 2011 11:10:04 -0700
Subject: [PATCH] inet_diag: Make sure we actually run the same bytecode we audited, CVE-2010-3880

BugLink: http://bugs.launchpad.net/bugs/711865

CVE-2010-3880

We were using nlmsg_find_attr() to look up the bytecode by attribute when
auditing, but then just using the first attribute when actually running
bytecode. So, if we received a message with two attribute elements, where only
the second had type INET_DIAG_REQ_BYTECODE, we would validate and run different
bytecode strings.

Fix this by consistently using nlmsg_find_attr everywhere.

Signed-off-by: Nelson Elhage <nelhage@ksplice.com>
Signed-off-by: Thomas Graf <tgraf@infradead.org>
Signed-off-by: David S. Miller <davem@davemloft.net>
(back ported from commit 22e76c849d505d87c5ecf3d3e6742a65f0ff4860)

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 net/ipv4/inet_diag.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

Comments

Stefan Bader Feb. 7, 2011, 3:01 p.m. UTC | #1 
On 02/02/2011 08:04 PM, Tim Gardner wrote:
> The following changes since commit 2677c2506a252a7a8f322434ce3e3ae7acffdd39:
>   Thomas Gleixner (1):
>         x86: replace LOCK_PREFIX in futex.h, CVE-2010-3086
> 
> are available in the git repository at:
> 
>   git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3880
> 
> Tim Gardner (1):
>       inet_diag: Make sure we actually run the same bytecode we audited, CVE-2010-3880
> 
>  net/ipv4/inet_diag.c |   17 +++++++++--------
>  1 files changed, 9 insertions(+), 8 deletions(-)
> 
> From e89143e20d2e91b55d7b1f78b57304dae7648160 Mon Sep 17 00:00:00 2001
> From: Tim Gardner <tim.gardner@canonical.com>
> Date: Wed, 2 Feb 2011 11:10:04 -0700
> Subject: [PATCH] inet_diag: Make sure we actually run the same bytecode we audited, CVE-2010-3880
> 
> BugLink: http://bugs.launchpad.net/bugs/711865
> 
> CVE-2010-3880
> 
> We were using nlmsg_find_attr() to look up the bytecode by attribute when
> auditing, but then just using the first attribute when actually running
> bytecode. So, if we received a message with two attribute elements, where only
> the second had type INET_DIAG_REQ_BYTECODE, we would validate and run different
> bytecode strings.
> 
> Fix this by consistently using nlmsg_find_attr everywhere.
> 
> Signed-off-by: Nelson Elhage <nelhage@ksplice.com>
> Signed-off-by: Thomas Graf <tgraf@infradead.org>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> (back ported from commit 22e76c849d505d87c5ecf3d3e6742a65f0ff4860)
> 
> Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  net/ipv4/inet_diag.c |   17 +++++++++--------
>  1 files changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
> index 39061ed..00c85b1 100644
> --- a/net/ipv4/inet_diag.c
> +++ b/net/ipv4/inet_diag.c
> @@ -28,12 +28,12 @@
>  #include <net/inet_hashtables.h>
>  #include <net/inet_timewait_sock.h>
>  #include <net/inet6_hashtables.h>
> +#include <net/netlink.h>
>  
>  #include <linux/inet.h>
>  #include <linux/stddef.h>
>  
>  #include <linux/inet_diag.h>
> -
>  static const struct inet_diag_handler **inet_diag_table;
>  
>  struct inet_diag_entry {
> @@ -418,9 +418,10 @@ static int inet_diag_dump_sock(struct sk_buff *skb, struct sock *sk,
>  {
>  	struct inet_diag_req *r = NLMSG_DATA(cb->nlh);
>  
> -	if (cb->nlh->nlmsg_len > 4 + NLMSG_SPACE(sizeof(*r))) {
> +	if (nlmsg_attrlen(cb->nlh, sizeof(*r))) {
>  		struct inet_diag_entry entry;
> -		struct rtattr *bc = (struct rtattr *)(r + 1);
> +		struct rtattr *bc = nlmsg_find_attr(cb->nlh, sizeof(*r),
> +						INET_DIAG_REQ_BYTECODE);
>  		struct inet_sock *inet = inet_sk(sk);
>  
>  		entry.family = sk->sk_family;
> @@ -440,7 +441,7 @@ static int inet_diag_dump_sock(struct sk_buff *skb, struct sock *sk,
>  		entry.dport = ntohs(inet->dport);
>  		entry.userlocks = sk->sk_userlocks;
>  
> -		if (!inet_diag_bc_run(RTA_DATA(bc), RTA_PAYLOAD(bc), &entry))
> +		if (!inet_diag_bc_run(nla_data(bc), nla_len(bc), &entry))
>  			return 0;
>  	}
>  
> @@ -530,8 +531,8 @@ static int inet_diag_dump_reqs(struct sk_buff *skb, struct sock *sk,
>  	if (!lopt || !lopt->qlen)
>  		goto out;
>  
> -	if (cb->nlh->nlmsg_len > 4 + NLMSG_SPACE(sizeof(*r))) {
> -		bc = (struct rtattr *)(r + 1);
> +	if (nlmsg_attrlen(cb->nlh, sizeof(*r))) {
> +		bc = nlmsg_find_attr(cb->nlh, sizeof(*r), INET_DIAG_REQ_BYTECODE);
>  		entry.sport = inet->num;
>  		entry.userlocks = sk->sk_userlocks;
>  	}
> @@ -564,8 +565,8 @@ static int inet_diag_dump_reqs(struct sk_buff *skb, struct sock *sk,
>  					&ireq->rmt_addr;
>  				entry.dport = ntohs(ireq->rmt_port);
>  
> -				if (!inet_diag_bc_run(RTA_DATA(bc),
> -						    RTA_PAYLOAD(bc), &entry))
> +				if (!inet_diag_bc_run(nla_data(bc),
> +						    nla_len(bc), &entry))
>  					continue;
>  			}
>
Brad Figg Feb. 9, 2011, 8:40 p.m. UTC | #2 
On 02/07/2011 07:01 AM, Stefan Bader wrote:
> On 02/02/2011 08:04 PM, Tim Gardner wrote:
>> The following changes since commit 2677c2506a252a7a8f322434ce3e3ae7acffdd39:
>>    Thomas Gleixner (1):
>>          x86: replace LOCK_PREFIX in futex.h, CVE-2010-3086
>>
>> are available in the git repository at:
>>
>>    git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3880
>>
>> Tim Gardner (1):
>>        inet_diag: Make sure we actually run the same bytecode we audited, CVE-2010-3880
>>
>>   net/ipv4/inet_diag.c |   17 +++++++++--------
>>   1 files changed, 9 insertions(+), 8 deletions(-)
>>
>>  From e89143e20d2e91b55d7b1f78b57304dae7648160 Mon Sep 17 00:00:00 2001
>> From: Tim Gardner<tim.gardner@canonical.com>
>> Date: Wed, 2 Feb 2011 11:10:04 -0700
>> Subject: [PATCH] inet_diag: Make sure we actually run the same bytecode we audited, CVE-2010-3880
>>
>> BugLink: http://bugs.launchpad.net/bugs/711865
>>
>> CVE-2010-3880
>>
>> We were using nlmsg_find_attr() to look up the bytecode by attribute when
>> auditing, but then just using the first attribute when actually running
>> bytecode. So, if we received a message with two attribute elements, where only
>> the second had type INET_DIAG_REQ_BYTECODE, we would validate and run different
>> bytecode strings.
>>
>> Fix this by consistently using nlmsg_find_attr everywhere.
>>
>> Signed-off-by: Nelson Elhage<nelhage@ksplice.com>
>> Signed-off-by: Thomas Graf<tgraf@infradead.org>
>> Signed-off-by: David S. Miller<davem@davemloft.net>
>> (back ported from commit 22e76c849d505d87c5ecf3d3e6742a65f0ff4860)
>>
>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
> Acked-by: Stefan Bader<stefan.bader@canonical.com>
>> ---
>>   net/ipv4/inet_diag.c |   17 +++++++++--------
>>   1 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
>> index 39061ed..00c85b1 100644
>> --- a/net/ipv4/inet_diag.c
>> +++ b/net/ipv4/inet_diag.c
>> @@ -28,12 +28,12 @@
>>   #include<net/inet_hashtables.h>
>>   #include<net/inet_timewait_sock.h>
>>   #include<net/inet6_hashtables.h>
>> +#include<net/netlink.h>
>>
>>   #include<linux/inet.h>
>>   #include<linux/stddef.h>
>>
>>   #include<linux/inet_diag.h>
>> -
>>   static const struct inet_diag_handler **inet_diag_table;
>>
>>   struct inet_diag_entry {
>> @@ -418,9 +418,10 @@ static int inet_diag_dump_sock(struct sk_buff *skb, struct sock *sk,
>>   {
>>   	struct inet_diag_req *r = NLMSG_DATA(cb->nlh);
>>
>> -	if (cb->nlh->nlmsg_len>  4 + NLMSG_SPACE(sizeof(*r))) {
>> +	if (nlmsg_attrlen(cb->nlh, sizeof(*r))) {
>>   		struct inet_diag_entry entry;
>> -		struct rtattr *bc = (struct rtattr *)(r + 1);
>> +		struct rtattr *bc = nlmsg_find_attr(cb->nlh, sizeof(*r),
>> +						INET_DIAG_REQ_BYTECODE);

In the other patches this was changed to:
                 const struct nlattr *bc = nlmsg_find_attr(cb->nlh,
                                                           sizeof(*r),
                                                           INET_DIAG_REQ_BYTECODE);


>>   		struct inet_sock *inet = inet_sk(sk);
>>
>>   		entry.family = sk->sk_family;
>> @@ -440,7 +441,7 @@ static int inet_diag_dump_sock(struct sk_buff *skb, struct sock *sk,
>>   		entry.dport = ntohs(inet->dport);
>>   		entry.userlocks = sk->sk_userlocks;
>>
>> -		if (!inet_diag_bc_run(RTA_DATA(bc), RTA_PAYLOAD(bc),&entry))
>> +		if (!inet_diag_bc_run(nla_data(bc), nla_len(bc),&entry))
>>   			return 0;
>>   	}
>>
>> @@ -530,8 +531,8 @@ static int inet_diag_dump_reqs(struct sk_buff *skb, struct sock *sk,
>>   	if (!lopt || !lopt->qlen)
>>   		goto out;
>>
>> -	if (cb->nlh->nlmsg_len>  4 + NLMSG_SPACE(sizeof(*r))) {
>> -		bc = (struct rtattr *)(r + 1);
>> +	if (nlmsg_attrlen(cb->nlh, sizeof(*r))) {
>> +		bc = nlmsg_find_attr(cb->nlh, sizeof(*r), INET_DIAG_REQ_BYTECODE);
>>   		entry.sport = inet->num;
>>   		entry.userlocks = sk->sk_userlocks;
>>   	}
>> @@ -564,8 +565,8 @@ static int inet_diag_dump_reqs(struct sk_buff *skb, struct sock *sk,
>>   					&ireq->rmt_addr;
>>   				entry.dport = ntohs(ireq->rmt_port);
>>
>> -				if (!inet_diag_bc_run(RTA_DATA(bc),
>> -						    RTA_PAYLOAD(bc),&entry))
>> +				if (!inet_diag_bc_run(nla_data(bc),
>> +						    nla_len(bc),&entry))
>>   					continue;
>>   			}
>>
>
>
Tim Gardner Feb. 9, 2011, 8:47 p.m. UTC | #3 
On 02/09/2011 01:40 PM, Brad Figg wrote:
> On 02/07/2011 07:01 AM, Stefan Bader wrote:
>> On 02/02/2011 08:04 PM, Tim Gardner wrote:
>>> The following changes since commit
>>> 2677c2506a252a7a8f322434ce3e3ae7acffdd39:
>>> Thomas Gleixner (1):
>>> x86: replace LOCK_PREFIX in futex.h, CVE-2010-3086
>>>
>>> are available in the git repository at:
>>>
>>> git://kernel.ubuntu.com/rtg/ubuntu-dapper.git CVE-2010-3880
>>>
>>> Tim Gardner (1):
>>> inet_diag: Make sure we actually run the same bytecode we audited,
>>> CVE-2010-3880
>>>
>>> net/ipv4/inet_diag.c | 17 +++++++++--------
>>> 1 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> From e89143e20d2e91b55d7b1f78b57304dae7648160 Mon Sep 17 00:00:00 2001
>>> From: Tim Gardner<tim.gardner@canonical.com>
>>> Date: Wed, 2 Feb 2011 11:10:04 -0700
>>> Subject: [PATCH] inet_diag: Make sure we actually run the same
>>> bytecode we audited, CVE-2010-3880
>>>
>>> BugLink: http://bugs.launchpad.net/bugs/711865
>>>
>>> CVE-2010-3880
>>>
>>> We were using nlmsg_find_attr() to look up the bytecode by attribute
>>> when
>>> auditing, but then just using the first attribute when actually running
>>> bytecode. So, if we received a message with two attribute elements,
>>> where only
>>> the second had type INET_DIAG_REQ_BYTECODE, we would validate and run
>>> different
>>> bytecode strings.
>>>
>>> Fix this by consistently using nlmsg_find_attr everywhere.
>>>
>>> Signed-off-by: Nelson Elhage<nelhage@ksplice.com>
>>> Signed-off-by: Thomas Graf<tgraf@infradead.org>
>>> Signed-off-by: David S. Miller<davem@davemloft.net>
>>> (back ported from commit 22e76c849d505d87c5ecf3d3e6742a65f0ff4860)
>>>
>>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>> Acked-by: Stefan Bader<stefan.bader@canonical.com>
>>> ---
>>> net/ipv4/inet_diag.c | 17 +++++++++--------
>>> 1 files changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
>>> index 39061ed..00c85b1 100644
>>> --- a/net/ipv4/inet_diag.c
>>> +++ b/net/ipv4/inet_diag.c
>>> @@ -28,12 +28,12 @@
>>> #include<net/inet_hashtables.h>
>>> #include<net/inet_timewait_sock.h>
>>> #include<net/inet6_hashtables.h>
>>> +#include<net/netlink.h>
>>>
>>> #include<linux/inet.h>
>>> #include<linux/stddef.h>
>>>
>>> #include<linux/inet_diag.h>
>>> -
>>> static const struct inet_diag_handler **inet_diag_table;
>>>
>>> struct inet_diag_entry {
>>> @@ -418,9 +418,10 @@ static int inet_diag_dump_sock(struct sk_buff
>>> *skb, struct sock *sk,
>>> {
>>> struct inet_diag_req *r = NLMSG_DATA(cb->nlh);
>>>
>>> - if (cb->nlh->nlmsg_len> 4 + NLMSG_SPACE(sizeof(*r))) {
>>> + if (nlmsg_attrlen(cb->nlh, sizeof(*r))) {
>>> struct inet_diag_entry entry;
>>> - struct rtattr *bc = (struct rtattr *)(r + 1);
>>> + struct rtattr *bc = nlmsg_find_attr(cb->nlh, sizeof(*r),
>>> + INET_DIAG_REQ_BYTECODE);
>
> In the other patches this was changed to:
> const struct nlattr *bc = nlmsg_find_attr(cb->nlh,
> sizeof(*r),
> INET_DIAG_REQ_BYTECODE);
>
>

I dropped 'const' because the prototype for nlmsg_find_attr() doesn't 
call for it in 2.6.15.

rtg