Re: [PATCH net 1/3] net: sctp: fix skb_over_panic when receiving malformed ASCONF chunks

[Neil Horman <nhorman@tuxdriver.com>]

On Thu, Oct 09, 2014 at 10:55:31PM +0200, Daniel Borkmann wrote:
> Commit 6f4c618ddb0 ("SCTP : Add paramters validity check for
> ASCONF chunk") added basic verification of ASCONF chunks, however,
> it is still possible to remotely crash a server by sending a
> special crafted ASCONF chunk, even up to pre 2.6.12 kernels:
> 
> skb_over_panic: text:ffffffffa01ea1c3 len:31056 put:30768
>  head:ffff88011bd81800 data:ffff88011bd81800 tail:0x7950
>  end:0x440 dev:<NULL>
>  ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:129!
> [...]
> Call Trace:
>  <IRQ>
>  [<ffffffff8144fb1c>] skb_put+0x5c/0x70
>  [<ffffffffa01ea1c3>] sctp_addto_chunk+0x63/0xd0 [sctp]
>  [<ffffffffa01eadaf>] sctp_process_asconf+0x1af/0x540 [sctp]
>  [<ffffffff8152d025>] ? _read_unlock_bh+0x15/0x20
>  [<ffffffffa01e0038>] sctp_sf_do_asconf+0x168/0x240 [sctp]
>  [<ffffffffa01e3751>] sctp_do_sm+0x71/0x1210 [sctp]
>  [<ffffffff8147645d>] ? fib_rules_lookup+0xad/0xf0
>  [<ffffffffa01e6b22>] ? sctp_cmp_addr_exact+0x32/0x40 [sctp]
>  [<ffffffffa01e8393>] sctp_assoc_bh_rcv+0xd3/0x180 [sctp]
>  [<ffffffffa01ee986>] sctp_inq_push+0x56/0x80 [sctp]
>  [<ffffffffa01fcc42>] sctp_rcv+0x982/0xa10 [sctp]
>  [<ffffffffa01d5123>] ? ipt_local_in_hook+0x23/0x28 [iptable_filter]
>  [<ffffffff8148bdc9>] ? nf_iterate+0x69/0xb0
>  [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
>  [<ffffffff8148bf86>] ? nf_hook_slow+0x76/0x120
>  [<ffffffff81496d10>] ? ip_local_deliver_finish+0x0/0x2d0
>  [<ffffffff81496ded>] ip_local_deliver_finish+0xdd/0x2d0
>  [<ffffffff81497078>] ip_local_deliver+0x98/0xa0
>  [<ffffffff8149653d>] ip_rcv_finish+0x12d/0x440
>  [<ffffffff81496ac5>] ip_rcv+0x275/0x350
>  [<ffffffff8145c88b>] __netif_receive_skb+0x4ab/0x750
>  [<ffffffff81460588>] netif_receive_skb+0x58/0x60
> 
> This can be triggered e.g., through a simple scripted nmap
> connection scan injecting the chunk after the handshake, for
> example, ...
> 
>   -------------- INIT[ASCONF; ASCONF_ACK] ------------->
>   <----------- INIT-ACK[ASCONF; ASCONF_ACK] ------------
>   -------------------- COOKIE-ECHO -------------------->
>   <-------------------- COOKIE-ACK ---------------------
>   ------------------ ASCONF; UNKNOWN ------------------>
> 
> ... where ASCONF chunk of length 280 contains 2 parameters ...
> 
>   1) Add IP address parameter (param length: 16)
>   2) Add/del IP address parameter (param length: 255)
> 
> ... followed by an UNKNOWN chunk of e.g. 4 bytes. Here, the
> Address Parameter in the ASCONF chunk is even missing, too.
> This is just an example and similarly-crafted ASCONF chunks
> could be used just as well.
> 
> The ASCONF chunk passes through sctp_verify_asconf() as all
> parameters passed sanity checks, and after walking, we ended
> up successfully at the chunk end boundary, and thus may invoke
> sctp_process_asconf(). Parameter walking is done with
> WORD_ROUND() to take padding into account.
> 
> In sctp_process_asconf()'s TLV processing, we may fail in
> sctp_process_asconf_param() e.g., due to removal of the IP
> address that is also the source address of the packet containing
> the ASCONF chunk, and thus we need to add all TLVs after the
> failure to our ASCONF response to remote via helper function
> sctp_add_asconf_response(), which basically invokes a
> sctp_addto_chunk() adding the error parameters to the given
> skb.
> 
> When walking to the next parameter this time, we proceed
> with ...
> 
>   length = ntohs(asconf_param->param_hdr.length);
>   asconf_param = (void *)asconf_param + length;
> 
> ... instead of the WORD_ROUND()'ed length, thus resulting here
> in an off-by-one that leads to reading the follow-up garbage
> parameter length of 12336, and thus throwing an skb_over_panic
> for the reply when trying to sctp_addto_chunk() next time,
> which implicitly calls the skb_put() with that length.
> 
> Fix it by using sctp_walk_params() [ which is also used in
> INIT parameter processing ] macro in the verification *and*
> in ASCONF processing: it will make sure we don't spill over,
> that we walk parameters WORD_ROUND()'ed. Moreover, we're being
> more defensive and guard against unknown parameter types and
> missized addresses.
> 
> Joint work with Vlad Yasevich.
> 
> Fixes: b896b82be4ae ("[SCTP] ADDIP: Support for processing incoming ASCONF_ACK chunks.")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Signed-off-by: Vlad Yasevich <vyasevich@gmail.com>

Acked-by: Neil Horman <nhorman@tuxdriver.com>