diff mbox

Hardy CVE-2010-3880, inet_diag: Make sure we actually run the same bytecode we audited (V3)

Message ID 4D52E449.10007@canonical.com
State Accepted
Headers show

Commit Message

Tim Gardner Feb. 9, 2011, 7 p.m. UTC 
On 02/09/2011 09:36 AM, Stefan Bader wrote:
> On 02/09/2011 03:15 PM, Tim Gardner wrote:
>> On 02/09/2011 07:08 AM, Tim Gardner wrote:
>>> On 02/07/2011 07:38 AM, Tim Gardner wrote:
>>>> On 02/07/2011 02:15 AM, Andy Whitcroft wrote:
>>>>> On Mon, Feb 07, 2011 at 09:48:11AM +0100, Stefan Bader wrote:
>>>>>> On 02/02/2011 09:43 PM, Tim Gardner wrote:
>>>>>>> On 02/02/2011 12:11 PM, Tim Gardner wrote:
>>>>>>>> Postpone this one for a bit. The custom binary openvz flavour is
>>>>>>>> failing.
>>>>>>>>
>>>>>>>> rtg
>>>>>>>
>>>>>>> Added the openvz cleanup patch.
>>>>>>>
>>>>>>> rtg
>>>>>>>
>>>>>> Hm, this one looks odd. The hunk in the openvz patch seems to add the
>>>>>> "sll->sll_plttype = 0" and that does not seem to be touched by this
>>>>>> cve.
>>>>>
>>>>> That sounds familiar. I think that that comes from the CVE below:
>>>>>
>>>>> commit 2c14b0a36cebda5a1fc2c69f2dd01eb73365aa65
>>>>> Author: Andy Whitcroft<apw@canonical.com>
>>>>> Date: Tue Feb 1 14:26:24 2011 +0000
>>>>>
>>>>> net: packet: fix information leak to userland, CVE-2010-3876
>>>>>
>>>>> I suspect that means this extra line should have come in earlier, not
>>>>> sure if it is worth splitting out now, but in a perfect world I suspect
>>>>> it should have been. My fault by the looks of it.
>>>>>
>>>>> -apw
>>>>>
>>>>
>>>> Yeah, I think it's still OK. It just moves a hunk of a patch around, and
>>>> the context lines go along for the ride. The custom binary patch
>>>> mechanism is a real pain.
>>>>
>>>> rtg
>>>
>>> I fixed 'net: packet: fix information leak to userland, CVE-2010-3876'
>>> for openvz and pushed +master-next, so you'll need to re-fetch.
>>>
>>> Now, back to the original Hardy CVE patch....
>>
>> OK, how about this.
>>
>> rtg
>>
> Probably... mostly... err maybe without the line saying there is a fixup for
> openvz in the commit message.
>
> -Stefan
>

I've removed the commit log line that references openvz.

rtg

Comments

