[PATCH nft 0/3] parser: fix scope closing with > 1 nested scope

[PATCH nft 0/3] parser: fix scope closing with > 1 nested scope

[Florian Westphal]

This resolves the parsing problem reported by Phil.
First patch is his test case, second patch is an
(unrelated) fix for SYNPROXY scoping (lack of close).

Last patch fixes closing with nested scopes, where we exited
a scope too early.

Florian Westphal (2):
  parser: add missing synproxy scope closure
  scanner: don't pop active flex scanner scope

Phil Sutter (1):
  tests/py: Add a test for failing ipsec after counter

 include/parser.h              |  3 +++
 src/parser_bison.y            |  2 +-
 src/scanner.l                 | 11 +++++++++++
 tests/py/inet/ipsec.t         |  2 ++
 tests/py/inet/ipsec.t.json    | 21 +++++++++++++++++++++
 tests/py/inet/ipsec.t.payload |  6 ++++++
 6 files changed, 44 insertions(+), 1 deletion(-)

-- 
2.35.1

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

[Florian Westphal]

From: Phil Sutter <phil@nwl.cc>

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>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 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 e924e9bcbdbc..b18df395de6c 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 d7d3a03c2113..18a64f3533b3 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 c46a2263f6c0..9648255df02e 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.35.1

[PATCH nft 2/3] parser: add missing synproxy scope closure

[Florian Westphal]

Fixes: 232f2c3287fc ("scanner: synproxy: Move to own scope")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 src/parser_bison.y | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_bison.y b/src/parser_bison.y
index 2a0240fb9862..73a2702dcec1 100644
--- a/src/parser_bison.y
+++ b/src/parser_bison.y
@@ -2015,7 +2015,7 @@ map_block_obj_type	:	COUNTER	close_scope_counter { $$ = NFT_OBJECT_COUNTER; }
 			|	QUOTA	close_scope_quota { $$ = NFT_OBJECT_QUOTA; }
 			|	LIMIT	close_scope_limit { $$ = NFT_OBJECT_LIMIT; }
 			|	SECMARK close_scope_secmark { $$ = NFT_OBJECT_SECMARK; }
-			|	SYNPROXY { $$ = NFT_OBJECT_SYNPROXY; }
+			|	SYNPROXY close_scope_synproxy { $$ = NFT_OBJECT_SYNPROXY; }
 			;
 
 map_block		:	/* empty */	{ $$ = $<set>-1; }
-- 
2.35.1

[PATCH nft 3/3] scanner: don't pop active flex scanner scope

[Florian Westphal]

Currently we can pop a flex scope that is still active, i.e. the
scanner_pop_start_cond() for the scope has not been done.

Example:
  counter ipsec out ip daddr 192.168.1.2 counter name "ipsec_out"

Here, parser fails because 'daddr' is parsed as STRING, not as DADDR token.

Bug is as follows:
COUNTER changes scope to COUNTER. (COUNTER).
Next, IPSEC scope gets pushed, stack is: COUNTER, IPSEC.

Then, the 'COUNTER' scope close happens.  Because active scope has changed,
we cannot pop (we would pop the 'ipsec' scope in flex).
The pop operation gets delayed accordingly.

Next, IP gets pushed, stack is: COUNTER, IPSEC, IP, plus the information
that one scope closure/pop was delayed.

Then, the IP scope is closed.  Because a pop operation was delayed, we pop again,
which brings us back to COUNTER state.

This is bogus: The pop operation CANNOT be done yet, because the ipsec scope
is still open, but the existing code lacks the information to detect this.

After popping the IP scope, we must remain in IPSEC scope until bison
parser calls scanner_pop_start_cond(, IPSEC).

This adds a counter per flex scope so that we can detect this case.
In above case, after the IP scope gets closed, the "new" (previous)
scope (IPSEC) will be treated as active and its close is attempted again
on the next call to scanner_pop_start_cond().

