[PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1097 bytes --]

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 (张谦) <zhangqian-c@360.cn>
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
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(+)

diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index 17201aa8423d..b9124ac82c29 100644
--- 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,
 		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));

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Ying Xue]

On 10/19/2016 10:16 AM, Ben Hutchings wrote:
> 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 (张谦) <zhangqian-c@360.cn>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> 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.

> Ben.
> 
>  net/tipc/msg.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 17201aa8423d..b9124ac82c29 100644
> --- 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,
>  		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.

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

Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1648 bytes --]

On Thu, 2016-10-20 at 17:30 +0800, Ying Xue wrote:
> On 10/19/2016 10:16 AM, Ben Hutchings wrote:
> > 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.
[...]
> >  
> > --- 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,
> > > >  		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.

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.

> > +
> > > >  	/* Prepare reusable fragment header */
> > > >  	tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER,
> > > >  		      FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr));
> > 
> 
> 
> 
-- 
Ben Hutchings
Never put off till tomorrow what you can avoid all together.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Jon Maloy]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Thursday, 20 October, 2016 08:46
> To: Ying Xue <ying.xue0@gmail.com>; Jon Maloy <jon.maloy@ericsson.com>
> Cc: netdev@vger.kernel.org; Qian Zhang <zhangqian-c@360.cn>; Eric Dumazet
> <edumazet@google.com>
> 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:
> > On 10/19/2016 10:16 AM, Ben Hutchings wrote:
> > > 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.
> [...]
> > >
> > > --- 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,
> > > > >  		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.
> 
> 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.
> 
> > > +
> > > > >  	/* Prepare reusable fragment header */
> > > > >  	tipc_msg_init(msg_prevnode(mhdr), &pkthdr,
> MSG_FRAGMENTER,
> > > > >  		      FIRST_FRAGMENT, INT_H_SIZE,
> msg_destnode(mhdr));
> > >
> >
> >
> >
> --
> Ben Hutchings
> Never put off till tomorrow what you can avoid all together.

Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

On Thu, 2016-10-20 at 14:51 +0000, Jon Maloy wrote:
[...]
> > 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.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Jon Maloy]

> -----Original Message-----
> From: Ben Hutchings [mailto:ben@decadent.org.uk]
> Sent: Thursday, 20 October, 2016 12:40
> To: Jon Maloy <jon.maloy@ericsson.com>; Ying Xue <ying.xue0@gmail.com>
> Cc: netdev@vger.kernel.org; Qian Zhang <zhangqian-c@360.cn>; Eric Dumazet
> <edumazet@google.com>
> 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:
> [...]
> > > 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.

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.

Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 2046 bytes --]

On Fri, 2016-10-21 at 14:57 +0000, Jon Maloy wrote:
> > -----Original Message-----
> > > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > Sent: Thursday, 20 October, 2016 12:40
> > > > To: Jon Maloy <jon.maloy@ericsson.com>; Ying Xue <ying.xue0@gmail.com>
> > > > > > Cc: netdev@vger.kernel.org; Qian Zhang <zhangqian-c@360.cn>; Eric Dumazet
> > > > <edumazet@google.com>
> > 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:
> > [...]
> > > > 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.
> 
> 
> 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.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[张谦]

yes,  tipc_l2_device_event() only change MTU of bearer rather than the MTU of link, tipc_enable_l2_media() will be the right place to test a tiny MTU.

Qian

Sent from my iPhone

On 5 Nov 2016, at 00:00, Jon Maloy <jon.maloy@ericsson.com> wrote:

>> -----Original Message-----
>> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
>> On Behalf Of ??
>> Sent: Friday, 04 November, 2016 03:24
>> To: Jon Maloy <jon.maloy@ericsson.com>; Ben Hutchings
>> <ben@decadent.org.uk>; Ying Xue <ying.xue0@gmail.com>
>> Cc: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
>> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
>> 
>> Hi,
>> I think both tipc_l2_device_event() and tipc_enable_l2_media() need to refuse a
>> tiny MTU for TIPC bearers.
> 
> Right, except that when looking into the code for tipc_l2_device_event() I realize that it currently doesn't try to re-adapt to a new MTU at all. It just calls tipc_reset_bearer(), which I suspect has changed somewhere along the road to ignore the MTU. So, you only need to change tipc_enable_l2_media().
> 
> ///jon
> 
>> 
>> tipc_l2_device_event() used to update the TIPC MTU value when executing a
>> command like 'ifconfig eth0 MTU 1 up'.
>> tipc_enable_l2_media() will be invoked when the TIPC network created.
>> 
>> Thanks.
>> 
>> Qian Zhang
>> MarvelTeam Qihoo 360
>> 
>> 
>> 
>> -----邮件原件-----
>> 发件人: Jon Maloy [mailto:jon.maloy@ericsson.com]
>> 发送时间: 2016年11月1日 19:37
>> 收件人: 张谦; Ben Hutchings; Ying Xue
>> 抄送: netdev@vger.kernel.org; Eric Dumazet
>> 主题: RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
>> 
>> Hi,
>> I think we all agreed in the end that this is a possible, but highly implausible,
>> scenario, and rather as a point of exploit than a functional bug.
>> The solution is very simple, and described further down in this mail thread. I have
>> not done anything to it yet, but you are welcome to contribute.
>> 
>> BR
>> ///jon
>> 
>> 
>>> -----Original Message-----
>>> From: 张谦 [mailto:zhangqian-c@360.cn]
>>> Sent: Tuesday, 01 November, 2016 02:35
>>> To: Ben Hutchings <ben@decadent.org.uk>; Jon Maloy
>>> <jon.maloy@ericsson.com>; Ying Xue <ying.xue0@gmail.com>
>>> Cc: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
>>> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in
>>> tipc_msg_build()
>>> 
>>> Hi all,
>>> I have accomplished a PoC can help you to confirm this issue.
>>> 
>>> And two weeks passed from the last mail, can you tell me the progress
>>> of the patch to this flaw?
>>> 
>>> Thanks.
>>> 
>>> Qian Zhang
>>> Marvel Team Qihoo 360
>>> 
>>> 
>>> -----邮件原件-----
>>> 发件人: Ben Hutchings [mailto:ben@decadent.org.uk]
>>> 发送时间: 2016年10月21日 23:00
>>> 收件人: Jon Maloy; Ying Xue
>>> 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet
>>> 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
>>> 
>>> On Fri, 2016-10-21 at 14:57 +0000, Jon Maloy wrote:
>>>>> -----Original Message-----
>>>>>>> From: Ben Hutchings [mailto:ben@decadent.org.uk]
>>>>> Sent: Thursday, 20 October, 2016 12:40
>>>>>>> To: Jon Maloy <jon.maloy@ericsson.com>; Ying Xue
>>>>>>> <ying.xue0@gmail.com>
>>>>>>>>> Cc: netdev@vger.kernel.org; Qian Zhang
>>>>>>>>> <zhangqian-c@360.cn>; Eric Dumazet
>>>>>>> <edumazet@google.com>
>>>>> 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:
>>>>> [...]
>>>>>>> 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.
>>>> 
>>>> 
>>>> 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.
> 

RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Jon Maloy]

> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
> On Behalf Of ??
> Sent: Friday, 04 November, 2016 03:24
> To: Jon Maloy <jon.maloy@ericsson.com>; Ben Hutchings
> <ben@decadent.org.uk>; Ying Xue <ying.xue0@gmail.com>
> Cc: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> 
> Hi,
> I think both tipc_l2_device_event() and tipc_enable_l2_media() need to refuse a
> tiny MTU for TIPC bearers.

Right, except that when looking into the code for tipc_l2_device_event() I realize that it currently doesn't try to re-adapt to a new MTU at all. It just calls tipc_reset_bearer(), which I suspect has changed somewhere along the road to ignore the MTU. So, you only need to change tipc_enable_l2_media().

///jon