Brad Figg Feb. 9, 2011, 8:04 p.m. UTC | #1 
On 02/09/2011 11:00 AM, Tim Gardner wrote:
> On 02/09/2011 09:36 AM, Stefan Bader wrote:
>> On 02/09/2011 03:15 PM, Tim Gardner wrote:
>>> On 02/09/2011 07:08 AM, Tim Gardner wrote:
>>>> On 02/07/2011 07:38 AM, Tim Gardner wrote:
>>>>> On 02/07/2011 02:15 AM, Andy Whitcroft wrote:
>>>>>> On Mon, Feb 07, 2011 at 09:48:11AM +0100, Stefan Bader wrote:
>>>>>>> On 02/02/2011 09:43 PM, Tim Gardner wrote:
>>>>>>>> On 02/02/2011 12:11 PM, Tim Gardner wrote:
>>>>>>>>> Postpone this one for a bit. The custom binary openvz flavour is
>>>>>>>>> failing.
>>>>>>>>>
>>>>>>>>> rtg
>>>>>>>>
>>>>>>>> Added the openvz cleanup patch.
>>>>>>>>
>>>>>>>> rtg
>>>>>>>>
>>>>>>> Hm, this one looks odd. The hunk in the openvz patch seems to add the
>>>>>>> "sll->sll_plttype = 0" and that does not seem to be touched by this
>>>>>>> cve.
>>>>>>
>>>>>> That sounds familiar. I think that that comes from the CVE below:
>>>>>>
>>>>>> commit 2c14b0a36cebda5a1fc2c69f2dd01eb73365aa65
>>>>>> Author: Andy Whitcroft<apw@canonical.com>
>>>>>> Date: Tue Feb 1 14:26:24 2011 +0000
>>>>>>
>>>>>> net: packet: fix information leak to userland, CVE-2010-3876
>>>>>>
>>>>>> I suspect that means this extra line should have come in earlier, not
>>>>>> sure if it is worth splitting out now, but in a perfect world I suspect
>>>>>> it should have been. My fault by the looks of it.
>>>>>>
>>>>>> -apw
>>>>>>
>>>>>
>>>>> Yeah, I think it's still OK. It just moves a hunk of a patch around, and
>>>>> the context lines go along for the ride. The custom binary patch
>>>>> mechanism is a real pain.
>>>>>
>>>>> rtg
>>>>
>>>> I fixed 'net: packet: fix information leak to userland, CVE-2010-3876'
>>>> for openvz and pushed +master-next, so you'll need to re-fetch.
>>>>
>>>> Now, back to the original Hardy CVE patch....
>>>
>>> OK, how about this.
>>>
>>> rtg
>>>
>> Probably... mostly... err maybe without the line saying there is a fixup for
>> openvz in the commit message.
>>
>> -Stefan
>>
>
> I've removed the commit log line that references openvz.
>
> rtg
>

Looks to do what it says.

Acked-by: Brad Figg <brad.figg@canonical.com>
Tim Gardner Feb. 9, 2011, 8:36 p.m. UTC | #2 
applied and pushed
diff mbox

Patch

From fa512b5cd928050833fc09b2e50dc387f7a8deed Mon Sep 17 00:00:00 2001
From: Nelson Elhage <nelhage@ksplice.com>
Date: Wed, 3 Nov 2010 16:35:41 +0000
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>
---
 include/net/netlink.h |    2 +-
 net/ipv4/inet_diag.c  |   27 ++++++++++++++++-----------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/net/netlink.h b/include/net/netlink.h
index 9298218..97caec7 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -383,7 +383,7 @@  static inline int nlmsg_parse(struct nlmsghdr *nlh, int hdrlen,
  *
  * Returns the first attribute which matches the specified type.
  */
-static inline struct nlattr *nlmsg_find_attr(struct nlmsghdr *nlh,
+static inline struct nlattr *nlmsg_find_attr(const struct nlmsghdr *nlh,
 					     int hdrlen, int attrtype)
 {
 	return nla_find(nlmsg_attrdata(nlh, hdrlen),
diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 6d2979c..bf49652 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c
@@ -495,9 +495,11 @@  static int inet_csk_diag_dump(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);
+		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;
@@ -517,7 +519,7 @@  static int inet_csk_diag_dump(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;
 	}
 
@@ -532,9 +534,11 @@  static int inet_twsk_diag_dump(struct inet_timewait_sock *tw,
 {
 	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);
+		const struct nlattr *bc = nlmsg_find_attr(cb->nlh,
+							  sizeof(*r),
+							  INET_DIAG_REQ_BYTECODE);
 
 		entry.family = tw->tw_family;
 #if defined(CONFIG_IPV6) || defined (CONFIG_IPV6_MODULE)
@@ -553,7 +557,7 @@  static int inet_twsk_diag_dump(struct inet_timewait_sock *tw,
 		entry.dport = ntohs(tw->tw_dport);
 		entry.userlocks = 0;
 
-		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;
 	}
 
@@ -623,7 +627,7 @@  static int inet_diag_dump_reqs(struct sk_buff *skb, struct sock *sk,
 	struct inet_diag_req *r = NLMSG_DATA(cb->nlh);
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct listen_sock *lopt;
-	struct rtattr *bc = NULL;
+	const struct nlattr *bc = NULL;
 	struct inet_sock *inet = inet_sk(sk);
 	int j, s_j;
 	int reqnum, s_reqnum;
@@ -643,8 +647,9 @@  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;
 	}
@@ -677,8 +682,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;
 			}
 
-- 
1.7.0.4