Skip to content

Commit 54d83fc

Browse files
Florian Westphalummakynes
Florian Westphal
authored andcommitted
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>
1 parent 6e94e0c commit 54d83fc

File tree

3 files changed

+31
-33
lines changed

3 files changed

+31
-33
lines changed

Diff for: net/ipv4/netfilter/arp_tables.c

+9-9
Original file line numberDiff line numberDiff line change
@@ -359,11 +359,12 @@ unsigned int arpt_do_table(struct sk_buff *skb,
359359
}
360360

361361
/* All zeroes == unconditional rule. */
362-
static inline bool unconditional(const struct arpt_arp *arp)
362+
static inline bool unconditional(const struct arpt_entry *e)
363363
{
364364
static const struct arpt_arp uncond;
365365

366-
return memcmp(arp, &uncond, sizeof(uncond)) == 0;
366+
return e->target_offset == sizeof(struct arpt_entry) &&
367+
memcmp(&e->arp, &uncond, sizeof(uncond)) == 0;
367368
}
368369

369370
/* Figures out from what hook each rule can be called: returns 0 if
@@ -402,11 +403,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo,
402403
|= ((1 << hook) | (1 << NF_ARP_NUMHOOKS));
403404

404405
/* Unconditional return/END. */
405-
if ((e->target_offset == sizeof(struct arpt_entry) &&
406+
if ((unconditional(e) &&
406407
(strcmp(t->target.u.user.name,
407408
XT_STANDARD_TARGET) == 0) &&
408-
t->verdict < 0 && unconditional(&e->arp)) ||
409-
visited) {
409+
t->verdict < 0) || visited) {
410410
unsigned int oldpos, size;
411411

412412
if ((strcmp(t->target.u.user.name,
@@ -551,7 +551,7 @@ static bool check_underflow(const struct arpt_entry *e)
551551
const struct xt_entry_target *t;
552552
unsigned int verdict;
553553

554-
if (!unconditional(&e->arp))
554+
if (!unconditional(e))
555555
return false;
556556
t = arpt_get_target_c(e);
557557
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
@@ -598,9 +598,9 @@ static inline int check_entry_size_and_hooks(struct arpt_entry *e,
598598
newinfo->hook_entry[h] = hook_entries[h];
599599
if ((unsigned char *)e - base == underflows[h]) {
600600
if (!check_underflow(e)) {
601-
pr_err("Underflows must be unconditional and "
602-
"use the STANDARD target with "
603-
"ACCEPT/DROP\n");
601+
pr_debug("Underflows must be unconditional and "
602+
"use the STANDARD target with "
603+
"ACCEPT/DROP\n");
604604
return -EINVAL;
605605
}
606606
newinfo->underflow[h] = underflows[h];

Diff for: net/ipv4/netfilter/ip_tables.c

+11-12
Original file line numberDiff line numberDiff line change
@@ -168,11 +168,12 @@ get_entry(const void *base, unsigned int offset)
168168

169169
/* All zeroes == unconditional rule. */
170170
/* Mildly perf critical (only if packet tracing is on) */
171-
static inline bool unconditional(const struct ipt_ip *ip)
171+
static inline bool unconditional(const struct ipt_entry *e)
172172
{
173173
static const struct ipt_ip uncond;
174174

175-
return memcmp(ip, &uncond, sizeof(uncond)) == 0;
175+
return e->target_offset == sizeof(struct ipt_entry) &&
176+
memcmp(&e->ip, &uncond, sizeof(uncond)) == 0;
176177
#undef FWINV
177178
}
178179

@@ -229,11 +230,10 @@ get_chainname_rulenum(const struct ipt_entry *s, const struct ipt_entry *e,
229230
} else if (s == e) {
230231
(*rulenum)++;
231232

232-
if (s->target_offset == sizeof(struct ipt_entry) &&
233+
if (unconditional(s) &&
233234
strcmp(t->target.u.kernel.target->name,
234235
XT_STANDARD_TARGET) == 0 &&
235-
t->verdict < 0 &&
236-
unconditional(&s->ip)) {
236+
t->verdict < 0) {
237237
/* Tail of chains: STANDARD target (return/policy) */
238238
*comment = *chainname == hookname
239239
? comments[NF_IP_TRACE_COMMENT_POLICY]
@@ -476,11 +476,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
476476
e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));
477477

478478
/* Unconditional return/END. */
479-
if ((e->target_offset == sizeof(struct ipt_entry) &&
479+
if ((unconditional(e) &&
480480
(strcmp(t->target.u.user.name,
481481
XT_STANDARD_TARGET) == 0) &&
482-
t->verdict < 0 && unconditional(&e->ip)) ||
483-
visited) {
482+
t->verdict < 0) || visited) {
484483
unsigned int oldpos, size;
485484

486485
if ((strcmp(t->target.u.user.name,
@@ -715,7 +714,7 @@ static bool check_underflow(const struct ipt_entry *e)
715714
const struct xt_entry_target *t;
716715
unsigned int verdict;
717716

718-
if (!unconditional(&e->ip))
717+
if (!unconditional(e))
719718
return false;
720719
t = ipt_get_target_c(e);
721720
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
@@ -763,9 +762,9 @@ check_entry_size_and_hooks(struct ipt_entry *e,
763762
newinfo->hook_entry[h] = hook_entries[h];
764763
if ((unsigned char *)e - base == underflows[h]) {
765764
if (!check_underflow(e)) {
766-
pr_err("Underflows must be unconditional and "
767-
"use the STANDARD target with "
768-
"ACCEPT/DROP\n");
765+
pr_debug("Underflows must be unconditional and "
766+
"use the STANDARD target with "
767+
"ACCEPT/DROP\n");
769768
return -EINVAL;
770769
}
771770
newinfo->underflow[h] = underflows[h];

Diff for: net/ipv6/netfilter/ip6_tables.c

+11-12
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,12 @@ get_entry(const void *base, unsigned int offset)
198198

199199
/* All zeroes == unconditional rule. */
200200
/* Mildly perf critical (only if packet tracing is on) */
201-
static inline bool unconditional(const struct ip6t_ip6 *ipv6)
201+
static inline bool unconditional(const struct ip6t_entry *e)
202202
{
203203
static const struct ip6t_ip6 uncond;
204204

205-
return memcmp(ipv6, &uncond, sizeof(uncond)) == 0;
205+
return e->target_offset == sizeof(struct ip6t_entry) &&
206+
memcmp(&e->ipv6, &uncond, sizeof(uncond)) == 0;
206207
}
207208

208209
static inline const struct xt_entry_target *
@@ -258,11 +259,10 @@ get_chainname_rulenum(const struct ip6t_entry *s, const struct ip6t_entry *e,
258259
} else if (s == e) {
259260
(*rulenum)++;
260261

261-
if (s->target_offset == sizeof(struct ip6t_entry) &&
262+
if (unconditional(s) &&
262263
strcmp(t->target.u.kernel.target->name,
263264
XT_STANDARD_TARGET) == 0 &&
264-
t->verdict < 0 &&
265-
unconditional(&s->ipv6)) {
265+
t->verdict < 0) {
266266
/* Tail of chains: STANDARD target (return/policy) */
267267
*comment = *chainname == hookname
268268
? comments[NF_IP6_TRACE_COMMENT_POLICY]
@@ -488,11 +488,10 @@ mark_source_chains(const struct xt_table_info *newinfo,
488488
e->comefrom |= ((1 << hook) | (1 << NF_INET_NUMHOOKS));
489489

490490
/* Unconditional return/END. */
491-
if ((e->target_offset == sizeof(struct ip6t_entry) &&
491+
if ((unconditional(e) &&
492492
(strcmp(t->target.u.user.name,
493493
XT_STANDARD_TARGET) == 0) &&
494-
t->verdict < 0 &&
495-
unconditional(&e->ipv6)) || visited) {
494+
t->verdict < 0) || visited) {
496495
unsigned int oldpos, size;
497496

498497
if ((strcmp(t->target.u.user.name,
@@ -727,7 +726,7 @@ static bool check_underflow(const struct ip6t_entry *e)
727726
const struct xt_entry_target *t;
728727
unsigned int verdict;
729728

730-
if (!unconditional(&e->ipv6))
729+
if (!unconditional(e))
731730
return false;
732731
t = ip6t_get_target_c(e);
733732
if (strcmp(t->u.user.name, XT_STANDARD_TARGET) != 0)
@@ -775,9 +774,9 @@ check_entry_size_and_hooks(struct ip6t_entry *e,
775774
newinfo->hook_entry[h] = hook_entries[h];
776775
if ((unsigned char *)e - base == underflows[h]) {
777776
if (!check_underflow(e)) {
778-
pr_err("Underflows must be unconditional and "
779-
"use the STANDARD target with "
780-
"ACCEPT/DROP\n");
777+
pr_debug("Underflows must be unconditional and "
778+
"use the STANDARD target with "
779+
"ACCEPT/DROP\n");
781780
return -EINVAL;
782781
}
783782
newinfo->underflow[h] = underflows[h];

0 commit comments

Comments
 (0)