lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <20100126233924.023394147@mini.kroah.org>
Date:	Tue, 26 Jan 2010 15:33:50 -0800
From:	Greg KH <gregkh@...e.de>
To:	linux-kernel@...r.kernel.org, stable@...nel.org
Cc:	stable-review@...nel.org, torvalds@...ux-foundation.org,
	akpm@...ux-foundation.org, alan@...rguk.ukuu.org.uk,
	Florian Westphal <fw@...len.de>,
	Patrick McHardy <kaber@...sh.net>,
	Ben Hutchings <ben@...adent.org.uk>
Subject: [24/98] netfilter: xtables: fix conntrack match v1 ipt-save output

2.6.32-stable review patch.  If anyone has any objections, please let us know.

------------------

From: Florian Westphal <fw@...len.de>

commit 3a0429292daa0e1ec848bd26479f5e48b0d54a42 upstream.

commit d6d3f08b0fd998b647a05540cedd11a067b72867
(netfilter: xtables: conntrack match revision 2) does break the
v1 conntrack match iptables-save output in a subtle way.

Problem is as follows:

    up = kmalloc(sizeof(*up), GFP_KERNEL);
[..]
   /*
    * The strategy here is to minimize the overhead of v1 matching,
    * by prebuilding a v2 struct and putting the pointer into the
    * v1 dataspace.
    */
    memcpy(up, info, offsetof(typeof(*info), state_mask));
[..]
    *(void **)info  = up;

As the v2 struct pointer is saved in the match data space,
it clobbers the first structure member (->origsrc_addr).

Because the _v1 match function grabs this pointer and does not actually
look at the v1 origsrc, run time functionality does not break.
But iptables -nvL (or iptables-save) cannot know that v1 origsrc_addr
has been overloaded in this way:

$ iptables -p tcp -A OUTPUT -m conntrack --ctorigsrc 10.0.0.1 -j ACCEPT
$ iptables-save
-A OUTPUT -p tcp -m conntrack --ctorigsrc 128.173.134.206 -j ACCEPT

(128.173... is the address to the v2 match structure).

To fix this, we take advantage of the fact that the v1 and v2 structures
are identical with exception of the last two structure members (u8 in v1,
u16 in v2).

We extract them as early as possible and prevent the v2 matching function
from looking at those two members directly.

Previously reported by Michel Messerschmidt via Ben Hutchings, also
see Debian Bug tracker #556587.

Signed-off-by: Florian Westphal <fw@...len.de>
Signed-off-by: Patrick McHardy <kaber@...sh.net>
Cc: Ben Hutchings <ben@...adent.org.uk>
Signed-off-by: Greg Kroah-Hartman <gregkh@...e.de>

---
 net/netfilter/xt_conntrack.c |   61 +++++++++++--------------------------------
 1 file changed, 17 insertions(+), 44 deletions(-)

--- a/net/netfilter/xt_conntrack.c
+++ b/net/netfilter/xt_conntrack.c
@@ -113,7 +113,8 @@ ct_proto_port_check(const struct xt_conn
 }
 
 static bool
