Re: [iptables PATCH 1/3] extensions: libebt_among: Fix for false positive match comparison

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Phil,

On Fri, Jul 21, 2023 at 11:59:55AM +0200, Phil Sutter wrote:
> Pablo,
> 
> On Mon, Jul 17, 2023 at 01:07:35PM +0200, Pablo Neira Ayuso wrote:
> > On Sat, Jul 15, 2023 at 02:59:26PM +0200, Phil Sutter wrote:
> > > When comparing matches for equality, trailing data in among match is not
> > > considered. Therefore two matches with identical pairs count may be
> > > treated as identical when the pairs actually differ.
> > 
> > By "trailing data", you mean the right-hand side of this?
> > 
> >         fe:ed:ba:be:00:01=10.0.0.1
> > 
> > > Matches' parsing callbacks have no access to the xtables_match itself,
> > > so can't update userspacesize field as needed.
> > > 
> > > To fix this, extend struct nft_among_data by a hash field to contain a
> > > DJB hash of the trailing data.
> > 
> > Is this DJB hash use subject to collisions?
> 
> Thanks for the heads-up. I suspected DJB hash algo might not be perfect
> when it comes to collisions, but "good enough" for the task. In fact,
> collisions are pretty common, so this approach is not a proper solution
> to the problem.
>
> Searching for other ways to fix the issue, I noticed that
> compare_matches() was deliberately changed to compare only the first
> 'userspacesize' bytes of extensions to avoid false-negatives caused by
> kernel-internals in extension data.

Indeed, that was a deliberate decision.

> I see two different solutions and would like to hear your opinion. First
> one is a hack, special treatment for among match in compare_matches():
> 
> | @@ -381,6 +381,7 @@ bool compare_matches(struct xtables_rule_match *mt1,
> |         for (mp1 = mt1, mp2 = mt2; mp1 && mp2; mp1 = mp1->next, mp2 = mp2->next) {
> |                 struct xt_entry_match *m1 = mp1->match->m;
> |                 struct xt_entry_match *m2 = mp2->match->m;
> | +               size_t cmplen = mp1->match->userspacesize;
> |  
> |                 if (strcmp(m1->u.user.name, m2->u.user.name) != 0) {
> |                         DEBUGP("mismatching match name\n");
> | @@ -392,8 +393,10 @@ bool compare_matches(struct xtables_rule_match *mt1,
> |                         return false;
> |                 }
> |  
> | -               if (memcmp(m1->data, m2->data,
> | -                          mp1->match->userspacesize) != 0) {
> | +               if (!strcmp(m1->u.user.name, "among"))
> | +                       cmplen = m1->u.match_size - sizeof(*m1);
> | +
> | +               if (memcmp(m1->data, m2->data, cmplen) != 0) {
> |                         DEBUGP("mismatch match data\n");
> |                         return false;
> |                 }

This incremental update is relatively simple and it is only 'among'
that requires this special handling. Maybe you start with this, also
placing a comment to describe the intention for this particular case.
I don't remember if among allows to delete a rule with set elements
that are placed in different order.

Then, if you have to follow up because this is not enough...

> The second one is more generic, reusing extensions' 'udata' pointer. One
> could make xtables_option_{m,t}fcall() functions zero the scratch area
> if present (so locally created extensions match ones fetched from
> kernel) and compare that scratch area in compare_matches(). For among
> match, using the scratch area to store pairs is fine.

then pursue this second approach?

Thanks for explaining.