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

[Phil Sutter <phil@nwl.cc>]

On Fri, Jul 21, 2023 at 03:56:13PM +0200, Pablo Neira Ayuso wrote:
> 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.

Yes, you did it! :)

> > 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...

Luckily, I had that in mind already and implemented element sorting in
the among parser, it should match how the kernel returns the elements.

> > 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?

ACK, I'll keep that around somewhere. For now that special casing above
is probably fine.

Thanks, Phil