[PATCH V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

[PATCH V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

[Taehee Yoo]

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(-)

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);
-- 
2.9.3

Re: [PATCH V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

[Florian Westphal]

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).

Re: [PATCH V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

[Pablo Neira Ayuso]

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!

Re: [PATCH V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

[Taehee Yoo]

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.