[PATCH net] tipc: fix UAF in tipc_buf_append via tipc_msg_validate

[PATCH net] tipc: fix UAF in tipc_buf_append via tipc_msg_validate

[nicholas]

From: Nicholas Carlini <nicholas@carlini.com>

tipc_buf_append() passes the address of a local variable `head` to
tipc_msg_validate(). When the flow-control ratio check in
tipc_msg_validate() fires, it frees the original skb and updates
*_skb to point to a new copy -- but this only updates the local
`head`, not *headbuf. If validation subsequently fails (e.g. the
reassembled message has an invalid TIPC version), the err path
calls kfree_skb(*headbuf) on the already-freed skb. The replacement
skb is also leaked.

A remote attacker with an established TIPC link over a UDP bearer
can trigger this by sending a sequence of MSG_FRAGMENTER packets
crafted to inflate the reassembled skb's truesize relative to its
length past the ratio threshold, with an invalid version field in
the inner message.

Fix by passing headbuf directly to tipc_msg_validate() so the
pointer update propagates correctly.

Fixes: d618d09a68e4 ("tipc: enforce valid ratio between skb truesize and contents")
Cc: stable@vger.kernel.org
Signed-off-by: Nicholas Carlini <nicholas@carlini.com>
---
 net/tipc/msg.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 76284fc53..9f4f612ee 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -177,8 +177,9 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 
 	if (fragid == LAST_FRAGMENT) {
 		TIPC_SKB_CB(head)->validated = 0;
-		if (unlikely(!tipc_msg_validate(&head)))
+		if (unlikely(!tipc_msg_validate(headbuf)))
 			goto err;
+		head = *headbuf;
 		*buf = head;
 		TIPC_SKB_CB(head)->tail = NULL;
 		*headbuf = NULL;
-- 
2.43.0

RE: [PATCH net] tipc: fix UAF in tipc_buf_append via tipc_msg_validate

[Tung Quang Nguyen]

>Subject: [PATCH net] tipc: fix UAF in tipc_buf_append via tipc_msg_validate
Please remove "via tipc_msg_validate". It gives wrong impression that the fix could be done in tipc_msg_validate().
>
>From: Nicholas Carlini <nicholas@carlini.com>
>
>tipc_buf_append() passes the address of a local variable `head` to
>tipc_msg_validate(). When the flow-control ratio check in
>tipc_msg_validate() fires, it frees the original skb and updates *_skb to point to
>a new copy -- but this only updates the local `head`, not *headbuf. If validation
>subsequently fails (e.g. the reassembled message has an invalid TIPC version),
>the err path calls kfree_skb(*headbuf) on the already-freed skb. The
>replacement skb is also leaked.
>
>A remote attacker with an established TIPC link over a UDP bearer can trigger
>this by sending a sequence of MSG_FRAGMENTER packets crafted to inflate the
>reassembled skb's truesize relative to its length past the ratio threshold, with
>an invalid version field in the inner message.
>
>Fix by passing headbuf directly to tipc_msg_validate() so the pointer update
>propagates correctly.
>
>Fixes: d618d09a68e4 ("tipc: enforce valid ratio between skb truesize and
>contents")
>Cc: stable@vger.kernel.org
>Signed-off-by: Nicholas Carlini <nicholas@carlini.com>
>---
> net/tipc/msg.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/net/tipc/msg.c b/net/tipc/msg.c index 76284fc53..9f4f612ee 100644
>--- a/net/tipc/msg.c
>+++ b/net/tipc/msg.c
>@@ -177,8 +177,9 @@ int tipc_buf_append(struct sk_buff **headbuf, struct
>sk_buff **buf)
>
> 	if (fragid == LAST_FRAGMENT) {
> 		TIPC_SKB_CB(head)->validated = 0;
>-		if (unlikely(!tipc_msg_validate(&head)))
>+		if (unlikely(!tipc_msg_validate(headbuf)))
> 			goto err;
>+		head = *headbuf;
This fix is not optimal because it adds overhead to normal path. This patch is better, I think:
diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 76284fc538eb..01a693559589 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -177,8 +177,19 @@ int tipc_buf_append(struct sk_buff **headbuf, struct sk_buff **buf)
 
        if (fragid == LAST_FRAGMENT) {
                TIPC_SKB_CB(head)->validated = 0;
-               if (unlikely(!tipc_msg_validate(&head)))
+               if (unlikely(!tipc_msg_validate(&head))) {
+                       /* reassembled skb has been freed in
+                        * tipc_msg_validate() because of invalid truesize.
+                        * head now points to newly-allocated reassembled skb
+                        * while *headbuf points to freed reassembled skb.
+                        * So, correct *headbuf for freeing newly-allocated
+                        * reassembled skb later.
+                        */
+                       if (head != *headbuf)
+                               *headbuf = head;
+
                        goto err;
+               }
                *buf = head;
                TIPC_SKB_CB(head)->tail = NULL;
                *headbuf = NULL;
> 		*buf = head;
> 		TIPC_SKB_CB(head)->tail = NULL;
> 		*headbuf = NULL;
>--
>2.43.0
>