netdev - Re: [net] netfilter: nat: sctp: fix ICMP packet to be dropped accidently

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20170308172943.GA8577@salvia>
Date:   Wed, 8 Mar 2017 18:29:43 +0100
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Ying Xue <ying.xue@...driver.com>
Cc:     netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org
Subject: Re: [net] netfilter: nat: sctp: fix ICMP packet to be dropped
 accidently

On Sat, Mar 04, 2017 at 06:00:02PM +0800, Ying Xue wrote:
> Regarding RFC 792, the first 64 bits of the original SCTP datagram's
> data could be contained in ICMP packet, such as:
> 
>     0                   1                   2                   3
>     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |     Type      |     Code      |          Checksum             |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                             unused                            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |      Internet Header + 64 bits of Original Data Datagram      |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> However, according to RFC 4960, SCTP datagram header is as below:
> 
>     0                   1                   2                   3
>     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |     Source Port Number        |     Destination Port Number   |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                      Verification Tag                         |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                           Checksum                            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 
> It means only the first three fields of SCTP header can be carried in
> ICMP packet except for Checksum field.
> 
> At present in sctp_manip_pkt(), no matter whether the packet is ICMP or
> not, it always calculates SCTP packet checksum. However, not only the
> calculation of checksum is unnecessary for ICMP, but also it causes
> another fatal issue that ICMP packet is dropped. The header size of
> SCTP is used to identify whether the writeable length of skb is bigger
> than skb->len through skb_make_writable() in sctp_manip_pkt(). But
> when it deals with ICMP packet, skb_make_writable() directly returns
> false as the writeable length of skb is bigger than skb->len.
> Subsequently ICMP is dropped.
> 
> Now we correct this misbahavior. When sctp_manip_pkt() handles ICMP
> packet, 8 bytes rather than the whole SCTP header size is used to check
> if writeable length of skb is overflowed. Meanwhile, as it's meaningless
> to calculate checksum when packet is ICMP, the computation of checksum
> is ignored as well.

Applied, thanks.