[nft] Regarding `tcp flags` (and a potential bug)

[nft] Regarding `tcp flags` (and a potential bug)

[Tom Yan]

Hi all,

I'm a bit uncertain how `tcp flags` works exactly. I once thought `tcp
flags syn` checks whether "syn and only syn is set", but after tests,
it looks more like it checks only whether "syn is set" (and it appears
that the right expression for the former is `tcp flags == syn`):

# nft add rule meh tcp_flags 'tcp flags syn'
# nft add rule meh tcp_flags 'tcp flags ! syn'
# nft add rule meh tcp_flags 'tcp flags == syn'
# nft add rule meh tcp_flags 'tcp flags != syn'
# nft list table meh
table ip meh {
    chain tcp_flags {
        tcp flags syn
        tcp flags ! syn
        tcp flags == syn
        tcp flags != syn
    }
}

Then I test the above respectively with a flag mask:

# nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) syn'
# nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) ! syn'
# nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) == syn'
# nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) != syn'
# nft list table meh
table ip meh {
    chain tcp_flags {
        tcp flags & (fin | syn | rst | ack) syn
        tcp flags & (fin | syn | rst | ack) ! syn
        tcp flags syn / fin,syn,rst,ack
        tcp flags syn / fin,syn,rst,ack
    }
}

I don't suppose the mask in the first two rules would matter. And with
`tcp flags syn / fin,syn,rst,ack`, I assume it would be false when
"syn is cleared and/or any/all of fin/rst/ack is/are set"?

Also, as you can see, for the last two rules, `nft` interpreted them
as an identical rule, which I assume to be a bug. These does NOT seem
to workaround it either:

# nft flush chain meh tcp_flags
# nft add rule meh tcp_flags 'tcp flags == syn / fin,syn,rst,ack'
# nft add rule meh tcp_flags 'tcp flags != syn / fin,syn,rst,ack'
# nft list table meh
table ip meh {
    chain tcp_flags {
        tcp flags syn / fin,syn,rst,ack
        tcp flags syn / fin,syn,rst,ack
    }
}

I'm not sure if `! --syn` in iptables (legacy) is affected by this as
well. Anyway, I'm doing the following for now as a workaround:

# nft flush chain meh tcp_flags
# nft add rule meh tcp_flags 'tcp flags ! syn reject with tcp reset'
# nft add rule meh tcp_flags 'tcp flags { fin, rst, ack } reject with tcp reset'
# nft list table meh
table ip meh {
    chain tcp_flags {
        tcp flags ! syn reject with tcp reset
        # syn: 1, other bits: not checked
        tcp flags { fin, rst, ack } reject with tcp reset
        # syn: 1, fin: 0, rst: 0, ack: 0, other bits: not checked
        ct state != invalid accept
    }
}

Are the comments in above correct? Are any of the assumptions in this
email incorrect?

As a side question, is it even possible that any packet will be
considered `invalid` with (syn: 1, fin: 0, rst: 0, ack: 0)?

Thanks in advance!

Regards,
Tom

Re: [nft] Regarding `tcp flags` (and a potential bug)

[Tom Yan]

Just noticed something that is even worse:

# nft add rule meh tcp_flags 'tcp flags { fin, rst, ack }'
# nft add rule meh tcp_flags 'tcp flags == { fin, rst, ack }'
# nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) != 0'
# nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) == 0'
# nft list table meh
table ip meh {
    chain tcp_flags {
        tcp flags { fin, rst, ack }
        tcp flags { fin, rst, ack }
        tcp flags fin,rst,ack
        tcp flags ! fin,rst,ack
    }
}

On Tue, 27 Jul 2021 at 19:18, Tom Yan <tom.ty89@gmail.com> wrote:
>
> Hi all,
>
> I'm a bit uncertain how `tcp flags` works exactly. I once thought `tcp
> flags syn` checks whether "syn and only syn is set", but after tests,
> it looks more like it checks only whether "syn is set" (and it appears
> that the right expression for the former is `tcp flags == syn`):
>
> # nft add rule meh tcp_flags 'tcp flags syn'
> # nft add rule meh tcp_flags 'tcp flags ! syn'
> # nft add rule meh tcp_flags 'tcp flags == syn'
> # nft add rule meh tcp_flags 'tcp flags != syn'
> # nft list table meh
> table ip meh {
>     chain tcp_flags {
>         tcp flags syn
>         tcp flags ! syn
>         tcp flags == syn
>         tcp flags != syn
>     }
> }
>
> Then I test the above respectively with a flag mask:
>
> # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) syn'
> # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) ! syn'
> # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) == syn'
> # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) != syn'
> # nft list table meh
> table ip meh {
>     chain tcp_flags {
>         tcp flags & (fin | syn | rst | ack) syn
>         tcp flags & (fin | syn | rst | ack) ! syn
>         tcp flags syn / fin,syn,rst,ack
>         tcp flags syn / fin,syn,rst,ack
>     }
> }
>
> I don't suppose the mask in the first two rules would matter. And with
> `tcp flags syn / fin,syn,rst,ack`, I assume it would be false when
> "syn is cleared and/or any/all of fin/rst/ack is/are set"?
>
> Also, as you can see, for the last two rules, `nft` interpreted them
> as an identical rule, which I assume to be a bug. These does NOT seem
> to workaround it either:
>
> # nft flush chain meh tcp_flags
> # nft add rule meh tcp_flags 'tcp flags == syn / fin,syn,rst,ack'
> # nft add rule meh tcp_flags 'tcp flags != syn / fin,syn,rst,ack'
> # nft list table meh
> table ip meh {
>     chain tcp_flags {
>         tcp flags syn / fin,syn,rst,ack
>         tcp flags syn / fin,syn,rst,ack
>     }
> }
>
> I'm not sure if `! --syn` in iptables (legacy) is affected by this as
> well. Anyway, I'm doing the following for now as a workaround:
>
> # nft flush chain meh tcp_flags
> # nft add rule meh tcp_flags 'tcp flags ! syn reject with tcp reset'
> # nft add rule meh tcp_flags 'tcp flags { fin, rst, ack } reject with tcp reset'
> # nft list table meh
> table ip meh {
>     chain tcp_flags {
>         tcp flags ! syn reject with tcp reset
>         # syn: 1, other bits: not checked
>         tcp flags { fin, rst, ack } reject with tcp reset
>         # syn: 1, fin: 0, rst: 0, ack: 0, other bits: not checked
>         ct state != invalid accept
>     }
> }
>
> Are the comments in above correct? Are any of the assumptions in this
> email incorrect?
>
> As a side question, is it even possible that any packet will be
> considered `invalid` with (syn: 1, fin: 0, rst: 0, ack: 0)?
>
> Thanks in advance!
>
> Regards,
> Tom

Re: [nft] Regarding `tcp flags` (and a potential bug)

[Florian Westphal]

Tom Yan <tom.ty89@gmail.com> wrote:
> Just noticed something that is even worse:
> 
> # nft add rule meh tcp_flags 'tcp flags { fin, rst, ack }'
> # nft add rule meh tcp_flags 'tcp flags == { fin, rst, ack }'

These two are identical.

> # nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) != 0'

This matches if any one of fin/rst/ack is set.

> # nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) == 0'

This matches if fin/rst/ack are all 0 (not set).

> # nft list table meh
> table ip meh {
>     chain tcp_flags {
>         tcp flags { fin, rst, ack }
>         tcp flags { fin, rst, ack }
>         tcp flags fin,rst,ack
>         tcp flags ! fin,rst,ack
>     }
> }

Can you elaborate?

This looks correct to me.

> > # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) ! syn'

Its unfortunate nft accepts this.  The trailing ! syn is nonsensical.

This is equal to tcp flags ! syn.

> > # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) == syn'
> > # nft add rule meh tcp_flags 'tcp flags & (fin | syn | rst | ack) != syn'
> > # nft list table meh
> > table ip meh {
> >     chain tcp_flags {
> >         tcp flags & (fin | syn | rst | ack) syn
> >         tcp flags & (fin | syn | rst | ack) ! syn
> >         tcp flags syn / fin,syn,rst,ack
> >         tcp flags syn / fin,syn,rst,ack
> >     }
> > }
> >
> > I don't suppose the mask in the first two rules would matter. And with
> > `tcp flags syn / fin,syn,rst,ack`, I assume it would be false when
> > "syn is cleared and/or any/all of fin/rst/ack is/are set"?
> >
> > Also, as you can see, for the last two rules, `nft` interpreted them
> > as an identical rule, which I assume to be a bug. These does NOT seem
> > to workaround it either:
> >
> > # nft flush chain meh tcp_flags
> > # nft add rule meh tcp_flags 'tcp flags == syn / fin,syn,rst,ack'
> > # nft add rule meh tcp_flags 'tcp flags != syn / fin,syn,rst,ack'
> > # nft list table meh
> > table ip meh {
> >     chain tcp_flags {
> >         tcp flags syn / fin,syn,rst,ack
> >         tcp flags syn / fin,syn,rst,ack

Seems the reverse translation is broken, the negation is lost.
The rule is added correctly (i.e., flags == syn vs. != syn adds
different rules, see nft --debug=netlink add ..

Re: [nft] Regarding `tcp flags` (and a potential bug)

[Pablo Neira Ayuso]

On Tue, Jul 27, 2021 at 10:52:39PM +0800, Tom Yan wrote:
> Just noticed something that is even worse:
> 
> # nft add rule meh tcp_flags 'tcp flags { fin, rst, ack }'
> # nft add rule meh tcp_flags 'tcp flags == { fin, rst, ack }'
> # nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) != 0'
> # nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) == 0'
> # nft list table meh
> table ip meh {
>     chain tcp_flags {
>         tcp flags { fin, rst, ack }
>         tcp flags { fin, rst, ack }
>         tcp flags fin,rst,ack
>         tcp flags ! fin,rst,ack
>     }
> }

Could you develop the issue you're seeing here?

Re: [nft] Regarding `tcp flags` (and a potential bug)

[Tom Yan]

Well, same problem as what I mentioned in the last reply from me for
your patch: inconsistent / unexpected meaning of the syntax.

As of the current code (or even according to what you said / implied
"should and would still be right"), `tcp flags syn` checks and checks
only whether the syn bit is on:

# nft --debug=netlink add rule meh tcp_flags 'tcp flags syn'
ip
  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ bitwise reg 1 = ( reg 1 & 0x00000002 ) ^ 0x00000000 ]
  [ cmp neq reg 1 0x00000000 ]

which I consider to be a really bad (or even wrong) "shortcut",
because it is so unexpected that it is not the same as this:

# nft --debug=netlink add rule meh tcp_flags 'tcp flags { syn }'
__set%d meh 3
__set%d meh 0
    element 00000002  : 0 [end]
ip
  [ meta load l4proto => reg 1 ]
  [ cmp eq reg 1 0x00000006 ]
  [ payload load 1b @ transport header + 13 => reg 1 ]
  [ lookup reg 1 set __set%d ]

Probably because `{ }` implies a `==`. And as per what you've written
in your other reply to me, apparently a mask implies one as well. So
many things implied. So "neat". But no one (at least not me) knows
what the different syntaxes mean anymore until they --debug=netlink...



On Wed, 28 Jul 2021 at 05:11, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>
> On Tue, Jul 27, 2021 at 10:52:39PM +0800, Tom Yan wrote:
> > Just noticed something that is even worse:
> >
> > # nft add rule meh tcp_flags 'tcp flags { fin, rst, ack }'
> > # nft add rule meh tcp_flags 'tcp flags == { fin, rst, ack }'
> > # nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) != 0'
> > # nft add rule meh tcp_flags 'tcp flags & ( fin | rst | ack ) == 0'
> > # nft list table meh
> > table ip meh {
> >     chain tcp_flags {
> >         tcp flags { fin, rst, ack }
> >         tcp flags { fin, rst, ack }
> >         tcp flags fin,rst,ack
> >         tcp flags ! fin,rst,ack
> >     }
> > }
>
> Could you develop the issue you're seeing here?

Re: [nft] Regarding `tcp flags` (and a potential bug)

[Pablo Neira Ayuso]

On Thu, Jul 29, 2021 at 10:27:57AM +0800, Tom Yan wrote:
[...]
> As of the current code (or even according to what you said / implied
> "should and would still be right"), `tcp flags syn` checks and checks
> only whether the syn bit is on:

It is actually the same topic that you are discussing in several
emails: You don't seem to like that the implicit operation for the
bitmask datatype is not ==.

Fair enough.

[..]
> Probably because `{ }` implies a `==`.

Again the same argument from another email: As I said { } implies a
set, which implies an exact match on the value.

For most datatypes, the implicit operation implies '==', except for
the bitmask datatype.