After this patch, transition in above rule is:

push counter (COUNTER)
push IPSEC (COUNTER, IPSEC)
pop COUNTER (delayed: COUNTER, IPSEC, pending-pop for COUNTER),
push IP (COUNTER, IPSEC, IP, pending-pop for COUNTER)
pop IP (COUNTER, IPSEC, pending-pop for COUNTER)
parse DADDR (we're in IPSEC scope, its valid token)
pop IPSEC (pops all remaining scopes).

We could also resurrect the commit:
"scanner: flags: move to own scope", the test case passes with the
new scope closure logic.

Fixes: bff106c5b277 ("scanner: add support for scope nesting")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/parser.h |  3 +++
 src/scanner.l    | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/parser.h b/include/parser.h
index d8d2eb115922..2fb037cb8470 100644
--- a/include/parser.h
+++ b/include/parser.h
@@ -26,6 +26,7 @@ struct parser_state {
 	unsigned int			flex_state_pop;
 	unsigned int			startcond_type;
 	struct list_head		*cmds;
+	unsigned int			*startcond_active;
 };
 
 enum startcond_type {
@@ -81,6 +82,8 @@ enum startcond_type {
 	PARSER_SC_STMT_REJECT,
 	PARSER_SC_STMT_SYNPROXY,
 	PARSER_SC_STMT_TPROXY,
+
+	__SC_MAX
 };
 
 struct mnl_socket;
diff --git a/src/scanner.l b/src/scanner.l
index 7eb74020ef84..5741261a690a 100644
--- a/src/scanner.l
+++ b/src/scanner.l
@@ -1144,6 +1144,8 @@ void *scanner_init(struct parser_state *state)
 	yylex_init_extra(state, &scanner);
 	yyset_out(NULL, scanner);
 
+	state->startcond_active = xzalloc_array(__SC_MAX,
+						sizeof(*state->startcond_active));
 	return scanner;
 }
 
@@ -1173,6 +1175,8 @@ void scanner_destroy(struct nft_ctx *nft)
 	struct parser_state *state = yyget_extra(nft->scanner);
 
 	input_descriptor_list_destroy(state);
+	xfree(state->startcond_active);
+
 	yylex_destroy(nft->scanner);
 }
 
@@ -1181,6 +1185,7 @@ static void scanner_push_start_cond(void *scanner, enum startcond_type type)
 	struct parser_state *state = yyget_extra(scanner);
 
 	state->startcond_type = type;
+	state->startcond_active[type]++;
 
 	yy_push_state((int)type, scanner);
 }
@@ -1189,6 +1194,8 @@ void scanner_pop_start_cond(void *scanner, enum startcond_type t)
 {
 	struct parser_state *state = yyget_extra(scanner);
 
+	state->startcond_active[t]--;
+
 	if (state->startcond_type != t) {
 		state->flex_state_pop++;
 		return; /* Can't pop just yet! */
@@ -1198,6 +1205,10 @@ void scanner_pop_start_cond(void *scanner, enum startcond_type t)
 		state->flex_state_pop--;
 		state->startcond_type = yy_top_state(scanner);
 		yy_pop_state(scanner);
+
+		t = state->startcond_type;
+		if (state->startcond_active[t])
+			return;
 	}
 
 	state->startcond_type = yy_top_state(scanner);
-- 
2.35.1

Re: [PATCH nft 0/3] parser: fix scope closing with > 1 nested scope

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> This resolves the parsing problem reported by Phil.
> First patch is his test case, second patch is an
> (unrelated) fix for SYNPROXY scoping (lack of close).
> 
> Last patch fixes closing with nested scopes, where we exited
> a scope too early.
> 
> Florian Westphal (2):
>   parser: add missing synproxy scope closure
>   scanner: don't pop active flex scanner scope
>
> Phil Sutter (1):
>   tests/py: Add a test for failing ipsec after counter

Pushed to nftables.git