[nft PATCH 0/2] Fix for failing 'counter ipsec ...' rule

[nft PATCH 0/2] Fix for failing 'counter ipsec ...' rule

[Phil Sutter]

The following rule is rejected by the parser:

| oifname "s_c" counter packets 0 bytes 0 ipsec out ip daddr 192.168.1.2 counter name "ipsec_out"

For unknown reasons, COUNTER scope is not closed before parsing 'daddr'
which is not recognized in that scope.

This series adds a test case in patch 1 and a workaround in patch 2,
namely moving saddr/daddr keywords back to global scope. Eliminating the
whole COUNTER scope would also work, but is neither a real solution.

The fact that a scope closed three words ago still causes trouble proves
the concept is flawed. IMO one should abandon it and instead deploy
quoting of all user-defined strings on output and consequently allow all
user-defined strings to be quoted on input.

Phil Sutter (2):
  tests/py: Add a test for failing ipsec after counter
  Revert "scanner: remove saddr/daddr from initial state"

 src/scanner.l                 |  6 ++----
 tests/py/inet/ipsec.t         |  2 ++
 tests/py/inet/ipsec.t.json    | 21 +++++++++++++++++++++
 tests/py/inet/ipsec.t.payload |  6 ++++++
 4 files changed, 31 insertions(+), 4 deletions(-)

-- 
2.34.1

[nft PATCH 1/2] tests/py: Add a test for failing ipsec after counter

[Phil Sutter]

This is a bug in parser/scanner due to scoping:

| Error: syntax error, unexpected string, expecting saddr or daddr
| add rule ip ipsec-ip4 ipsec-forw counter ipsec out ip daddr 192.168.1.2
|                                                       ^^^^^

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 tests/py/inet/ipsec.t         |  2 ++
 tests/py/inet/ipsec.t.json    | 21 +++++++++++++++++++++
 tests/py/inet/ipsec.t.payload |  6 ++++++
 3 files changed, 29 insertions(+)

diff --git a/tests/py/inet/ipsec.t b/tests/py/inet/ipsec.t
index e924e9bcbdbc4..b18df395de6ce 100644
--- a/tests/py/inet/ipsec.t
+++ b/tests/py/inet/ipsec.t
@@ -19,3 +19,5 @@ ipsec in ip6 daddr dead::beef;ok
 ipsec out ip6 saddr dead::feed;ok
 
 ipsec in spnum 256 reqid 1;fail
+
+counter ipsec out ip daddr 192.168.1.2;ok
diff --git a/tests/py/inet/ipsec.t.json b/tests/py/inet/ipsec.t.json
index d7d3a03c21131..18a64f3533b34 100644
--- a/tests/py/inet/ipsec.t.json
+++ b/tests/py/inet/ipsec.t.json
@@ -134,3 +134,24 @@
         }
     }
 ]
+
+# counter ipsec out ip daddr 192.168.1.2
+[
+    {
+        "counter": null
+    },
+    {
+        "match": {
+            "left": {
+                "ipsec": {
+                    "dir": "out",
+                    "family": "ip",
+                    "key": "daddr",
+                    "spnum": 0
+                }
+            },
+            "op": "==",
+            "right": "192.168.1.2"
+        }
+    }
+]
diff --git a/tests/py/inet/ipsec.t.payload b/tests/py/inet/ipsec.t.payload
index c46a2263f6c01..9648255df02e9 100644
--- a/tests/py/inet/ipsec.t.payload
+++ b/tests/py/inet/ipsec.t.payload
@@ -37,3 +37,9 @@ ip ipsec-ip4 ipsec-forw
   [ xfrm load out 0 saddr6 => reg 1 ]
   [ cmp eq reg 1 0x0000adde 0x00000000 0x00000000 0xedfe0000 ]
 
+# counter ipsec out ip daddr 192.168.1.2
+ip ipsec-ip4 ipsec-forw
+  [ counter pkts 0 bytes 0 ]
+  [ xfrm load out 0 daddr4 => reg 1 ]
+  [ cmp eq reg 1 0x0201a8c0 ]
+
-- 
2.34.1

[nft PATCH 2/2] Revert "scanner: remove saddr/daddr from initial state"

[Phil Sutter]

This reverts commit df4ee3171f3e3c0e85dd45d555d7d06e8c1647c5 as it
breaks ipsec expression if preceeded by a counter statement:

| Error: syntax error, unexpected string, expecting saddr or daddr
| add rule ip ipsec-ip4 ipsec-forw counter ipsec out ip daddr 192.168.1.2
|                                                       ^^^^^

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/scanner.l | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/scanner.l b/src/scanner.l
index 7eb74020ef848..6d6396bbb7413 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -464,10 +464,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
 "bridge"		{ return BRIDGE; }
 
 "ether"			{ scanner_push_start_cond(yyscanner, SCANSTATE_ETH); return ETHER; }
-<SCANSTATE_ARP,SCANSTATE_CT,SCANSTATE_ETH,SCANSTATE_IP,SCANSTATE_IP6,SCANSTATE_EXPR_FIB,SCANSTATE_EXPR_IPSEC>{
-	"saddr"			{ return SADDR; }
-	"daddr"			{ return DADDR; }
-}
+"saddr"			{ return SADDR; }
+"daddr"			{ return DADDR; }
 "type"			{ scanner_push_start_cond(yyscanner, SCANSTATE_TYPE); return TYPE; }
 "typeof"		{ return TYPEOF; }
 
-- 
2.34.1

Re: [nft PATCH 2/2] Revert "scanner: remove saddr/daddr from initial state"

[Pablo Neira Ayuso]

On Thu, Jun 23, 2022 at 04:28:43PM +0200, Phil Sutter wrote:
> This reverts commit df4ee3171f3e3c0e85dd45d555d7d06e8c1647c5 as it
> breaks ipsec expression if preceeded by a counter statement:
> 
> | Error: syntax error, unexpected string, expecting saddr or daddr
> | add rule ip ipsec-ip4 ipsec-forw counter ipsec out ip daddr 192.168.1.2
> |                                                       ^^^^^

Please add a test covering this regression case.

Thanks

> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>  src/scanner.l | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/scanner.l b/src/scanner.l
> index 7eb74020ef848..6d6396bbb7413 100644
> --- a/src/scanner.l
> +++ b/src/scanner.l
> @@ -464,10 +464,8 @@ addrstring	({macaddr}|{ip4addr}|{ip6addr})
>  "bridge"		{ return BRIDGE; }
>  
>  "ether"			{ scanner_push_start_cond(yyscanner, SCANSTATE_ETH); return ETHER; }
> -<SCANSTATE_ARP,SCANSTATE_CT,SCANSTATE_ETH,SCANSTATE_IP,SCANSTATE_IP6,SCANSTATE_EXPR_FIB,SCANSTATE_EXPR_IPSEC>{
> -	"saddr"			{ return SADDR; }
> -	"daddr"			{ return DADDR; }
> -}
> +"saddr"			{ return SADDR; }
> +"daddr"			{ return DADDR; }
>  "type"			{ scanner_push_start_cond(yyscanner, SCANSTATE_TYPE); return TYPE; }
>  "typeof"		{ return TYPEOF; }
>  
> -- 
> 2.34.1
> 

Re: [nft PATCH 2/2] Revert "scanner: remove saddr/daddr from initial state"

[Pablo Neira Ayuso]

On Thu, Jun 23, 2022 at 05:41:46PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 23, 2022 at 04:28:43PM +0200, Phil Sutter wrote:
> > This reverts commit df4ee3171f3e3c0e85dd45d555d7d06e8c1647c5 as it
> > breaks ipsec expression if preceeded by a counter statement:
> > 
> > | Error: syntax error, unexpected string, expecting saddr or daddr
> > | add rule ip ipsec-ip4 ipsec-forw counter ipsec out ip daddr 192.168.1.2
> > |                                                       ^^^^^
> 
> Please add a test covering this regression case.

Oh, actually coming in a separated patch. Probably collapsing them is
better for git annotate, but your call.

Thanks

Re: [nft PATCH 2/2] Revert "scanner: remove saddr/daddr from initial state"

[Phil Sutter]

On Thu, Jun 23, 2022 at 05:42:30PM +0200, Pablo Neira Ayuso wrote:
> On Thu, Jun 23, 2022 at 05:41:46PM +0200, Pablo Neira Ayuso wrote:
> > On Thu, Jun 23, 2022 at 04:28:43PM +0200, Phil Sutter wrote:
> > > This reverts commit df4ee3171f3e3c0e85dd45d555d7d06e8c1647c5 as it
> > > breaks ipsec expression if preceeded by a counter statement:
> > > 
> > > | Error: syntax error, unexpected string, expecting saddr or daddr
> > > | add rule ip ipsec-ip4 ipsec-forw counter ipsec out ip daddr 192.168.1.2
> > > |                                                       ^^^^^
> > 
> > Please add a test covering this regression case.
> 
> Oh, actually coming in a separated patch. Probably collapsing them is
> better for git annotate, but your call.

ACK, will send a v2.