CVE-2016-3134: Improper Restriction of Operations within the Bounds of a Memory Buffer (CWE-119)
value: The netfilter subsystem in the Linux kernel through 4.5.2 does not validate certain offset fields, which allows local users to gain privileges or cause a denial of service (heap memory corruption) via an IPT_SO_SET_REPLACE setsockopt call.
CWE description: The product performs operations on a memory buffer, but it reads from or writes to a memory location outside the buffer's intended boundary. This may result in read or write operations on unexpected memory locations that could be linked to other variables, data structures, or internal program data.
CVSS v3 base score: 8.4
Published: 2016-04-27T17:59Z
Repository: https://github.com/torvalds/linux
Language: C
Files changed: net/ipv4/netfilter/arp_tables.c, net/ipv4/netfilter/ip_tables.c, net/ipv6/netfilter/ip6_tables.c
Commit date: 2016-03-28 17:59:15 +0200
Commit message: netfilter: x_tables: fix unconditional helper

Ben Hawkes says:

 In the mark_source_chains function (net/ipv4/netfilter/ip_tables.c) it
 is possible for a user-supplied ipt_entry structure to have a large
 next_offset field. This field is not bounds checked prior to writing a
 counter value at the supplied offset.

Problem is that mark_source_chains should not have been called --
the rule doesn't have a next entry, so its supposed to return
an absolute verdict of either ACCEPT or DROP.

However, the function conditional() doesn't work as the name implies.
It only checks that the rule is using wildcard address matching.

However, an unconditional rule must also not be using any matches
(no -m args).

The underflow validator only checked the addresses, therefore
passing the 'unconditional absolute verdict' test, while
mark_source_chains also tested for presence of matches, and thus
proceeeded to the next (not-existent) rule.

Unify this so that all the callers have same idea of 'unconditional rule'.

Reported-by: Ben Hawkes <hawkes@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>

Diff: diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 51d4fe56b807b..a1bb5e7129a2f 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -357,15 +357,16 @@ unsigned int arpt_do_table(struct sk_buff *skb,
 	else
 		return verdict;
 }
 
 /* All zeroes == unconditional rule. */
-static inline bool unconditional(const struct arpt_arp *arp)
+static inline bool unconditional(const struct arpt_entry *e)
 {
 	static const struct arpt_arp uncond;
 
-	return memcmp(arp, &uncond, sizeof(uncond)) == 0;
+	return e->target_offset == sizeof(struct arpt_entry) &&
+	       memcmp(&e->arp, &uncond, sizeof(uncond)) == 0;
 }
 
 /* Figures out from what hook each rule can be called: returns 0 if
  * there are loops.  Puts hook bitmask in comefrom.
  */
@@ -400,15 +401,14 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
 			}
 			e->comefrom
 				|= ((1 << hook) | (1 << NF_ARP_NUMHOOKS));
 
 			/* Unconditional return/END. */