> 
> tipc_l2_device_event() used to update the TIPC MTU value when executing a
> command like 'ifconfig eth0 MTU 1 up'.
> tipc_enable_l2_media() will be invoked when the TIPC network created.
> 
> Thanks.
> 
> Qian Zhang
> MarvelTeam Qihoo 360
> 
> 
> 
> -----邮件原件-----
> 发件人: Jon Maloy [mailto:jon.maloy@ericsson.com]
> 发送时间: 2016年11月1日 19:37
> 收件人: 张谦; Ben Hutchings; Ying Xue
> 抄送: netdev@vger.kernel.org; Eric Dumazet
> 主题: RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> 
> Hi,
> I think we all agreed in the end that this is a possible, but highly implausible,
> scenario, and rather as a point of exploit than a functional bug.
> The solution is very simple, and described further down in this mail thread. I have
> not done anything to it yet, but you are welcome to contribute.
> 
> BR
> ///jon
> 
> 
> > -----Original Message-----
> > From: 张谦 [mailto:zhangqian-c@360.cn]
> > Sent: Tuesday, 01 November, 2016 02:35
> > To: Ben Hutchings <ben@decadent.org.uk>; Jon Maloy
> > <jon.maloy@ericsson.com>; Ying Xue <ying.xue0@gmail.com>
> > Cc: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
> > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in
> > tipc_msg_build()
> >
> > Hi all,
> > I have accomplished a PoC can help you to confirm this issue.
> >
> > And two weeks passed from the last mail, can you tell me the progress
> > of the patch to this flaw?
> >
> > Thanks.
> >
> > Qian Zhang
> > Marvel Team Qihoo 360
> >
> >
> > -----邮件原件-----
> > 发件人: Ben Hutchings [mailto:ben@decadent.org.uk]
> > 发送时间: 2016年10月21日 23:00
> > 收件人: Jon Maloy; Ying Xue
> > 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet
> > 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> >
> > On Fri, 2016-10-21 at 14:57 +0000, Jon Maloy wrote:
> > > > -----Original Message-----
> > > > > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > > > Sent: Thursday, 20 October, 2016 12:40
> > > > > > To: Jon Maloy <jon.maloy@ericsson.com>; Ying Xue
> > > > > > <ying.xue0@gmail.com>
> > > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang
> > > > > > > > <zhangqian-c@360.cn>; Eric Dumazet
> > > > > > <edumazet@google.com>
> > > > 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:
> > > > [...]
> > > > > > 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.
> > >
> > >
> > > 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.

Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[张谦]

Hi,
I think both tipc_l2_device_event() and tipc_enable_l2_media() need to refuse a tiny MTU for TIPC bearers.

tipc_l2_device_event() used to update the TIPC MTU value when executing a command like 'ifconfig eth0 MTU 1 up'.
tipc_enable_l2_media() will be invoked when the TIPC network created.

Thanks.

Qian Zhang
MarvelTeam Qihoo 360



-----邮件原件-----
发件人: Jon Maloy [mailto:jon.maloy@ericsson.com] 
发送时间: 2016年11月1日 19:37
收件人: 张谦; Ben Hutchings; Ying Xue
抄送: netdev@vger.kernel.org; Eric Dumazet
主题: RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

Hi,
I think we all agreed in the end that this is a possible, but highly implausible, scenario, and rather as a point of exploit than a functional bug.
The solution is very simple, and described further down in this mail thread. I have not done anything to it yet, but you are welcome to contribute.

BR
///jon


