Re: [PATCH nft v2 1/6] osf: add version fingerprint support

[Fernando Fernandez Mancera <ffmancera@riseup.net>]

Hi,

On 3/15/19 6:13 PM, Pablo Neira Ayuso wrote:
> On Fri, Mar 15, 2019 at 11:03:33AM +0100, Phil Sutter wrote:
> [...]
>> On Thu, Mar 14, 2019 at 09:07:37PM +0100, Pablo Neira Ayuso wrote:
>> [...]
>>> The osf expression returns a string with the OS genre, and if thev
>>> version flag is set on, it appends the version to this string, ie.
>>> genre + version.
>>>
>>> This allows us to build maps, ie.
>>>
>>>         meta mark set osf genre { "linux" : 0x10, "windows" : 0x20, "macos" : 0x40 }
>>>
>>> But, with this new version, you could also do:
>>>
>>>         meta mark set osf genre { "linux::4.0" : 0x11, "linux::3.0" : 0x12, ...}
>>>
>>> and so on.
>>>
>>> So I see this version thing as a extended matching.
>>>
>>> The osf engine actually _already_ finds a precise matching, ie. genre
>>> + version, since the fingerprint is per genre + version. But you can
>>> just decide to match on the genre (eg. linux).
>>
>> The problem we're facing IMO is that nft_cmp is limited to a simple
>> memcmp(). This demands LHS to know what RHS contains. I'm not implying
>> it would be a good idea, but imagine nft_cmp could handle wildcards, we
>> could use "linux:*" to match on genre only, "linux:4.0:*" to match on
>> genre and version and even "linux:4.*" to match genre and major version
>> number.
>>
>> Actually we might be able to implement the above by setting 'len' field
>> correctly.
> 
> The wildcard at the end of the string already works out of the box
> via:
> 
>         iifname eth\*
> 
> The wildcard matching is generic, so it can be used from any string
> datatype, including the osf expression.
> 
>>>> Applying the same logic to osf expression, we would implement 'osf name
>>>> foo osf version 3.141' and add 'osf_try_merge()' routine to
>>>> 'rule_postprocess()' which tries to combine the two statements.
>>>> Obviously, this is quite a bit of extra work, not sure if feasible.
>>>
>>> I think the discussion here is the syntax, ie.
>>>
>>>         osf genre "Linux::4.10"
>>>
>>> vs.
>>>
>>>         osf genre "Linux" version "4.10"
>>>
>>> This only requires changes to the userspace nftables side, if you
>>> prefer this syntax, which is what I understand you would like to see,
>>> right?
>>
>> Not quite. I like how osf is an expression, not a statement. This makes
>> things like 'osf name != "Linux"' possible. What I didn't like was how
>> the proposed extension requires users to input redundant info:
>>
>> | osf name version "Linux:4.20"
>>
>> RHS contains the version number, so LHS should not need to have
>> "version" explicitly stated.
> 
> I see, then part of your discussion is focused on this syntax:
> 
>         osf name version "Linux:4.20"
> 
> in order to remove the "version" keyword there and make it more
> compact.
> 
>> On Thu, Mar 14, 2019 at 09:13:09PM +0100, Pablo Neira Ayuso wrote:
>> [...]
>>> I think we could even extend this later on to match things like:
>>>
>>> # Popular cluster config scripts disable timestamps and
>>> # selective ACK:
>>> S4:64:1:48:M1460,N,W0:          Linux:2.4:cluster:Linux 2.4 in cluster
>>>                                 ^^^^^^^^^^^^^^^^^
>>> Then, do:
>>>
>>>         os gente "Linux:2.4:cluster"
>>>
>>> by adding a new flag to match the "Subtype" field (according to the
>>> file description in pf.os).
>>
>> In an ideal world, we could match on any (combination of) fields in the
>> database. I am aware this is probably over-engineering. :)
> 
> We can probably achieve this with a more advance nft_cmp expression,
> that allows us to do some sort of limited regex matching. But I agree
> in that adding this only for the osf expression is probably too much.
> I don't like regex, they will use it for layer-7, and users do not
> understand the computational complexity of the regular expressions
> (they can easily ruin performance by adding a few expression that need
> to be search all over the packet payload).
> 
> Anyway, this is a different topic :-).

I agree with you about this. Using regex is too much in this case and
can ruin performance easily.

> 
>> What we could do though with little effort is to make use of the OS info
>> structure in database by making use of nft_cmp comparing only the first
>> 'len' bytes of data in registers. My idea would be that:
>>
>> * 'osf' expression always returns "full" data, i.e.: "OS:VER:SUB"
>> * nft_cmp compares that string to RHS up to RHS length
>>
>> So let's assume DB lookup returns "Windows:2003:AS:", then:
>>
>> osf name "Windows" -> match
>> osf name "Windows:" -> match
>> osf name "Windows:XP:" -> no match
>> osf name "Windows:2000:" -> no match
>> osf name "Windows:200" -> match
>>
>> So we have optional version match and even a poor-man's wildcard
>> functionality. Specifying the trailing semi-colon implicitly causes an
>> exact match on the last field.
>>
>> What do you think?
> 
> Hm, if we follow this path, this would need a bit more work, note
> that:
> 
> * nft userspace currently compares 16 bytes for the string case,
>   according to what I see via --debug=mnl.
> 
> * When the string is less than 16 bytes, it assumes it is a wildcard
>   matching and the end of the string.
> 
> * Kernel would need to inconditionally build the OS:version string.
> 
> * We may need to ask users to break existing osf ruleset so they
>   explicitly add the wildcard at the end, ie.
> 
>         osf name "Windows\*"
>         osf name "Linux:4.\*"
> 
> And the kernel would have no notion of what userspace is willing to
> match.

Following this path, IMO the users could have some problems with always
adding the wildcard at the end. I think that it is not natural to set
the wildcard at the end by default.

> 
> If the problem is the syntax, not the NFT_OSF_F_VERSION flags, we
> could explore this syntax:
> 
>         osf genre "Linux"
>         osf version "Linux:4.20"
> 
> then, in the future (if ever needed) add subtypes:
> 
>         osf subtype "Linux:2.4:cluster"
> 
> With flags in place, we would have a bit of knowledge of what the user
> is doing vs. matching part of a string.
> 
> Note that this would still allow you to do wildcard matching, ie:
> 
>         osf version "Linux:4.\*"
> 
> Thanks!
> 

I like more this solution, we can add an easily understandable
explanation about the usage of 'genre' and 'version' in the manual file.
Furthermore, this syntax is easy to extend if needed in the future as
Pablo said and it seems more natural to me.

We can hide the flags for the json support if needed by counting the
numbers of colons. Thanks! :-)