-			if ((e->target_offset == sizeof(struct arpt_entry) &&
+			if ((unconditional(e) &&
 			     (strcmp(t->target.u.user.name,
 				     XT_STANDARD_TARGET) == 0) &&
-			     t->verdict < 0 && unconditional(&e->arp)) ||
-			    visited) {
+			     t->verdict < 0) || visited) {
 				unsigned int oldpos, size;
 
 				if ((strcmp(t->target.u.user.name,
 					    XT_STANDARD_TARGET) == 0) &&
 				    t->verdict < -NF_MAX_VERDICT - 1) {
@@ -549,11 +549,11 @@ find_check_entry(struct arpt_entry *e, const char *name, unsigned int size)
 static bool check_underflow(const struct arpt_entry *e)
 {
 	const struct xt_entry_target *t;
 	unsigned int verdict;
 
-	if (!unconditional(&e->arp))
+	if (!unconditional(e))
 		return false;
 	t = arpt_get_target_c(e);
 	if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
 		return false;
 	verdict = ((struct xt_standard_target *)t)->verdict;
@@ -596,13 +596,13 @@ static inline int check_entry_size_and_hooks(struct arpt_entry *e,
 			continue;
 		if ((unsigned char *)e - base == hook_entries[h])
 			newinfo->hook_entry[h] = hook_entries[h];
 		if ((unsigned char *)e - base == underflows[h]) {
 			if (!check_underflow(e)) {
-				pr_err("Underflows must be unconditional and "
-				       "use the STANDARD target with "
-				       "ACCEPT/DROP\n");
+				pr_debug("Underflows must be unconditional and "
+					 "use the STANDARD target with "
+					 "ACCEPT/DROP\n");
 				return -EINVAL;
 			}
 			newinfo->underflow[h] = underflows[h];
 		}
 	}
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index fb7694e6663e8..89b5d959eda31 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -166,15 +166,16 @@ get_entry(const void *base, unsigned int offset)
 	return (struct ipt_entry *)(base + offset);
 }
 
 /* All zeroes == unconditional rule. */
 /* Mildly perf critical (only if packet tracing is on) */
-static inline bool unconditional(const struct ipt_ip *ip)
+static inline bool unconditional(const struct ipt_entry *e)
 {
 	static const struct ipt_ip uncond;
 
-	return memcmp(ip, &uncond, sizeof(uncond)) == 0;
+	return e->target_offset == sizeof(struct ipt_entry) &&
+	       memcmp(&e->ip, &uncond, sizeof(uncond)) == 0;
 #undef FWINV
 }
 
 /* for const-correctness */
 static inline const struct xt_entry_target *
@@ -227,15 +228,14 @@ get_chainname_rulenum(const struct ipt_entry *s, const struct ipt_entry *e,
 		*chainname = t->target.data;
 		(*rulenum) = 0;
 	} else if (s == e) {
 		(*rulenum)++;
 
-		if (s->target_offset == sizeof(struct ipt_entry) &&
+		if (unconditional(s) &&
 		    strcmp(t->target.u.kernel.target->name,
 			   XT_STANDARD_TARGET) == 0 &&
-		   t->verdict < 0 &&
-		   unconditional(&s->ip)) {
+		   t->verdict < 0) {
 			/* Tail of chains: STANDARD target (return/policy) */
 			*comment = *chainname == hookname
 				? comments[NF_IP_TRACE_COMMENT_POLICY]
 				: comments[NF_IP_TRACE_COMMENT_RETURN];
 		}
@@ -474,15 +474,14 @@ mark_source_chains(const struct xt_table_info *newinfo,
 				return 0;
 			}
 			e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));
 
 			/* Unconditional return/END. */
-			if ((e->target_offset == sizeof(struct ipt_entry) &&
+			if ((unconditional(e) &&
 			     (strcmp(t->target.u.user.name,
 				     XT_STANDARD_TARGET) == 0) &&
-			     t->verdict < 0 && unconditional(&e->ip)) ||
-			    visited) {
+			     t->verdict < 0) || visited) {
 				unsigned int oldpos, size;
 
 				if ((strcmp(t->target.u.user.name,
 					    XT_STANDARD_TARGET) == 0) &&
 				    t->verdict < -NF_MAX_VERDICT - 1) {
@@ -713,11 +712,11 @@ find_check_entry(struct ipt_entry *e, struct net *net, const char *name,
 static bool check_underflow(const struct ipt_entry *e)
 {
 	const struct xt_entry_target *t;
 	unsigned int verdict;
 
-	if (!unconditional(&e->ip))
+	if (!unconditional(e))
 		return false;
 	t = ipt_get_target_c(e);
 	if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
 		return false;
 	verdict = ((struct xt_standard_target *)t)->verdict;
@@ -761,13 +760,13 @@ check_entry_size_and_hooks(struct ipt_entry *e,
 			continue;
 		if ((unsigned char *)e - base == hook_entries[h])
 			newinfo->hook_entry[h] = hook_entries[h];
 		if ((unsigned char *)e - base == underflows[h]) {
 			if (!check_underflow(e)) {
-				pr_err("Underflows must be unconditional and "
-				       "use the STANDARD target with "
-				       "ACCEPT/DROP\n");
+				pr_debug("Underflows must be unconditional and "
+					 "use the STANDARD target with "
+					 "ACCEPT/DROP\n");
 				return -EINVAL;
 			}
 			newinfo->underflow[h] = underflows[h];
 		}
 	}
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index b248528f2a172..541b59f835952 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -196,15 +196,16 @@ get_entry(const void *base, unsigned int offset)
 	return (struct ip6t_entry *)(base + offset);
 }
 
 /* All zeroes == unconditional rule. */
 /* Mildly perf critical (only if packet tracing is on) */
-static inline bool unconditional(const struct ip6t_ip6 *ipv6)
+static inline bool unconditional(const struct ip6t_entry *e)
 {
 	static const struct ip6t_ip6 uncond;
 
-	return memcmp(ipv6, &uncond, sizeof(uncond)) == 0;
+	return e->target_offset == sizeof(struct ip6t_entry) &&
+	       memcmp(&e->ipv6, &uncond, sizeof(uncond)) == 0;
 }
 
 static inline const struct xt_entry_target *
 ip6t_get_target_c(const struct ip6t_entry *e)
 {
@@ -256,15 +257,14 @@ get_chainname_rulenum(const struct ip6t_entry *s, const struct ip6t_entry *e,
 		*chainname = t->target.data;
 		(*rulenum) = 0;
 	} else if (s == e) {
 		(*rulenum)++;
 
-		if (s->target_offset == sizeof(struct ip6t_entry) &&
+		if (unconditional(s) &&
 		    strcmp(t->target.u.kernel.target->name,
 			   XT_STANDARD_TARGET) == 0 &&
-		    t->verdict < 0 &&
-		    unconditional(&s->ipv6)) {
+		    t->verdict < 0) {
 			/* Tail of chains: STANDARD target (return/policy) */
 			*comment = *chainname == hookname
 				? comments[NF_IP6_TRACE_COMMENT_POLICY]
 				: comments[NF_IP6_TRACE_COMMENT_RETURN];
 		}
@@ -486,15 +486,14 @@ mark_source_chains(const struct xt_table_info *newinfo,
 				return 0;
 			}
 			e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));
 
 			/* Unconditional return/END. */
-			if ((e->target_offset == sizeof(struct ip6t_entry) &&
+			if ((unconditional(e) &&
 			     (strcmp(t->target.u.user.name,
 				     XT_STANDARD_TARGET) == 0) &&
-			     t->verdict < 0 &&
-			     unconditional(&e->ipv6)) || visited) {
+			     t->verdict < 0) || visited) {
 				unsigned int oldpos, size;
 
 				if ((strcmp(t->target.u.user.name,
 					    XT_STANDARD_TARGET) == 0) &&
 				    t->verdict < -NF_MAX_VERDICT - 1) {
@@ -725,11 +724,11 @@ find_check_entry(struct ip6t_entry *e, struct net *net, const char *name,
 static bool check_underflow(const struct ip6t_entry *e)
 {
 	const struct xt_entry_target *t;
 	unsigned int verdict;
 
-	if (!unconditional(&e->ipv6))
+	if (!unconditional(e))
 		return false;
 	t = ip6t_get_target_c(e);
 	if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
 		return false;
 	verdict = ((struct xt_standard_target *)t)->verdict;
@@ -773,13 +772,13 @@ check_entry_size_and_hooks(struct ip6t_entry *e,
 			continue;
 		if ((unsigned char *)e - base == hook_entries[h])
 			newinfo->hook_entry[h] = hook_entries[h];
 		if ((unsigned char *)e - base == underflows[h]) {
 			if (!check_underflow(e)) {
-				pr_err("Underflows must be unconditional and "
-				       "use the STANDARD target with "
-				       "ACCEPT/DROP\n");
+				pr_debug("Underflows must be unconditional and "
+					 "use the STANDARD target with "
+					 "ACCEPT/DROP\n");
 				return -EINVAL;
 			}
 			newinfo->underflow[h] = underflows[h];
 		}
 	}

Vulnerable code: static inline bool unconditional(const struct arpt_arp *arp)
	return memcmp(arp, &uncond, sizeof(uncond)) == 0;
			if ((e->target_offset == sizeof(struct arpt_entry) &&
			     t->verdict < 0 && unconditional(&e->arp)) ||
			    visited) {
	if (!unconditional(&e->arp))
				pr_err("Underflows must be unconditional and "
				       "use the STANDARD target with "
				       "ACCEPT/DROP\n");
static inline bool unconditional(const struct ipt_ip *ip)
	return memcmp(ip, &uncond, sizeof(uncond)) == 0;
		if (s->target_offset == sizeof(struct ipt_entry) &&
		   t->verdict < 0 &&
		   unconditional(&s->ip)) {
			if ((e->target_offset == sizeof(struct ipt_entry) &&
			     t->verdict < 0 && unconditional(&e->ip)) ||
			    visited) {
	if (!unconditional(&e->ip))
				pr_err("Underflows must be unconditional and "
				       "use the STANDARD target with "
				       "ACCEPT/DROP\n");
static inline bool unconditional(const struct ip6t_ip6 *ipv6)
	return memcmp(ipv6, &uncond, sizeof(uncond)) == 0;
		if (s->target_offset == sizeof(struct ip6t_entry) &&
		    t->verdict < 0 &&
		    unconditional(&s->ipv6)) {
			if ((e->target_offset == sizeof(struct ip6t_entry) &&
			     t->verdict < 0 &&
			     unconditional(&e->ipv6)) || visited) {
	if (!unconditional(&e->ipv6))
				pr_err("Underflows must be unconditional and "
				       "use the STANDARD target with "
				       "ACCEPT/DROP\n");
Fixed code: static inline bool unconditional(const struct arpt_entry *e)
	return e->target_offset == sizeof(struct arpt_entry) &&
	       memcmp(&e->arp, &uncond, sizeof(uncond)) == 0;
			if ((unconditional(e) &&
			     t->verdict < 0) || visited) {
	if (!unconditional(e))
				pr_debug("Underflows must be unconditional and "
					 "use the STANDARD target with "
					 "ACCEPT/DROP\n");
static inline bool unconditional(const struct ipt_entry *e)
	return e->target_offset == sizeof(struct ipt_entry) &&
	       memcmp(&e->ip, &uncond, sizeof(uncond)) == 0;
		if (unconditional(s) &&
		   t->verdict < 0) {
			if ((unconditional(e) &&
			     t->verdict < 0) || visited) {
	if (!unconditional(e))
				pr_debug("Underflows must be unconditional and "
					 "use the STANDARD target with "
					 "ACCEPT/DROP\n");
static inline bool unconditional(const struct ip6t_entry *e)
	return e->target_offset == sizeof(struct ip6t_entry) &&
	       memcmp(&e->ipv6, &uncond, sizeof(uncond)) == 0;
		if (unconditional(s) &&
		    t->verdict < 0) {
			if ((unconditional(e) &&
			     t->verdict < 0) || visited) {
	if (!unconditional(e))
				pr_debug("Underflows must be unconditional and "
					 "use the STANDARD target with "
					 "ACCEPT/DROP\n");