> -----Original Message-----
> From: 张谦 [mailto:zhangqian-c@360.cn]
> Sent: Tuesday, 01 November, 2016 02:35
> To: Ben Hutchings <ben@decadent.org.uk>; Jon Maloy 
> <jon.maloy@ericsson.com>; Ying Xue <ying.xue0@gmail.com>
> Cc: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in 
> tipc_msg_build()
> 
> Hi all,
> I have accomplished a PoC can help you to confirm this issue.
> 
> And two weeks passed from the last mail, can you tell me the progress 
> of the patch to this flaw?
> 
> Thanks.
> 
> Qian Zhang
> Marvel Team Qihoo 360
> 
> 
> -----邮件原件-----
> 发件人: Ben Hutchings [mailto:ben@decadent.org.uk]
> 发送时间: 2016年10月21日 23:00
> 收件人: Jon Maloy; Ying Xue
> 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet
> 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> 
> On Fri, 2016-10-21 at 14:57 +0000, Jon Maloy wrote:
> > > -----Original Message-----
> > > > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > > Sent: Thursday, 20 October, 2016 12:40
> > > > > To: Jon Maloy <jon.maloy@ericsson.com>; Ying Xue 
> > > > > <ying.xue0@gmail.com>
> > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang 
> > > > > > > <zhangqian-c@360.cn>; Eric Dumazet
> > > > > <edumazet@google.com>
> > > 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:
> > > [...]
> > > > > 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.
> >
> >
> > 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.

RE: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[Jon Maloy]

Hi,
I think we all agreed in the end that this is a possible, but highly implausible, scenario, and rather as a point of exploit than a functional bug.
The solution is very simple, and described further down in this mail thread. I have not done anything to it yet, but you are welcome to contribute.

BR
///jon


> -----Original Message-----
> From: 张谦 [mailto:zhangqian-c@360.cn]
> Sent: Tuesday, 01 November, 2016 02:35
> To: Ben Hutchings <ben@decadent.org.uk>; Jon Maloy
> <jon.maloy@ericsson.com>; Ying Xue <ying.xue0@gmail.com>
> Cc: netdev@vger.kernel.org; Eric Dumazet <edumazet@google.com>
> Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> 
> Hi all,
> I have accomplished a PoC can help you to confirm this issue.
> 
> And two weeks passed from the last mail, can you tell me the progress of the
> patch to this flaw?
> 
> Thanks.
> 
> Qian Zhang
> Marvel Team Qihoo 360
> 
> 
> -----邮件原件-----
> 发件人: Ben Hutchings [mailto:ben@decadent.org.uk]
> 发送时间: 2016年10月21日 23:00
> 收件人: Jon Maloy; Ying Xue
> 抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet
> 主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
> 
> On Fri, 2016-10-21 at 14:57 +0000, Jon Maloy wrote:
> > > -----Original Message-----
> > > > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > > Sent: Thursday, 20 October, 2016 12:40
> > > > > To: Jon Maloy <jon.maloy@ericsson.com>; Ying Xue
> > > > > <ying.xue0@gmail.com>
> > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang <zhangqian-c@360.cn>;
> > > > > > > Eric Dumazet
> > > > > <edumazet@google.com>
> > > 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:
> > > [...]
> > > > > 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.
> >
> >
> > 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.

Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

[张谦]

[-- Attachment #1: Type: text/plain, Size: 2621 bytes --]

Hi all,
I have accomplished a PoC can help you to confirm this issue.

And two weeks passed from the last mail, can you tell me the progress of the patch to this flaw?

Thanks.

Qian Zhang
Marvel Team Qihoo 360


-----邮件原件-----
发件人: Ben Hutchings [mailto:ben@decadent.org.uk] 
发送时间: 2016年10月21日 23:00
收件人: Jon Maloy; Ying Xue
抄送: netdev@vger.kernel.org; 张谦; Eric Dumazet
主题: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()

On Fri, 2016-10-21 at 14:57 +0000, Jon Maloy wrote:
> > -----Original Message-----
> > > > From: Ben Hutchings [mailto:ben@decadent.org.uk]
> > Sent: Thursday, 20 October, 2016 12:40
> > > > To: Jon Maloy <jon.maloy@ericsson.com>; Ying Xue 
> > > > <ying.xue0@gmail.com>
> > > > > > Cc: netdev@vger.kernel.org; Qian Zhang <zhangqian-c@360.cn>; 
> > > > > > Eric Dumazet
> > > > <edumazet@google.com>
> > 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:
> > [...]
> > > > 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.
> 
> 
> 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.


[-- Attachment #2: tipc_tiny_mtu_poc.zip --]
[-- Type: application/x-zip-compressed, Size: 78166 bytes --]