| Message ID | 20180309132731.22082-1-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf,v5] netfilter: bridge: ebt_among: add more missing match size checks | expand |
On 03/09/2018 05:27 AM, Florian Westphal wrote: > ebt_among is special, it has a dynamic match size and is exempt > from the central size checks. > > commit c4585a2823edf ("bridge: ebt_among: add missing match size checks") > added validation for pool size, but missed fact that the macros > ebt_among_wh_src/dst can already return out-of-bound result because > they do not check value of wh_src/dst_ofs (an offset) vs. the size > of the match that userspace gave to us. > > v2: > check that offset has correct alignment. > Paolo Abeni points out that we should also check that src/dst > wormhash arrays do not overlap, and src + length lines up with > start of dst (or vice versa). > v3: compact wormhash_sizes_valid() part > > NB: Fixes tag is intentionally wrong, this bug exists from day > one when match was added for 2.6 kernel. Tag is there so stable > maintainers will notice this one too. > > Tested with same rules from the earlier patch. > > Fixes: c4585a2823edf ("bridge: ebt_among: add missing match size checks") > Reported-by: <syzbot+bdabab6f1983a03fc009@syzkaller.appspotmail.com> > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > v5: this is really v3 again, but with explicit (int)sizeof() > cast rather than use of temporary 'int minsize'. > objdump shows identical output for v3/v4/v5. SGTM, thanks Florian ;) Reviewed-by: Eric Dumazet <edumazet@google.com> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Mar 09, 2018 at 02:27:31PM +0100, Florian Westphal wrote: > ebt_among is special, it has a dynamic match size and is exempt > from the central size checks. Applied, thanks Florian. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/netfilter/ebt_among.c b/net/bridge/netfilter/ebt_among.c index c5afb4232ecb..620e54f08296 100644 --- a/net/bridge/netfilter/ebt_among.c +++ b/net/bridge/netfilter/ebt_among.c @@ -177,6 +177,28 @@ static bool poolsize_invalid(const struct ebt_mac_wormhash *w) return w && w->poolsize >= (INT_MAX / sizeof(struct ebt_mac_wormhash_tuple)); } +static bool wormhash_offset_invalid(int off, unsigned int len) +{ + if (off == 0) /* not present */ + return false; + + if (off < (int)sizeof(struct ebt_among_info) || + off % __alignof__(struct ebt_mac_wormhash)) + return true; + + off += sizeof(struct ebt_mac_wormhash); + + return off > len; +} + +static bool wormhash_sizes_valid(const struct ebt_mac_wormhash *wh, int a, int b) +{ + if (a == 0) + a = sizeof(struct ebt_among_info); + + return ebt_mac_wormhash_size(wh) + a == b; +} + static int ebt_among_mt_check(const struct xt_mtchk_param *par) { const struct ebt_among_info *info = par->matchinfo; @@ -189,6 +211,10 @@ static int ebt_among_mt_check(const struct xt_mtchk_param *par) if (expected_length > em->match_size) return -EINVAL; + if (wormhash_offset_invalid(info->wh_dst_ofs, em->match_size) || + wormhash_offset_invalid(info->wh_src_ofs, em->match_size)) + return -EINVAL; + wh_dst = ebt_among_wh_dst(info); if (poolsize_invalid(wh_dst)) return -EINVAL; @@ -201,6 +227,14 @@ static int ebt_among_mt_check(const struct xt_mtchk_param *par) if (poolsize_invalid(wh_src)) return -EINVAL; + if (info->wh_src_ofs < info->wh_dst_ofs) { + if (!wormhash_sizes_valid(wh_src, info->wh_src_ofs, info->wh_dst_ofs)) + return -EINVAL; + } else { + if (!wormhash_sizes_valid(wh_dst, info->wh_dst_ofs, info->wh_src_ofs)) + return -EINVAL; + } + expected_length += ebt_mac_wormhash_size(wh_src); if (em->match_size != EBT_ALIGN(expected_length)) {
ebt_among is special, it has a dynamic match size and is exempt from the central size checks. commit c4585a2823edf ("bridge: ebt_among: add missing match size checks") added validation for pool size, but missed fact that the macros ebt_among_wh_src/dst can already return out-of-bound result because they do not check value of wh_src/dst_ofs (an offset) vs. the size of the match that userspace gave to us. v2: check that offset has correct alignment. Paolo Abeni points out that we should also check that src/dst wormhash arrays do not overlap, and src + length lines up with start of dst (or vice versa). v3: compact wormhash_sizes_valid() part NB: Fixes tag is intentionally wrong, this bug exists from day one when match was added for 2.6 kernel. Tag is there so stable maintainers will notice this one too. Tested with same rules from the earlier patch. Fixes: c4585a2823edf ("bridge: ebt_among: add missing match size checks") Reported-by: <syzbot+bdabab6f1983a03fc009@syzkaller.appspotmail.com> Signed-off-by: Florian Westphal <fw@strlen.de> --- v5: this is really v3 again, but with explicit (int)sizeof() cast rather than use of temporary 'int minsize'. objdump shows identical output for v3/v4/v5. net/bridge/netfilter/ebt_among.c | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+)