From: Ben Hutchings <hidden> Date: 2016-10-19 02:17:03
Qian Zhang (张谦) reported a potential socket buffer overflow in
tipc_msg_build(). The minimum fragment length needs to be checked
against the maximum packet size, which is based on the link MTU.
Reported-by: Qian Zhang (张谦) <redacted>
Signed-off-by: Ben Hutchings <redacted>
---
This is untested, but I think it fixes the issue reported. Ideally
tipc_l2_device_event() would also disable use of TIPC on devices with
too small an MTU, like several other protocols do.
Ben.
net/tipc/msg.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,gotoerror;}+/* Check that fragment and message header will fit */+if(INT_H_SIZE+mhsz>pktmax)+return-EMSGSIZE;+/* Prepare reusable fragment header */tipc_msg_init(msg_prevnode(mhdr),&pkthdr,MSG_FRAGMENTER,FIRST_FRAGMENT,INT_H_SIZE,msg_destnode(mhdr));
Qian Zhang (张谦) reported a potential socket buffer overflow in
tipc_msg_build(). The minimum fragment length needs to be checked
against the maximum packet size, which is based on the link MTU.
Reported-by: Qian Zhang (张谦) <redacted>
Signed-off-by: Ben Hutchings <redacted>
---
This is untested, but I think it fixes the issue reported. Ideally
tipc_l2_device_event() would also disable use of TIPC on devices with
too small an MTU, like several other protocols do.
Yes, I think so. I will create a patch to disable TIPC sending process
when MTU size is too small.
@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,gotoerror;}+/* Check that fragment and message header will fit */+if(INT_H_SIZE+mhsz>pktmax)+return-EMSGSIZE;
The "mhsz" represents the size of tipc packet header for current socket,
INT_H_SIZE indicates the size of tipc internal message header. So it
seems unreasonable to identify whether the sum of both header sizes is
bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to
compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should
return EMSGSIZE error code.
From: Ben Hutchings <hidden> Date: 2016-10-20 12:46:35
On Thu, 2016-10-20 at 17:30 +0800, Ying Xue wrote:
On 10/19/2016 10:16 AM, Ben Hutchings wrote:
quoted
Qian Zhang (张谦) reported a potential socket buffer overflow in
tipc_msg_build(). The minimum fragment length needs to be checked
against the maximum packet size, which is based on the link MTU.
+ /* Check that fragment and message header will fit */
+ if (INT_H_SIZE + mhsz > pktmax)
+ return -EMSGSIZE;
The "mhsz" represents the size of tipc packet header for current socket,
INT_H_SIZE indicates the size of tipc internal message header. So it
seems unreasonable to identify whether the sum of both header sizes is
bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to
compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should
return EMSGSIZE error code.
At this point we're about to copy INT_H_SIZE + mhsz bytes into the
first fragment. If that's already limited to be less than or equal to
MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE
is the maximum value of mhsz, that won't be good enough.
Ben.
From: Jon Maloy <hidden> Date: 2016-10-20 15:06:18
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Thursday, 20 October, 2016 08:46
To: Ying Xue <redacted>; Jon Maloy <redacted>
Cc: netdev@vger.kernel.org; Qian Zhang <redacted>; Eric Dumazet
[off-list ref]
Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Thu, 2016-10-20 at 17:30 +0800, Ying Xue wrote:
quoted
On 10/19/2016 10:16 AM, Ben Hutchings wrote:
quoted
Qian Zhang (张谦) reported a potential socket buffer overflow in
tipc_msg_build(). The minimum fragment length needs to be checked
against the maximum packet size, which is based on the link MTU.
[...]
quoted
quoted
--- a/net/tipc/msg.c+++ b/net/tipc/msg.c
@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct
msghdr *m,
quoted
quoted
quoted
quoted
goto error;
}
quoted
quoted
+ /* Check that fragment and message header will fit */
+ if (INT_H_SIZE + mhsz > pktmax)
+ return -EMSGSIZE;
The "mhsz" represents the size of tipc packet header for current socket,
INT_H_SIZE indicates the size of tipc internal message header. So it
seems unreasonable to identify whether the sum of both header sizes is
bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to
compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should
return EMSGSIZE error code.
At this point we're about to copy INT_H_SIZE + mhsz bytes into the
first fragment. If that's already limited to be less than or equal to
MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE
is the maximum value of mhsz, that won't be good enough.
MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger than the biggest header we are actually using, which is MCAST_H_SIZE (==44 bytes).
INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu < 84 bytes.
You won't find any interfaces or protocols that come even close to this limitation, so to me this test is redundant.
Regards
///jon
Ben.
quoted
quoted
+
quoted
quoted
/* Prepare reusable fragment header */
tipc_msg_init(msg_prevnode(mhdr), &pkthdr,
MSG_FRAGMENTER,
quoted
quoted
quoted
quoted
FIRST_FRAGMENT, INT_H_SIZE,
msg_destnode(mhdr));
quoted
quoted
--
Ben Hutchings
Never put off till tomorrow what you can avoid all together.
From: Ben Hutchings <hidden> Date: 2016-10-20 16:40:20
On Thu, 2016-10-20 at 14:51 +0000, Jon Maloy wrote:
[...]
quoted
At this point we're about to copy INT_H_SIZE + mhsz bytes into the
first fragment. If that's already limited to be less than or equal to
MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE
is the maximum value of mhsz, that won't be good enough.
MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger than the biggest header we are actually using, which is MCAST_H_SIZE (==44 bytes).
INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu < 84 bytes.
You won't find any interfaces or protocols that come even close to this limitation, so to me this test is redundant.
But I can easily create such an interface:
$ unshare -n -U -r
# ip l set lo mtu 1
Ben.
--
Ben Hutchings
Never put off till tomorrow what you can avoid all together.
From: Jon Maloy <hidden> Date: 2016-10-21 14:57:58
-----Original Message-----
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Thursday, 20 October, 2016 12:40
To: Jon Maloy <redacted>; Ying Xue <redacted>
Cc: netdev@vger.kernel.org; Qian Zhang <redacted>; Eric Dumazet
[off-list ref]
Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Thu, 2016-10-20 at 14:51 +0000, Jon Maloy wrote:
[...]
quoted
quoted
At this point we're about to copy INT_H_SIZE + mhsz bytes into the
first fragment. If that's already limited to be less than or equal to
MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if
MAX_H_SIZE
quoted
quoted
is the maximum value of mhsz, that won't be good enough.
MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger than
the biggest header we are actually using, which is MCAST_H_SIZE (==44 bytes).
quoted
INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu
< 84 bytes.
quoted
You won't find any interfaces or protocols that come even close to this
limitation, so to me this test is redundant.
But I can easily create such an interface:
$ unshare -n -U -r
# ip l set lo mtu 1
Ben.
It won't be very useful though. But I assume you mean it could be a possible exploit, and I suspect a few other things would break both in TIPC and in other stacks if you do anything like that. I think the solution to this is not to fix all possible places in the code where this can go wrong, but rather to have a generic test where we refuse to attach bearers/interfaces offering an mtu < e.g. 1000 bytes. This can easily be done in tipc_enable_l2_media().
///jon
--
Ben Hutchings
Never put off till tomorrow what you can avoid all together.
From: Ben Hutchings <hidden> Date: 2016-10-21 15:00:18
On Fri, 2016-10-21 at 14:57 +0000, Jon Maloy wrote:
quoted
-----Original Message-----
quoted
quoted
From: Ben Hutchings [mailto:ben@decadent.org.uk]
Sent: Thursday, 20 October, 2016 12:40
quoted
quoted
To: Jon Maloy <redacted>; Ying Xue <redacted>
quoted
quoted
Cc: netdev@vger.kernel.org; Qian Zhang <redacted>; Eric Dumazet
[off-list ref]
Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Thu, 2016-10-20 at 14:51 +0000, Jon Maloy wrote:
[...]
quoted
quoted
At this point we're about to copy INT_H_SIZE + mhsz bytes into the
first fragment. If that's already limited to be less than or equal to
MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if
MAX_H_SIZE
quoted
quoted
is the maximum value of mhsz, that won't be good enough.
MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger than
the biggest header we are actually using, which is MCAST_H_SIZE (==44 bytes).
quoted
INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu
< 84 bytes.
quoted
You won't find any interfaces or protocols that come even close to this
limitation, so to me this test is redundant.
But I can easily create such an interface:
$ unshare -n -U -r
# ip l set lo mtu 1
Ben.
It won't be very useful though. But I assume you mean it could be a
possible exploit,
Exactly.
and I suspect a few other things would break both in TIPC and in
other stacks if you do anything like that. I think the solution to
this is not to fix all possible places in the code where this can go
wrong, but rather to have a generic test where we refuse to attach
bearers/interfaces offering an mtu < e.g. 1000 bytes. This can easily
be done in tipc_enable_l2_media().
Yes.
Ben.
--
Ben Hutchings
One of the nice things about standards is that there are so many of
them.