| Message ID | 20170726091648.20639-1-ap420073@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
Taehee Yoo <ap420073@gmail.com> wrote: > If verdict is NF_STOLEN in the SYNPROXY target, > the skb is consumed. > However, ipt_do_table() always tries to get ip header from the skb. > So that, KASAN triggers the use-after-free message. In case anyone wonders, ip6tables doesn't have this problem because we pass *skb, not ip6hdr to ip6_packet_match(). arptables has the same bug, it seems (no target returns STOLEN, but I think we should fix it there as well). -- 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 Wed, Jul 26, 2017 at 11:27:16AM +0200, Florian Westphal wrote: > Taehee Yoo <ap420073@gmail.com> wrote: > > If verdict is NF_STOLEN in the SYNPROXY target, > > the skb is consumed. > > However, ipt_do_table() always tries to get ip header from the skb. > > So that, KASAN triggers the use-after-free message. > > In case anyone wonders, ip6tables doesn't have this problem > because we pass *skb, not ip6hdr to ip6_packet_match(). I think it would be good to make these code converge to what ip6tables is doing while fixing up this? > arptables has the same bug, it seems (no target returns STOLEN, > but I think we should fix it there as well). Yes, even if no target returns what triggers the problem, it's good if we fix this now so we make sure whatever new extension gets in in the future works accordingly. Thanks! -- 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
2017-07-26 20:06 GMT+09:00 Pablo Neira Ayuso <pablo@netfilter.org>: > On Wed, Jul 26, 2017 at 11:27:16AM +0200, Florian Westphal wrote: >> Taehee Yoo <ap420073@gmail.com> wrote: >> > If verdict is NF_STOLEN in the SYNPROXY target, >> > the skb is consumed. >> > However, ipt_do_table() always tries to get ip header from the skb. >> > So that, KASAN triggers the use-after-free message. >> >> In case anyone wonders, ip6tables doesn't have this problem >> because we pass *skb, not ip6hdr to ip6_packet_match(). > > I think it would be good to make these code converge to what ip6tables > is doing while fixing up this? > >> arptables has the same bug, it seems (no target returns STOLEN, >> but I think we should fix it there as well). > > Yes, even if no target returns what triggers the problem, it's good if > we fix this now so we make sure whatever new extension gets in in the > future works accordingly. > > Thanks! Thank you for reviews! I will send the V3 patch that includes modified arpt_do_table() that is reviewed point. -- 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/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index 2a55a40..622ed28 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -352,13 +352,14 @@ ipt_do_table(struct sk_buff *skb, acpar.targinfo = t->data; verdict = t->u.kernel.target->target(skb, &acpar); - /* Target might have changed stuff. */ - ip = ip_hdr(skb); - if (verdict == XT_CONTINUE) + if (verdict == XT_CONTINUE) { + /* Target might have changed stuff. */ + ip = ip_hdr(skb); e = ipt_next_entry(e); - else + } else { /* Verdict */ break; + } } while (!acpar.hotdrop); xt_write_recseq_end(addend);
If verdict is NF_STOLEN in the SYNPROXY target, the skb is consumed. However, ipt_do_table() always tries to get ip header from the skb. So that, KASAN triggers the use-after-free message. We can reproduce this message using below command. # iptables -I INPUT -p tcp -j SYNPROXY --mss 1460 [ 193.542265] BUG: KASAN: use-after-free in ipt_do_table+0x1405/0x1c10 [ ... ] [ 193.578603] Call Trace: [ 193.581590] <IRQ> [ 193.584107] dump_stack+0x68/0xa0 [ 193.588168] print_address_description+0x78/0x290 [ 193.593828] ? ipt_do_table+0x1405/0x1c10 [ 193.598690] kasan_report+0x230/0x340 [ 193.603194] __asan_report_load2_noabort+0x19/0x20 [ 193.608950] ipt_do_table+0x1405/0x1c10 [ 193.613591] ? rcu_read_lock_held+0xae/0xd0 [ 193.618631] ? ip_route_input_rcu+0x27d7/0x4270 [ 193.624348] ? ipt_do_table+0xb68/0x1c10 [ 193.629124] ? do_add_counters+0x620/0x620 [ 193.634234] ? iptable_filter_net_init+0x60/0x60 [ ... ] After this patch, only when verdict is XT_CONTINUE, ipt_do_table() tries to get ip header. Signed-off-by: Taehee Yoo <ap420073@gmail.com> --- V2: - Change commit log message. V1: - Initial Version net/ipv4/netfilter/ip_tables.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)