| Message ID | 1457642646-17454-1-git-send-email-chris.j.arges@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 10, 2016 at 02:44:06PM -0600, Chris J Arges wrote: > From: Florian Westphal <fw@strlen.de> > > BugLink: http://bugs.launchpad.net/bugs/1555338 > > 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 xt_entry_foreach() macro stops iterating once e->next_offset > is out of bounds, assuming this is the last entry. > > With malformed data thats not necessarily the case so we can > write outside of allocated area later as we might not have walked the > entire blob. > > Fix this by simplifying mark_source_chains -- it already has to check > if nextoff is in range to catch invalid jumps, so just do the check > when we move to a next entry as well. > > Also, check that the offset meets the xtables_entry alignment. > > Reported-by: Ben Hawkes <hawkes@google.com> > Signed-off-by: Florian Westphal <fw@strlen.de> > Signed-off-by: Chris J Arges <chris.j.arges@canonical.com> > --- > net/ipv4/netfilter/arp_tables.c | 25 +++++++++++++++++-------- > net/ipv4/netfilter/ip_tables.c | 24 +++++++++++++++++------- > net/ipv6/netfilter/ip6_tables.c | 24 +++++++++++++++++------- > 3 files changed, 51 insertions(+), 22 deletions(-) > > diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c > index 59da7cd..88334b1 100644 > --- a/net/ipv4/netfilter/arp_tables.c > +++ b/net/ipv4/netfilter/arp_tables.c > @@ -362,6 +362,17 @@ static inline bool unconditional(const struct arpt_arp *arp) > return memcmp(arp, &uncond, sizeof(uncond)) == 0; > } > > +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos) > +{ > + if (newpos > t->size - sizeof(struct arpt_entry)) > + return false; > + > + if (newpos % __alignof__(struct arpt_entry) != 0) > + return false; > + > + return true; > +} > + > /* Figures out from what hook each rule can be called: returns 0 if > * there are loops. Puts hook bitmask in comefrom. > */ > @@ -433,6 +444,8 @@ static int mark_source_chains(const struct xt_table_info *newinfo, > > /* Move along one */ > size = e->next_offset; > + if (!next_offset_ok(newinfo, pos + size)) > + return 0; > e = (struct arpt_entry *) > (entry0 + pos + size); > e->counters.pcnt = pos; > @@ -443,14 +456,6 @@ static int mark_source_chains(const struct xt_table_info *newinfo, > if (strcmp(t->target.u.user.name, > XT_STANDARD_TARGET) == 0 && > newpos >= 0) { > - if (newpos > newinfo->size - > - sizeof(struct arpt_entry)) { > - duprintf("mark_source_chains: " > - "bad verdict (%i)\n", > - newpos); > - return 0; > - } > - > /* This a jump; chase it. */ > duprintf("Jump rule %u -> %u\n", > pos, newpos); > @@ -458,6 +463,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo, > /* ... this is a fallthru */ > newpos = pos + e->next_offset; > } > + > + if (!next_offset_ok(newinfo, newpos)) > + return 0; > + > e = (struct arpt_entry *) > (entry0 + newpos); > e->counters.pcnt = pos; > diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c > index 718dfbd..ee17b65 100644 > --- a/net/ipv4/netfilter/ip_tables.c > +++ b/net/ipv4/netfilter/ip_tables.c > @@ -439,6 +439,17 @@ ipt_do_table(struct sk_buff *skb, > #endif > } > > +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos) > +{ > + if (newpos > t->size - sizeof(struct ipt_entry)) > + return false; > + > + if (newpos % __alignof__(struct ipt_entry) != 0) > + return false; > + > + return true; > +} > + > /* Figures out from what hook each rule can be called: returns 0 if > there are loops. Puts hook bitmask in comefrom. */ > static int > @@ -515,6 +526,8 @@ mark_source_chains(const struct xt_table_info *newinfo, > > /* Move along one */ > size = e->next_offset; > + if (!next_offset_ok(newinfo, pos + size)) > + return 0; > e = (struct ipt_entry *) > (entry0 + pos + size); > e->counters.pcnt = pos; > @@ -525,13 +538,6 @@ mark_source_chains(const struct xt_table_info *newinfo, > if (strcmp(t->target.u.user.name, > XT_STANDARD_TARGET) == 0 && > newpos >= 0) { > - if (newpos > newinfo->size - > - sizeof(struct ipt_entry)) { > - duprintf("mark_source_chains: " > - "bad verdict (%i)\n", > - newpos); > - return 0; > - } > /* This a jump; chase it. */ > duprintf("Jump rule %u -> %u\n", > pos, newpos); > @@ -539,6 +545,10 @@ mark_source_chains(const struct xt_table_info *newinfo, > /* ... this is a fallthru */ > newpos = pos + e->next_offset; > } > + > + if (!next_offset_ok(newinfo, newpos)) > + return 0; > + > e = (struct ipt_entry *) > (entry0 + newpos); > e->counters.pcnt = pos; > diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c > index 710238f..5b4b008 100644 > --- a/net/ipv6/netfilter/ip6_tables.c > +++ b/net/ipv6/netfilter/ip6_tables.c > @@ -449,6 +449,17 @@ ip6t_do_table(struct sk_buff *skb, > #endif > } > > +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos) > +{ > + if (newpos > t->size - sizeof(struct ip6t_entry)) > + return false; > + > + if (newpos % __alignof__(struct ip6t_entry) != 0) > + return false; > + > + return true; > +} > + > /* Figures out from what hook each rule can be called: returns 0 if > there are loops. Puts hook bitmask in comefrom. */ > static int > @@ -525,6 +536,8 @@ mark_source_chains(const struct xt_table_info *newinfo, > > /* Move along one */ > size = e->next_offset; > + if (!next_offset_ok(newinfo, pos + size)) > + return 0; > e = (struct ip6t_entry *) > (entry0 + pos + size); > e->counters.pcnt = pos; > @@ -535,13 +548,6 @@ mark_source_chains(const struct xt_table_info *newinfo, > if (strcmp(t->target.u.user.name, > XT_STANDARD_TARGET) == 0 && > newpos >= 0) { > - if (newpos > newinfo->size - > - sizeof(struct ip6t_entry)) { > - duprintf("mark_source_chains: " > - "bad verdict (%i)\n", > - newpos); > - return 0; > - } > /* This a jump; chase it. */ > duprintf("Jump rule %u -> %u\n", > pos, newpos); > @@ -549,6 +555,10 @@ mark_source_chains(const struct xt_table_info *newinfo, > /* ... this is a fallthru */ > newpos = pos + e->next_offset; > } > + > + if (!next_offset_ok(newinfo, newpos)) > + return 0; > + > e = (struct ip6t_entry *) > (entry0 + newpos); > e->counters.pcnt = pos; > -- > 2.7.0 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team Looks good
From IRC chatter I gather that you had a reproducer. Care to dump your test method and results in the bug ?
diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index 59da7cd..88334b1 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c @@ -362,6 +362,17 @@ static inline bool unconditional(const struct arpt_arp *arp) return memcmp(arp, &uncond, sizeof(uncond)) == 0; } +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos) +{ + if (newpos > t->size - sizeof(struct arpt_entry)) + return false; + + if (newpos % __alignof__(struct arpt_entry) != 0) + return false; + + return true; +} + /* Figures out from what hook each rule can be called: returns 0 if * there are loops. Puts hook bitmask in comefrom. */ @@ -433,6 +444,8 @@ static int mark_source_chains(const struct xt_table_info *newinfo, /* Move along one */ size = e->next_offset; + if (!next_offset_ok(newinfo, pos + size)) + return 0; e = (struct arpt_entry *) (entry0 + pos + size); e->counters.pcnt = pos; @@ -443,14 +456,6 @@ static int mark_source_chains(const struct xt_table_info *newinfo, if (strcmp(t->target.u.user.name, XT_STANDARD_TARGET) == 0 && newpos >= 0) { - if (newpos > newinfo->size - - sizeof(struct arpt_entry)) { - duprintf("mark_source_chains: " - "bad verdict (%i)\n", - newpos); - return 0; - } - /* This a jump; chase it. */ duprintf("Jump rule %u -> %u\n", pos, newpos); @@ -458,6 +463,10 @@ static int mark_source_chains(const struct xt_table_info *newinfo, /* ... this is a fallthru */ newpos = pos + e->next_offset; } + + if (!next_offset_ok(newinfo, newpos)) + return 0; + e = (struct arpt_entry *) (entry0 + newpos); e->counters.pcnt = pos; diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index 718dfbd..ee17b65 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -439,6 +439,17 @@ ipt_do_table(struct sk_buff *skb, #endif } +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos) +{ + if (newpos > t->size - sizeof(struct ipt_entry)) + return false; + + if (newpos % __alignof__(struct ipt_entry) != 0) + return false; + + return true; +} + /* Figures out from what hook each rule can be called: returns 0 if there are loops. Puts hook bitmask in comefrom. */ static int @@ -515,6 +526,8 @@ mark_source_chains(const struct xt_table_info *newinfo, /* Move along one */ size = e->next_offset; + if (!next_offset_ok(newinfo, pos + size)) + return 0; e = (struct ipt_entry *) (entry0 + pos + size); e->counters.pcnt = pos; @@ -525,13 +538,6 @@ mark_source_chains(const struct xt_table_info *newinfo, if (strcmp(t->target.u.user.name, XT_STANDARD_TARGET) == 0 && newpos >= 0) { - if (newpos > newinfo->size - - sizeof(struct ipt_entry)) { - duprintf("mark_source_chains: " - "bad verdict (%i)\n", - newpos); - return 0; - } /* This a jump; chase it. */ duprintf("Jump rule %u -> %u\n", pos, newpos); @@ -539,6 +545,10 @@ mark_source_chains(const struct xt_table_info *newinfo, /* ... this is a fallthru */ newpos = pos + e->next_offset; } + + if (!next_offset_ok(newinfo, newpos)) + return 0; + e = (struct ipt_entry *) (entry0 + newpos); e->counters.pcnt = pos; diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index 710238f..5b4b008 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -449,6 +449,17 @@ ip6t_do_table(struct sk_buff *skb, #endif } +static bool next_offset_ok(const struct xt_table_info *t, unsigned int newpos) +{ + if (newpos > t->size - sizeof(struct ip6t_entry)) + return false; + + if (newpos % __alignof__(struct ip6t_entry) != 0) + return false; + + return true; +} + /* Figures out from what hook each rule can be called: returns 0 if there are loops. Puts hook bitmask in comefrom. */ static int @@ -525,6 +536,8 @@ mark_source_chains(const struct xt_table_info *newinfo, /* Move along one */ size = e->next_offset; + if (!next_offset_ok(newinfo, pos + size)) + return 0; e = (struct ip6t_entry *) (entry0 + pos + size); e->counters.pcnt = pos; @@ -535,13 +548,6 @@ mark_source_chains(const struct xt_table_info *newinfo, if (strcmp(t->target.u.user.name, XT_STANDARD_TARGET) == 0 && newpos >= 0) { - if (newpos > newinfo->size - - sizeof(struct ip6t_entry)) { - duprintf("mark_source_chains: " - "bad verdict (%i)\n", - newpos); - return 0; - } /* This a jump; chase it. */ duprintf("Jump rule %u -> %u\n", pos, newpos); @@ -549,6 +555,10 @@ mark_source_chains(const struct xt_table_info *newinfo, /* ... this is a fallthru */ newpos = pos + e->next_offset; } + + if (!next_offset_ok(newinfo, newpos)) + return 0; + e = (struct ip6t_entry *) (entry0 + newpos); e->counters.pcnt = pos;