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

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

[Sangho Lee]

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:

  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@vger.kernel.org
Signed-off-by: Sangho Lee <kudo3228@gmail.com>
---
 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)) {
 		p = (struct tipc_gap_ack_blks *)msg_data(hdr);
 		sz = ntohs(p->len);
 		/* Sanity check */
-- 
2.43.0

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@vger.kernel.org
>Signed-off-by: Sangho Lee <kudo3228@gmail.com>
>---
> 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
>