Re: Netfilter: match "tcp option" with the same type present multiple times

[Fernando Fernandez Mancera <fmancera@suse.de>]

On 10/7/26 1:29 AM, Pablo Neira Ayuso wrote:
> Hi Matthieu,
> 
> On Wed, Sep 30, 2026 at 08:33:36PM +0200, Matthieu Baerts wrote:
>> Hi Pablo,
>>
>> Thank you for your reply!
>>
>> On 29/09/2026 14:03, Pablo Neira Ayuso wrote:
>>> On Tue, Sep 29, 2026 at 12:40:47PM +0200, Matthieu Baerts wrote:
>>>> On 29/09/2026 12:00, Pablo Neira Ayuso wrote:
>>>>> On Tue, Sep 29, 2026 at 11:21:15AM +0200, Matthieu Baerts wrote:
>>>>>> Hi Pablo,
>>>>>>
>>>>>> Thank you for your reply!
>>>>>>
>>>>>> On 28/09/2026 23:52, Pablo Neira Ayuso wrote:
>>>>>>> Hi Mattieu,
>>>>>>>
>>>>>>> On Mon, Sep 28, 2026 at 02:13:16PM +0200, Matthieu Baerts wrote:
>>>>>>>> Hello Netfilter devs,
>>>>>>>>
>>>>>>>> First, thank you for maintaining Netfilter in the kernel and the
>>>>>>>> userspace tools.
>>>>>>>>
>>>>>>>> The MPTCP selftests are switching from IPTables to NFTables, and
>>>>>>>> Clashiko reported that this part of a rule wouldn't match anything:
>>>>>>>>
>>>>>>>>    tcp option mptcp subtype remove-addr drop
>>>>>>>>
>>>>>>>> When an MPTCP REMOVE_ADDR suboption (type 0x30, len >=4, subtype 0x4) is
>>>>>>>> added to the TCP options, it is added after an MPTCP DSS option (type
>>>>>>>> 0x30, len >= 8, subtype 0x2). In other words, there will be two MPTCP
>>>>>>>> (type 30) options in the TCP options. It looks like Netfilter doesn't
>>>>>>>> handle that, because it stops processing other TCP options when the
>>>>>>>> expected type is found:
>>>>>>>>
>>>>>>>> net/netfilter/nft_exthdr.c:nft_exthdr_tcp_eval() {
>>>>>>>>      ...
>>>>>>>> 	for (i = sizeof(*tcph); i < tcphdr_len - 1; i += optl) {
>>>>>>>> 		optl = optlen(opt, i);
>>>>>>>>
>>>>>>>> 		if (priv->type != opt[i])
>>>>>>>> 			continue;
>>>>>>>> 		...
>>>>>>>> 		return;  // <== it will look at the first MPTCP option
>>>>>>>> 	}
>>>>>>>>      ...
>>>>>>>> }
>>>>>>>>
>>>>>>>> It looks like it shouldn't stop if the wrong subtype is found, but the
>>>>>>>> subtype is not compared there if I'm not mistaken. Should there be a fix
>>>>>>>> to support this case?
>>>>>>>
>>>>>>> Would it work for you if you can specify what TCP option you want to
>>>>>>> match? ie.
>>>>>>>   
>>>>>>>      tcp option[2] mptcp subtype remove-addr drop
>>>>>>>                 ^
>>>>>>>                 |
>>>>>>>                 |
>>>>>>>         allow to specify a match at a given tcp option
>>>>>>
>>>>>> In my specific use-case, yes it would work. Some MPTCP sub-options will
>>>>>> always be after another one... when the Linux stack is used.
>>>>>
>>>>> This would require a smaller patch.
>>>>
>>>> I understand. But I guess that still means adding a new option.
>>>>
>>>> In my case, it would work, but this might be confusing for people who
>>>> want to use this filter: they need to know if a suboption can be used
>>>> with another one. I guess they could have multiple rules to check the
>>>> presence of a suboption in the first matched type, and in a second one,
>>>> but that seems confusing. Or maybe not?
>>>
>>> You mean, the might want to validate the entire list of suboptions in
>>> a particular order, correct? ie. first suboption A then suboption B.
>>
>> Yes, that's one possibility. But initially, I was thinking about having
>> two rules (or a set?) to match "the MPTCP subtype is either in the first
>> MPTCP option, or the second one (if any)". (Or nft could always say with
>> MPTCP by default, check also for a second MPTCP option, if any)
>>
>>>>>>> Or would this be too strict for your use-case and you would prefer
>>>>>>> that you can match it anywhere?
>>>>>>
>>>>>> I think it would make more sense to match it anywhere. For example, an
>>>>>> MP_RESET can be used alone, or after an MP_FASTCLOSE. Plus some stacks
>>>>>> could reorder the options as there is no imposed order.
>>>>>
>>>>> I think this would require a MPTCP subtype parser, ie. make kernel
>>>>> aware of MPTCP subtypes.
>>>>
>>>> In nft_exthdr_tcp_eval(), why can the comparison not be done there
>>>> instead of copying the data in a register, and compare later on? If the
>>>> comparison is done directly for a given type and is different, the code
>>>> could continue and check for the same type. (Or store in multiple
>>>> registers.)
>>>
>>> In nftables, the fetch then cmp instructions are splitted, ie. you
>>> first retrieve then you compare (or make a set lookup with it).
>>> Builtin comparison should be possible, but it defeats the integration
>>> with the set infrastructure.
>>
>> OK, I see.
>>
>> If someone gives ...
>>
>>    tcp option mptcp subtype { mp-capable, mp-join, remove-addr } drop
>>
>> ... a bitmap could be used, but then that's specific to MPTCP I suppose.
> 
> I think this makes sense from user perspective.
> 
> The nft_exthdr expression needs iterate over the whole list of tcp
> options, then if type is 30, make a set lookup to check if the subtype
> value is in the set.
> 
> Something like:
> 
>   - loop
>   |  r1 <- exthdr     [ if no more tcp options, break loop ]
>   |  r2 <- lookup(r1) [ if found, break loop ]
> 
> but exthdr need to be taugh to resume from the last visited tcp
> option. A new loop expression would wrap these two exthdr and lookup
> expression (similar to nft_inner) and store the iteration context (ie.
> last visited tcp option to continue from there).
> 
> exthdr tcp option support needs advertise a new NFT_EXPR_LOOP flag, so
> it can be used with within this new nft_loop expression.
> 