-conntrack_mt(const struct sk_buff *skb, const struct xt_match_param *par)
+conntrack_mt(const struct sk_buff *skb, const struct xt_match_param *par,
+             u16 state_mask, u16 status_mask)
 {
 	const struct xt_conntrack_mtinfo2 *info = par->matchinfo;
 	enum ip_conntrack_info ctinfo;
@@ -136,7 +137,7 @@ conntrack_mt(const struct sk_buff *skb, 
 			if (test_bit(IPS_DST_NAT_BIT, &ct->status))
 				statebit |= XT_CONNTRACK_STATE_DNAT;
 		}
-		if (!!(info->state_mask & statebit) ^
+		if (!!(state_mask & statebit) ^
 		    !(info->invert_flags & XT_CONNTRACK_STATE))
 			return false;
 	}
@@ -172,7 +173,7 @@ conntrack_mt(const struct sk_buff *skb, 
 		return false;
 
 	if ((info->match_flags & XT_CONNTRACK_STATUS) &&
-	    (!!(info->status_mask & ct->status) ^
+	    (!!(status_mask & ct->status) ^
 	    !(info->invert_flags & XT_CONNTRACK_STATUS)))
 		return false;
 
@@ -192,11 +193,17 @@ conntrack_mt(const struct sk_buff *skb, 
 static bool
 conntrack_mt_v1(const struct sk_buff *skb, const struct xt_match_param *par)
 {
-	const struct xt_conntrack_mtinfo2 *const *info = par->matchinfo;
-	struct xt_match_param newpar = *par;
+	const struct xt_conntrack_mtinfo1 *info = par->matchinfo;
 
-	newpar.matchinfo = *info;
-	return conntrack_mt(skb, &newpar);
+	return conntrack_mt(skb, par, info->state_mask, info->status_mask);
+}
+
+static bool
+conntrack_mt_v2(const struct sk_buff *skb, const struct xt_match_param *par)
+{
+	const struct xt_conntrack_mtinfo2 *info = par->matchinfo;
+
+	return conntrack_mt(skb, par, info->state_mask, info->status_mask);
 }
 
 static bool conntrack_mt_check(const struct xt_mtchk_param *par)
@@ -209,45 +216,11 @@ static bool conntrack_mt_check(const str
 	return true;
 }
 
-static bool conntrack_mt_check_v1(const struct xt_mtchk_param *par)
-{
-	struct xt_conntrack_mtinfo1 *info = par->matchinfo;
-	struct xt_conntrack_mtinfo2 *up;
-	int ret = conntrack_mt_check(par);
-
-	if (ret < 0)
-		return ret;
-
-	up = kmalloc(sizeof(*up), GFP_KERNEL);
-	if (up == NULL) {
-		nf_ct_l3proto_module_put(par->family);
-		return -ENOMEM;
-	}
-
-	/*
-	 * The strategy here is to minimize the overhead of v1 matching,
-	 * by prebuilding a v2 struct and putting the pointer into the
-	 * v1 dataspace.
-	 */
-	memcpy(up, info, offsetof(typeof(*info), state_mask));
-	up->state_mask  = info->state_mask;
-	up->status_mask = info->status_mask;
-	*(void **)info  = up;
-	return true;
-}
-
 static void conntrack_mt_destroy(const struct xt_mtdtor_param *par)
 {
 	nf_ct_l3proto_module_put(par->family);
 }
 
-static void conntrack_mt_destroy_v1(const struct xt_mtdtor_param *par)
-{
-	struct xt_conntrack_mtinfo2 **info = par->matchinfo;
-	kfree(*info);
-	conntrack_mt_destroy(par);
-}
-
 static struct xt_match conntrack_mt_reg[] __read_mostly = {
 	{
 		.name       = "conntrack",
@@ -255,8 +228,8 @@ static struct xt_match conntrack_mt_reg[
 		.family     = NFPROTO_UNSPEC,
 		.matchsize  = sizeof(struct xt_conntrack_mtinfo1),
 		.match      = conntrack_mt_v1,
-		.checkentry = conntrack_mt_check_v1,
-		.destroy    = conntrack_mt_destroy_v1,
+		.checkentry = conntrack_mt_check,
+		.destroy    = conntrack_mt_destroy,
 		.me         = THIS_MODULE,
 	},
 	{
@@ -264,7 +237,7 @@ static struct xt_match conntrack_mt_reg[
 		.revision   = 2,
 		.family     = NFPROTO_UNSPEC,
 		.matchsize  = sizeof(struct xt_conntrack_mtinfo2),
-		.match      = conntrack_mt,
+		.match      = conntrack_mt_v2,
 		.checkentry = conntrack_mt_check,
 		.destroy    = conntrack_mt_destroy,
 		.me         = THIS_MODULE,


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