netdev - Re: [PATCH net,v2] uapi: linux: restore IPPROTO_MAX to 256 and add IPPROTO_UAPI

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: <405a8fa2-4a71-71c8-7715-10d3d2301dac@tessares.net>
Date:   Wed, 12 Apr 2023 17:22:36 +0200
From:   Matthieu Baerts <matthieu.baerts@...sares.net>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Pablo Neira Ayuso <pablo@...filter.org>,
        netfilter-devel@...r.kernel.org, davem@...emloft.net,
        netdev@...r.kernel.org, pabeni@...hat.com, edumazet@...gle.com,
        mathew.j.martineau@...ux.intel.com, mptcp@...ts.linux.dev
Subject: Re: [PATCH net,v2] uapi: linux: restore IPPROTO_MAX to 256 and add
 IPPROTO_UAPI_MAX

Hi Jakub,

On 12/04/2023 16:21, Jakub Kicinski wrote:
> On Thu, 6 Apr 2023 12:45:25 +0200 Matthieu Baerts wrote:
>> The modification in the kernel looks good to me. But I don't know how to
>> make sure this will not have any impact on MPTCP on the userspace side,
>> e.g. somewhere before calling the socket syscall, a check could be done
>> to restrict the protocol number to IPPROTO_MAX and then breaking MPTCP
>> support.
> 
> Then again any code which stores the ipproto in an unsigned char will 
> be broken. A perfect solution is unlikely to exist.

I wonder if the root cause is not the fact we mix the usage of the
protocol parameter from the socket syscall (int/s32) and the protocol
field from the IP header (u8).

To me, the enum is linked to the socket syscall, not the IP header. In
this case, it would make sense to have a dedicated "MAX" macro for the
IP header, no?

>> Is it not safer to expose something new to userspace, something
>> dedicated to what can be visible on the wire?
>>
>> Or recommend userspace programs to limit to lower than IPPROTO_RAW
>> because this number is marked as "reserved" by the IANA anyway [1]?
>>
>> Or define something new linked to UINT8_MAX because the layer 4 protocol
>> field in IP headers is limited to 8 bits?
>> This limit is not supposed to be directly linked to the one of the enum
>> you modified. I think we could even say it works "by accident" because
>> "IPPROTO_RAW" is 255. But OK "IPPROTO_RAW" is there from the "beginning"
>> [2] :)
> 
> I'm not an expert but Pablo's patch seems reasonable to me TBH.
> Maybe I'm missing some extra MPTCP specific context?

I was imagining userspace programs doing something like:

    if (protocol < 0 || protocol >= IPPROTO_MAX)
        die();

    syscall(...);

With Pablo's modification and such userspace code, this will break MPTCP
support.

I'm maybe/probably worry for nothing, I don't know any specific lib
doing that and to be honest, I don't know what is usually done in libc
and libs implemented on top of that. On the other hand, it is hard to
guess how it is used everywhere.

So yes, maybe it is fine?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net