netdev - Re: [PATCH] net: socket: Always initialize family field at move_addr_to

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <b4083428-53b2-0f71-585f-d648b8fd1331@i-love.sakura.ne.jp>
Date:   Wed, 3 Apr 2019 06:07:40 +0900
From:   Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org,
        syzbot+0049bebbf3042dbd2e8f@...kaller.appspotmail.com,
        syzbot+915c9f99f3dbc4bd6cd1@...kaller.appspotmail.com
Subject: Re: [PATCH] net: socket: Always initialize family field at
 move_addr_to_kernel().

On 2019/04/03 5:23, David Miller wrote:
> From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> Date: Mon,  1 Apr 2019 23:19:22 +0900
> 
>> syzbot is reporting uninitialized value at rds_connect [1] and
>> rds_bind [2]. This is because syzbot is passing ulen == 0 whereas
>> these functions expects that it is safe to access sockaddr->family field
>> in order to determine minimal ulen size for validation. I noticed that
>> the same problem also exists in tomoyo_check_inet_address() function.
>>
>> Although the right fix might be to scatter around
>>
>>   if (ulen < sizeof(__kernel_sa_family_t))
>>     return 0;
>>
>> if the function wants to become no-op when the address is too short or
>>
>>   if (ulen < sizeof(__kernel_sa_family_t))
>>     return -EINVAL;
>>
>> if the function wants to reject when the address is too short, we can
>> avoid duplication (at e.g. LSM layer and protocol layer) if we make sure
>> that sockaddr->family field is always accessible.
>>
>> [1] https://syzkaller.appspot.com/bug?id=f4e61c010416c1e6f0fa3ffe247561b60a50ad71
>> [2] https://syzkaller.appspot.com/bug?id=a4bf9e41b7e055c3823fdcd83e8c58ca7270e38f
>>
>> Reported-by: syzbot <syzbot+0049bebbf3042dbd2e8f@...kaller.appspotmail.com>
>> Reported-by: syzbot <syzbot+915c9f99f3dbc4bd6cd1@...kaller.appspotmail.com>
>> Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
> 
> I do not think at all that it is wise to be OK with the socket address
> interpreation code ignoring the length.

They do check the length of socket address based on the family of socket address.
This patch tries to avoid branches by making sure that it is safe to read the family
of socket address (as if sockaddr->family and sockkaddr->addr are passed separately).

> 
> Please fix RDS and other protocols to examine the length properly
> instead.

Do you prefer adding branches only for allow reading the family of socket address?

> 
> Thank you.
>