diff mbox

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

Message ID 4D49C1DE.3070206@canonical.com
State Accepted
Delegated to: Stefan Bader
Headers show

Commit Message

Tim Gardner Feb. 2, 2011, 8:43 p.m. UTC 
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

Comments

Stefan Bader Feb. 7, 2011, 8:48 a.m. UTC | #1 
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.

-Stefan
Andy Whitcroft Feb. 7, 2011, 9:15 a.m. UTC | #2 
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
Tim Gardner Feb. 7, 2011, 2:38 p.m. UTC | #3 
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
Tim Gardner Feb. 9, 2011, 2:08 p.m. UTC | #4 
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....
diff mbox

Patch

From 9d2be2f63b548b635daef35d7ffaef192b4a9a07 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

openvz: Fixup after 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>
---
 .../openvz/patchset/0001-2.6.24-ovz002.patch       |   24 ++++++++++-------
 include/net/netlink.h                              |    2 +-
 net/ipv4/inet_diag.c                               |   27 +++++++++++--------
 3 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
index 0bde9f8..497fb92 100644
--- a/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
+++ b/debian/binary-custom.d/openvz/patchset/0001-2.6.24-ovz002.patch
@@ -90152,7 +90152,7 @@  Index: kernel/net/packet/af_packet.c
  out:
  	return err;
  }
-@@ -1140,7 +1156,7 @@
+@@ -1143,7 +1159,7 @@
  		return -EOPNOTSUPP;
  
  	uaddr->sa_family = AF_PACKET;
@@ -90161,15 +90161,6 @@  Index: kernel/net/packet/af_packet.c
  	if (dev) {
  		strlcpy(uaddr->sa_data, dev->name, 15);
  		dev_put(dev);
-@@ -1165,7 +1181,7 @@
- 	sll->sll_family = AF_PACKET;
- 	sll->sll_ifindex = po->ifindex;
- 	sll->sll_protocol = po->num;
--	dev = dev_get_by_index(&init_net, po->ifindex);
-+	dev = dev_get_by_index(current->nsproxy->net_ns, po->ifindex);
- 	if (dev) {
- 		sll->sll_hatype = dev->type;
- 		sll->sll_halen = dev->addr_len;
 @@ -1217,7 +1233,7 @@
  	rtnl_lock();
  
@@ -91583,3 +91574,16 @@  Index: kernel/sound/core/info.c
  	}
  	return 0;
  }
+diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
+index c23c539..795d798 100644
+--- a/net/packet/af_packet.c
++++ b/net/packet/af_packet.c
+@@ -1182,7 +1182,7 @@ static int packet_getname(struct socket *sock, struct sockaddr *uaddr,
+ 	sll->sll_ifindex = po->ifindex;
+ 	sll->sll_protocol = po->num;
+ 	sll->sll_pkttype = 0;
+-	dev = dev_get_by_index(&init_net, po->ifindex);
++	dev = dev_get_by_index(current->nsproxy->net_ns, po->ifindex);
+ 	if (dev) {
+ 		sll->sll_hatype = dev->type;
+ 		sll->sll_halen = dev->addr_len;
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