Re: [PATCH nft 3/3] parser_bison: fix length check for ifname in ifname_expr_alloc()

[Thomas Haller <thaller@redhat.com>]

On Mon, 2023-10-23 at 19:16 +0200, Pablo Neira Ayuso wrote:
> On Mon, Oct 23, 2023 at 07:15:10PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Oct 23, 2023 at 07:00:47PM +0200, Thomas Haller wrote:
> > > IFNAMSIZ is 16, and the allowed byte length of the name is one
> > > less than
> > > that. Fix the length check and adjust a test for covering the
> > > longest
> > > allowed interface name.
> > > 
> > > This is obviously a change in behavior, because previously
> > > interface
> > > names with length 16 were accepted and were silently truncated
> > > along the
> > > way. Now they are rejected as invalid.
> > > 
> > > Fixes: fa52bc225806 ('parser: reject zero-length interface
> > > names')
> > > Signed-off-by: Thomas Haller <thaller@redhat.com>
> > > ---
> > >  src/parser_bison.y                                | 3 ++-
> > >  tests/shell/testcases/chains/0042chain_variable_0 | 7 +------
> > >  2 files changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/src/parser_bison.y b/src/parser_bison.y
> > > index f0652ba651c6..9bfc3cdb2d12 100644
> > > --- a/src/parser_bison.y
> > > +++ b/src/parser_bison.y
> > > @@ -16,6 +16,7 @@
> > >  #include <stdio.h>
> > >  #include <inttypes.h>
> > >  #include <syslog.h>
> > > +#include <net/if.h>
> > >  #include <netinet/ip.h>
> > >  #include <netinet/tcp.h>
> > >  #include <netinet/if_ether.h>
> > > @@ -158,7 +159,7 @@ static struct expr *ifname_expr_alloc(const
> > > struct location *location,
> > >                 return NULL;
> > >         }
> > >  
> > > -       if (length > 16) {
> > > +       if (length >= IFNAMSIZ) {
> > >                 xfree(name);
> > >                 erec_queue(error(location, "interface name too
> > > long"), queue);
> > >                 return NULL;
> > > diff --git a/tests/shell/testcases/chains/0042chain_variable_0
> > > b/tests/shell/testcases/chains/0042chain_variable_0
> > > index 739dc05a1777..a4b929f7344c 100755
> > > --- a/tests/shell/testcases/chains/0042chain_variable_0
> > > +++ b/tests/shell/testcases/chains/0042chain_variable_0
> > > @@ -26,18 +26,13 @@ table netdev filter2 {
> > >  
> > >  rc=0
> > >  $NFT -f - <<< $EXPECTED || rc=$?
> > > -test "$rc" = 0
> > > +test "$rc" = 1
> > >  cat <<EOF | $DIFF -u <($NFT list ruleset) -
> > >  table netdev filter1 {
> > >         chain Main_Ingress1 {
> > >                 type filter hook ingress device "lo" priority -
> > > 500; policy accept;
> > >         }
> > >  }
> > > -table netdev filter2 {
> > > -       chain Main_Ingress2 {
> > > -               type filter hook ingress devices = {
> > > d23456789012345, lo } priority -500; policy accept;
> > > -       }
> > > -}
> > 
> > Please, do not remove it, fix this test.
> 
> Or maybe I am missing the reason for this change? It seems this
> d23456789012345 was added to your previous patch in this series.
> 
> 

The point is to add a test for the bug first.

Followed by the fix. And the change to the test shows how the fix
fixes.


Thomas