netdev - Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <35A00A1C-3842-4BDB-8995-9176D5453469@360.cn>
Date:   Fri, 4 Nov 2016 17:45:51 +0000
From:   张谦 <zhangqian-c@....cn>
To:     Jon Maloy <jon.maloy@...csson.com>
CC:     Ben Hutchings <ben@...adent.org.uk>,
        Ying Xue <ying.xue0@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        "tyrande000@...il.com" <tyrande000@...il.com>
Subject: 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@...csson.com> wrote:

>> -----Original Message-----
>> From: netdev-owner@...r.kernel.org [mailto:netdev-owner@...r.kernel.org]
>> On Behalf Of ??
>> Sent: Friday, 04 November, 2016 03:24
>> To: Jon Maloy <jon.maloy@...csson.com>; Ben Hutchings
>> <ben@...adent.org.uk>; Ying Xue <ying.xue0@...il.com>
>> Cc: netdev@...r.kernel.org; Eric Dumazet <edumazet@...gle.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@...csson.com]
>> 发送时间: 2016年11月1日 19:37
>> 收件人: 张谦; Ben Hutchings; Ying Xue
>> 抄送: netdev@...r.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@....cn]
>>> Sent: Tuesday, 01 November, 2016 02:35
>>> To: Ben Hutchings <ben@...adent.org.uk>; Jon Maloy
>>> <jon.maloy@...csson.com>; Ying Xue <ying.xue0@...il.com>
>>> Cc: netdev@...r.kernel.org; Eric Dumazet <edumazet@...gle.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@...adent.org.uk]
>>> 发送时间: 2016年10月21日 23:00
>>> 收件人: Jon Maloy; Ying Xue
>>> 抄送: netdev@...r.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@...adent.org.uk]
>>>>> Sent: Thursday, 20 October, 2016 12:40
>>>>>>> To: Jon Maloy <jon.maloy@...csson.com>; Ying Xue
>>>>>>> <ying.xue0@...il.com>
>>>>>>>>> Cc: netdev@...r.kernel.org; Qian Zhang
>>>>>>>>> <zhangqian-c@....cn>; Eric Dumazet
>>>>>>> <edumazet@...gle.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.
>