Wouldn't this be too complex? I was thinking in something a bit more 
generic and simpler. E.g a new NFTA_EXTHDR_INNER_TYPE which will work 
with PRESENT flag.

That way one can match MPTCP options exactly the same way TCP options 
are matched. The implementation would look first for the TCP option 30 
and then check the subtype, if it matches return 1 otherwise continue.

This is chained with a CMP_EQ expression.

I have a working patch, let me polish the code and will send it as RFC 
today so we can discuss it. I like this idea because it could be 
extended to match similar situations in IPv6 headers or SCTP.

> Another question: Would you still like to validate that DSS comes
> before REMOVE_ADDR?
> 
>>>> I don't know if you need something specific to MPTCP, maybe just a way
>>>> to check all options with the same type.
>>>
>>> Going back to this:
>>>
>>> "When an MPTCP REMOVE_ADDR suboption (type 0x30, len >=4, subtype 0x4) is
>>> added to the TCP options, it is added after an MPTCP DSS option (type
>>> 0x30, len >= 8, subtype 0x2)"
>>>
>>> Would you like to check for this particular sequence, right? Then
>>
>> In my case, no particularly -- I just want to count/drop REMOVE_ADDR to
>> catch kernel regressions in the selftests -- but I can.
>>
>>> position-based solution would work, because this would allow to match
>>> strictly:
>>>
>>> pos = 0, suboption DSS
>>> pos = 1, suboption REMOVE_ADDR
>>>
>>> This ruleset might break the protocol if there is ever a suboption
>>> before DSS.
>>
>> Indeed. Technically, with MPTCP, we could send the REMOVE_ADDR alone
>> (without DSS), or before the DSS, or with another one in between if
>> there is room.
> 
> Position matching is too strict.
> 
> You can still make it with the raw expression, but I think the native
> matching representation should be more flexible too as you describe.
> 
>>> Loose mode is more protocol designer friendly as it does not constrain
>>> future updates in the protocol:
>>>
>>> "please find MP-TCP suboption REMOVE_ADDR for me"
>>>
>>> but security folks might probably say this is ... loose, because it
>>> does not validate DSS before and REMOVE_ADDR might come anywhere.
>>> Protocol designer would say that this is good for them because this
>>> firewall does not constrain future enhancements of the protocol (still
>>> raw expressions are possible, then this last statement does not hold
>>> anymore).
>>>
>>> Which one would you pick? :-)
>>
>> I would first prefer if people don't drop packets with MPTCP options :-D
>>
>> Honestly, I'm not sure what people would be interested in doing in
>> production, but I guess it would be more: "please match packet with
>> MPTCP suboption X, no matter the order".
> 
> Yes, as discussed, this is more protocol friendly too in terms of
> extensibility.
> 
>> If people are interested in a specific packet where MPTCP suboptions
>> are in a specific order, with a specific length, I *think* they can
>> use a raw payload expression instead, no?
>>
>> In my case, I just wanted to drop packets with a specific MPTCP
>> suboption. It happens that I know exactly which packet I want to block,
>> and I can use a raw payload expression, but a simpler rule would be
>> welcome :)
> 
> Makes sense.
> 
> This would need an extension in the kernel as Florian has anticipated.