RE: [PATCH net] tipc: validate Gap ACK blocks header before parsing

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: validate Gap ACK blocks header before parsing
>
>An established TIPC peer that negotiated TIPC_GAP_ACK_BLOCK can send a
>STATE_MSG with fewer than the four bytes required for struct
>tipc_gap_ack_blks. Basic TIPC message validation accepts such a message,
>including one with an empty data area, because its declared size still contains a
>complete protocol header.
>
>tipc_get_gap_ack_blks() then dereferences the record to read len, ugack_cnt,
>and bgack_cnt. Both callers compare the returned record size with the declared
>message data length, but only after these fields have already been read. The
>late checks therefore cannot protect the fixed record header access.
>
>This was reproduced on current net-next with two live TIPC nodes over a veth
>bearer. Replaying an established peer's STATE_MSG with its message size
>reduced from 44 to 40 bytes produced:

It is not recommended to use TIPC in insecure environment and create fake protocol messages because TIPC cannot handle this.
You need to use IPSec (UDP bearer) or Encryption (Ethernet bearer) in insecure environment. See the requirement below:
https://datatracker.ietf.org/doc/html/draft-maloy-tipc-01.txt#section-6

>
>  BUG: KMSAN: uninit-value in tipc_get_gap_ack_blks
>   tipc_get_gap_ack_blks
>   tipc_bcast_sync_rcv
>   tipc_node_bc_sync_rcv
>   tipc_rcv
>   tipc_l2_rcv_msg
>
>  Uninit was created at:
>   __alloc_skb
>   alloc_skb_with_frags
>   sock_alloc_send_pskb
>   packet_sendmsg
>
>The uninitialized record length then reached tipc_bcast_sync_rcv(), where it
>was used to decide whether the STATE_MSG should be dropped. Supplying
>four initialized bytes after the declared end of otherwise identical messages
>also changed that decision: a trailing len of zero continued processing, while a
>trailing len of four dropped the message. Protocol processing therefore
>depends on bytes that are not part of the declared TIPC message.
>
>The KMSAN finding was reproduced for declared data lengths zero through
>three. After adding the minimum-length check, none of those four inputs
>reported an uninitialized value in tipc_get_gap_ack_blks() or its caller, and
>bytes after the declared message no longer affected the decision.
>
>Check that the fixed record header is present before parsing it. The callers'
>existing size checks continue to validate the complete variable-length record.
>
>Fixes: d7626b5acff9 ("tipc: introduce Gap ACK blocks for broadcast link")
>Cc: stable@xxxxxxxxxxxxxxx
>Signed-off-by: Sangho Lee <kudo3228@xxxxxxxxx>
>---
> net/tipc/link.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/net/tipc/link.c b/net/tipc/link.c index 49dfc098d89b..8cc30ebdf575
>100644
>--- a/net/tipc/link.c
>+++ b/net/tipc/link.c
>@@ -1418,7 +1418,8 @@ u16 tipc_get_gap_ack_blks(struct tipc_gap_ack_blks
>**ga, struct tipc_link *l,
> 	u16 sz = 0;
>
> 	/* Does peer support the Gap ACK blocks feature? */
>-	if (l->peer_caps & TIPC_GAP_ACK_BLOCK) {
>+	if ((l->peer_caps & TIPC_GAP_ACK_BLOCK) &&
>+	    msg_data_sz(hdr) >= sizeof(*p)) {

It does not make sense to have this check because when data size can be modified, there is no guarantee that fields in struct tipc_gap_ack_blks are correct.

> 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
> 		sz = ntohs(p->len);
> 		/* Sanity check */
>--
>2.43.0
>