| 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
| ||
|
Date: Mon, 2 Oct 2023 10:59:35 -0400
From: Xin Long <lucien.xin@...il.com>
To: network dev <netdev@...r.kernel.org>, netfilter-devel@...r.kernel.org,
linux-sctp@...r.kernel.org
Cc: davem@...emloft.net, kuba@...nel.org, Eric Dumazet <edumazet@...gle.com>,
Paolo Abeni <pabeni@...hat.com>, Pablo Neira Ayuso <pablo@...filter.org>,
Jozsef Kadlecsik <kadlec@...filter.org>, Florian Westphal <fw@...len.de>,
Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH nf] netfilter: handle the connecting collision properly in nf_conntrack_proto_sctp
On Sun, Oct 1, 2023 at 11:07 AM Xin Long <lucien.xin@...il.com> wrote:
>
> In Scenario A and B below, as the delayed INIT_ACK always changes the peer
> vtag, SCTP ct with the incorrect vtag may cause packet loss.
>
> Scenario A: INIT_ACK is delayed until the peer receives its own INIT_ACK
>
> 192.168.1.2 > 192.168.1.1: [INIT] [init tag: 1328086772]
> 192.168.1.1 > 192.168.1.2: [INIT] [init tag: 1414468151]
> 192.168.1.2 > 192.168.1.1: [INIT ACK] [init tag: 1328086772]
> 192.168.1.1 > 192.168.1.2: [INIT ACK] [init tag: 1650211246] *
> 192.168.1.2 > 192.168.1.1: [COOKIE ECHO]
> 192.168.1.1 > 192.168.1.2: [COOKIE ECHO]
> 192.168.1.2 > 192.168.1.1: [COOKIE ACK]
>
> Scenario B: INIT_ACK is delayed until the peer completes its own handshake
>
> 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408]
> 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885]
> 192.168.1.2 > 192.168.1.1: sctp (1) [INIT ACK] [init tag: 3922216408]
> 192.168.1.1 > 192.168.1.2: sctp (1) [COOKIE ECHO]
> 192.168.1.2 > 192.168.1.1: sctp (1) [COOKIE ACK]
> 192.168.1.1 > 192.168.1.2: sctp (1) [INIT ACK] [init tag: 3914796021] *
>
> This patch fixes it as below:
>
> In SCTP_CID_INIT processing:
> - clear ct->proto.sctp.init[!dir] if ct->proto.sctp.init[dir] &&
> ct->proto.sctp.init[!dir]. (Scenario E)
> - set ct->proto.sctp.init[dir].
>
> In SCTP_CID_INIT_ACK processing:
> - drop it if !ct->proto.sctp.init[!dir] && ct->proto.sctp.vtag[!dir] &&
> ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario B, Scenario C)
> - drop it if ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] &&
> ct->proto.sctp.vtag[!dir] != ih->init_tag. (Scenario A)
>
> In SCTP_CID_COOKIE_ACK processing:
> - clear ct->proto.sctp.init[dir] and ct->proto.sctp.init[!dir]. (Scenario D)
>
> Also, it's important to allow the ct state to move forward with cookie_echo
> and cookie_ack from the opposite dir for the collision scenarios.
>
> There are also other Scenarios where it should allow the packet through,
> addressed by the processing above:
>
> Scenario C: new CT is created by INIT_ACK.
>
> Scenario D: start INIT on the existing ESTABLISHED ct.
>
> Scenario E: start INIT after the old collision on the existing ESTABLISHED ct.
>
> 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 3922216408]
> 192.168.1.1 > 192.168.1.2: sctp (1) [INIT] [init tag: 144230885]
> (both side are stopped, then start new connection again in hours)
> 192.168.1.2 > 192.168.1.1: sctp (1) [INIT] [init tag: 242308742]
>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
> include/linux/netfilter/nf_conntrack_sctp.h | 1 +
> net/netfilter/nf_conntrack_proto_sctp.c | 41 ++++++++++++++++-----
> 2 files changed, 33 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/netfilter/nf_conntrack_sctp.h b/include/linux/netfilter/nf_conntrack_sctp.h
> index 625f491b95de..fb31312825ae 100644
> --- a/include/linux/netfilter/nf_conntrack_sctp.h
> +++ b/include/linux/netfilter/nf_conntrack_sctp.h
> @@ -9,6 +9,7 @@ struct ip_ct_sctp {
> enum sctp_conntrack state;
>
> __be32 vtag[IP_CT_DIR_MAX];
> + u8 init[IP_CT_DIR_MAX];
> u8 last_dir;
> u8 flags;
> };
> diff --git a/net/netfilter/nf_conntrack_proto_sctp.c b/net/netfilter/nf_conntrack_proto_sctp.c
> index b6bcc8f2f46b..91aee286d503 100644
> --- a/net/netfilter/nf_conntrack_proto_sctp.c
> +++ b/net/netfilter/nf_conntrack_proto_sctp.c
> @@ -112,7 +112,7 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
> /* shutdown_ack */ {sSA, sCL, sCW, sCE, sES, sSA, sSA, sSA, sSA},
> /* error */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't have Stale cookie*/
> /* cookie_echo */ {sCL, sCL, sCE, sCE, sES, sSS, sSR, sSA, sCL},/* 5.2.4 - Big TODO */
> -/* cookie_ack */ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sCL},/* Can't come in orig dir */
> +/* cookie_ack */ {sCL, sCL, sCW, sES, sES, sSS, sSR, sSA, sCL},/* Can't come in orig dir */
> /* shutdown_comp*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sCL, sCL},
> /* heartbeat */ {sHS, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS},
> /* heartbeat_ack*/ {sCL, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS},
> @@ -126,7 +126,7 @@ static const u8 sctp_conntracks[2][11][SCTP_CONNTRACK_MAX] = {
> /* shutdown */ {sIV, sCL, sCW, sCE, sSR, sSS, sSR, sSA, sIV},
> /* shutdown_ack */ {sIV, sCL, sCW, sCE, sES, sSA, sSA, sSA, sIV},
> /* error */ {sIV, sCL, sCW, sCL, sES, sSS, sSR, sSA, sIV},
> -/* cookie_echo */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sIV},/* Can't come in reply dir */
> +/* cookie_echo */ {sIV, sCL, sCE, sCE, sES, sSS, sSR, sSA, sIV},/* Can't come in reply dir */
> /* cookie_ack */ {sIV, sCL, sCW, sES, sES, sSS, sSR, sSA, sIV},
> /* shutdown_comp*/ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sCL, sIV},
> /* heartbeat */ {sIV, sCL, sCW, sCE, sES, sSS, sSR, sSA, sHS},
> @@ -412,6 +412,9 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct,
> /* (D) vtag must be same as init_vtag as found in INIT_ACK */
> if (sh->vtag != ct->proto.sctp.vtag[dir])
> goto out_unlock;
> + } else if (sch->type == SCTP_CID_COOKIE_ACK) {
> + ct->proto.sctp.init[dir] = 0;
> + ct->proto.sctp.init[!dir] = 0;
> } else if (sch->type == SCTP_CID_HEARTBEAT) {
> if (ct->proto.sctp.vtag[dir] == 0) {
> pr_debug("Setting %d vtag %x for dir %d\n", sch->type, sh->vtag, dir);
> @@ -461,16 +464,18 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct,
> }
>
> /* If it is an INIT or an INIT ACK note down the vtag */
> - if (sch->type == SCTP_CID_INIT ||
> - sch->type == SCTP_CID_INIT_ACK) {
> - struct sctp_inithdr _inithdr, *ih;
> + if (sch->type == SCTP_CID_INIT) {
> + struct sctp_inithdr _ih, *ih;
>
> - ih = skb_header_pointer(skb, offset + sizeof(_sch),
> - sizeof(_inithdr), &_inithdr);
> + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih);
> if (ih == NULL)
> goto out_unlock;
> - pr_debug("Setting vtag %x for dir %d\n",
> - ih->init_tag, !dir);
> +
> + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir])
> + ct->proto.sctp.init[!dir] = 0;
> + ct->proto.sctp.init[dir] = 1;
> +
> + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir);
> ct->proto.sctp.vtag[!dir] = ih->init_tag;
>
> /* don't renew timeout on init retransmit so
> @@ -481,6 +486,24 @@ int nf_conntrack_sctp_packet(struct nf_conn *ct,
> old_state == SCTP_CONNTRACK_CLOSED &&
> nf_ct_is_confirmed(ct))
> ignore = true;
> + } else if (sch->type == SCTP_CID_INIT_ACK) {
> + struct sctp_inithdr _ih, *ih;
> + u32 vtag;
> +
> + ih = skb_header_pointer(skb, offset + sizeof(_sch), sizeof(*ih), &_ih);
> + if (ih == NULL)
> + goto out_unlock;
> +
> + vtag = ct->proto.sctp.vtag[!dir];
> + if (!ct->proto.sctp.init[!dir] && vtag && vtag != ih->init_tag)
> + goto out_unlock;
> + /* collision */
> + if (ct->proto.sctp.init[dir] && ct->proto.sctp.init[!dir] &&
> + vtag != ih->init_tag)
> + goto out_unlock;
> +
> + pr_debug("Setting vtag %x for dir %d\n", ih->init_tag, !dir);
> + ct->proto.sctp.vtag[!dir] = ih->init_tag;
> }
>
> ct->proto.sctp.state = new_state;
> --
> 2.39.1
>
a reproducer is attached.
Thanks.
View attachment "sctp_collision.sh" of type "text/x-sh" (5392 bytes)
Powered by blists - more mailing lists