Skip to contentSkip to conversation

tipc: Guard against tiny MTU in tipc_msg_build()

PATCH In review Ben Hutchings 3 participants 7 messages Oct 19, 2016 – Oct 21, 2016

Subsystems (3)
Networking Network drivers TIPC network layer
lore
Share & tools
lore

Ben Hutchings

author
#0 lore raw

PATCH 1/1 tipc: Guard against tiny MTU in tipc_msg_build()

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 (张谦) <[email protected]>
Signed-off-by Ben Hutchings <[email protected]>

---
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(+)
net/tipc/msg.c +4 -0
index 17201aa8423d..b9124ac82c29 100644
@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,
 		goto error;
 	}
 
+	/* 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));
#1 lore raw
Quoted 11 lines · Ben Hutchings

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 (张谦) <[email protected]>
Signed-off-by Ben Hutchings <[email protected]>

---
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.

Quoted 10 lines

Ben.

 net/tipc/msg.c | 4 ++++
 1 file changed, 4 insertions(+)
net/tipc/msg.c +0 -0
index 17201aa8423d..b9124ac82c29 100644

net/tipc/msg.c int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, · line 279

Quoted 6 lines
net/tipc/msg.c +3 -0
@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,
 		goto error;
 	}
 
+	/* 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.

Quoted 5 lines
net/tipc/msg.c +1 -0
+
 	/* Prepare reusable fragment header */
 	tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER,
 		      FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr));
author
#2 lore raw
Quoted 4 lines · Ying Xue
Quoted 3 lines · Ben Hutchings

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 7 lines
Quoted 7 lines

net/tipc/msg.c +0 -0
@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,
 > >  		goto error;
 > >  	}

Quoted 11 lines

net/tipc/msg.c int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, · line 279

Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
net/tipc/msg.c +2 -0
+	/* Check that fragment and message header will fit */
+	if (INT_H_SIZE + mhsz > pktmax)
net/tipc/msg.c +1 -0
+		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.

Quoted 8 lines
Quoted 5 lines

+

Quoted 3 lines
Quoted 3 lines

  /* Prepare reusable fragment header */ tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER, FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr));

Signature · 2 lines
Ben Hutchings
Never put off till tomorrow what you can avoid all together.
#3 lore raw
Quoted 36 lines

-----Original Message-----
From: Ben Hutchings [mailto:[email protected]]
Sent: Thursday, 20 October, 2016 08:46
To: Ying Xue <[email protected]>; Jon Maloy <[email protected]>
Cc: [email protected]; Qian Zhang <[email protected]>; Eric Dumazet <[email protected]>
Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

Quoted 4 lines · Ying Xue
Quoted 3 lines · Ben Hutchings

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 4 lines

net/tipc/msg.c int tipc_msg_build(struct tipc_msg *mhdr, struct

Quoted 4 lines
net/tipc/msg.c +0 -0
@@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct

msghdr *m,

Quoted 14 lines

net/tipc/msg.c int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, · line 279

Quoted 6 lines
net/tipc/msg.c +0 -0
 > >  		goto error;
 > >  	}
Quoted 2 lines
Quoted 2 lines
net/tipc/msg.c +2 -0
+	/* Check that fragment and message header will fit */
+	if (INT_H_SIZE + mhsz > pktmax)
net/tipc/msg.c +1 -0
+		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.

Quoted 4 lines

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

Quoted 16 lines

Ben.

Quoted 3 lines
Quoted 3 lines

+

Quoted 2 lines
Quoted 2 lines

  /* Prepare reusable fragment header */ tipc_msg_init(msg_prevnode(mhdr), &pkthdr,

MSG_FRAGMENTER,

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

        FIRST_FRAGMENT, INT_H_SIZE,

msg_destnode(mhdr));

Quoted 4 lines
Quoted 1 line

--
Ben Hutchings
Never put off till tomorrow what you can avoid all together.

author
#4 lore raw

On Thu, 2016-10-20 at 14:51 +0000, Jon Maloy wrote:
[...]

Quoted 9 lines
Quoted 4 lines

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.

Signature · 2 lines
Ben Hutchings
Never put off till tomorrow what you can avoid all together.
#5 lore raw
Quoted 24 lines

-----Original Message-----
From: Ben Hutchings [mailto:[email protected]]
Sent: Thursday, 20 October, 2016 12:40
To: Jon Maloy <[email protected]>; Ying Xue <[email protected]>
Cc: [email protected]; Qian Zhang <[email protected]>; Eric Dumazet <[email protected]>
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 3 lines
Quoted 3 lines

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 4 lines
Quoted 1 line

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 1 line

INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu

< 84 bytes.

Quoted 1 line

You won't find any interfaces or protocols that come even close to this

Quoted 8 lines

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

Quoted 4 lines

--
Ben Hutchings
Never put off till tomorrow what you can avoid all together.

author
#6 lore raw
Quoted 31 lines · Jon Maloy
Quoted 31 lines

-----Original Message-----

Quoted 1 line
Quoted 1 line

From: Ben Hutchings [mailto:[email protected]]

Sent: Thursday, 20 October, 2016 12:40

Quoted 3 lines
Quoted 3 lines

To: Jon Maloy <[email protected]>; Ying Xue <[email protected]>

Quoted 1 line
Quoted 1 line
Cc [email protected]; Qian Zhang <[email protected]>; Eric Dumazet

<[email protected]>

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 3 lines
Quoted 3 lines

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 5 lines
Quoted 1 line

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 1 line

INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an mtu

< 84 bytes.

Quoted 1 line

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:

Quoted 8 lines
Quoted 4 lines
$ 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.

Quoted 6 lines

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.

Signature · 3 lines
Ben Hutchings
One of the nice things about standards is that there are so many of
them.