[nft PATCH 0/9] Misc JSON parser fixes

[nft PATCH 0/9] Misc JSON parser fixes

[Phil Sutter]

This is a series of memory corruption fixes kindly reported by Secunet.
The first six patches fix severe issues, patches seven and eight
moderate problems and the last one a minor issue noticed along the way.

Phil Sutter (9):
  parser_json: Catch wrong "reset" payload
  parser_json: Fix typo in json_parse_cmd_add_object()
  parser_json: Proper ct expectation attribute parsing
  parser_json: Fix flowtable prio value parsing
  parser_json: Fix limit object burst value parsing
  parser_json: Fix synproxy object mss/wscale parsing
  parser_json: Wrong check in json_parse_ct_timeout_policy()
  parser_json: Catch nonsense ops in match statement
  parser_json: Default meter size to zero

 src/parser_json.c | 50 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 18 deletions(-)

-- 
2.41.0

[nft PATCH 1/9] parser_json: Catch wrong "reset" payload

[Phil Sutter]

The statement happily accepted any valid expression as payload and
assumed it to be a tcpopt expression (actually, a special case of
exthdr). Add a check to make sure this is the case.

Standard syntax does not provide this flexibility, so no need to have
the check there as well.

Fixes: 5d837d270d5a8 ("src: add tcp option reset support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 4ea5b4326a900..242f05eece58c 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -2797,7 +2797,14 @@ static struct stmt *json_parse_optstrip_stmt(struct json_ctx *ctx,
 {
 	struct expr *expr = json_parse_expr(ctx, value);
 
-	return expr ? optstrip_stmt_alloc(int_loc, expr) : NULL;
+	if (!expr ||
+	    expr->etype != EXPR_EXTHDR ||
+	    expr->exthdr.op != NFT_EXTHDR_OP_TCPOPT) {
+		json_error(ctx, "Illegal TCP optstrip argument");
+		return NULL;
+	}
+
+	return optstrip_stmt_alloc(int_loc, expr);
 }
 
 static struct stmt *json_parse_stmt(struct json_ctx *ctx, json_t *root)
-- 
2.41.0

[nft PATCH 2/9] parser_json: Fix typo in json_parse_cmd_add_object()

[Phil Sutter]

A case of bad c'n'p in the fixed commit broke ct timeout objects
parsing.

Fixes: c7a5401943df8 ("parser_json: Fix for ineffective family value checks")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 242f05eece58c..045bee1d8edaa 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3570,7 +3570,7 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 			obj_free(obj);
 			return NULL;
 		}
-		obj->ct_helper.l3proto = l3proto;
+		obj->ct_timeout.l3proto = l3proto;
 
 		init_list_head(&obj->ct_timeout.timeout_list);
 		if (json_parse_ct_timeout_policy(ctx, root, obj)) {
-- 
2.41.0

[nft PATCH 3/9] parser_json: Proper ct expectation attribute parsing

[Phil Sutter]

Parts of the code were unsafe (parsing 'I' format into uint32_t), the
rest just plain wrong (parsing 'o' format into char *tmp). Introduce a
temporary int variable to parse into.

Fixes: 1dd08fcfa07a4 ("src: add ct expectations support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 045bee1d8edaa..92168b9ab580e 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3447,8 +3447,8 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 {
 	const char *family, *tmp, *rate_unit = "packets", *burst_unit = "bytes";
 	uint32_t l3proto = NFPROTO_UNSPEC;
+	int inv = 0, flags = 0, i;
 	struct handle h = { 0 };
-	int inv = 0, flags = 0;
 	struct obj *obj;
 	json_t *jflags;
 
@@ -3599,11 +3599,12 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 				return NULL;
 			}
 		}
-		if (!json_unpack(root, "{s:o}", "dport", &tmp))
-			obj->ct_expect.dport = atoi(tmp);
-		json_unpack(root, "{s:I}", "timeout", &obj->ct_expect.timeout);
-		if (!json_unpack(root, "{s:o}", "size", &tmp))
-			obj->ct_expect.size = atoi(tmp);
+		if (!json_unpack(root, "{s:i}", "dport", &i))
+			obj->ct_expect.dport = i;
+		if (!json_unpack(root, "{s:i}", "timeout", &i))
+			obj->ct_expect.timeout = i;
+		if (!json_unpack(root, "{s:i}", "size", &i))
+			obj->ct_expect.size = i;
 		break;
 	case CMD_OBJ_LIMIT:
 		obj->type = NFT_OBJECT_LIMIT;
-- 
2.41.0

[nft PATCH 4/9] parser_json: Fix flowtable prio value parsing

[Phil Sutter]

Using format specifier 'I' requires a 64bit variable to write into. The
temporary variable 'prio' is of type int, though.

Fixes: 586ad210368b7 ("libnftables: Implement JSON parser")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 92168b9ab580e..1921f301c51be 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3374,7 +3374,7 @@ static struct cmd *json_parse_cmd_add_flowtable(struct json_ctx *ctx,
 	if (op == CMD_DELETE || op == CMD_LIST || op == CMD_DESTROY)
 		return cmd_alloc(op, cmd_obj, &h, int_loc, NULL);
 
-	if (json_unpack_err(ctx, root, "{s:s, s:I}",
+	if (json_unpack_err(ctx, root, "{s:s, s:i}",
 			    "hook", &hook,
 			    "prio", &prio)) {
 		handle_free(&h);
-- 
2.41.0

[nft PATCH 5/9] parser_json: Fix limit object burst value parsing

[Phil Sutter]

The field is of type uint32_t, use lower case 'i' format specifier.

Fixes: c36288dbe2ba3 ("JSON: Fix parsing and printing of limit objects")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 1921f301c51be..2b244783c442d 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3616,7 +3616,7 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 		}
 		json_unpack(root, "{s:s}", "rate_unit", &rate_unit);
 		json_unpack(root, "{s:b}", "inv", &inv);
-		json_unpack(root, "{s:I}", "burst", &obj->limit.burst);
+		json_unpack(root, "{s:i}", "burst", &obj->limit.burst);
 		json_unpack(root, "{s:s}", "burst_unit", &burst_unit);
 
 		if (!strcmp(rate_unit, "packets")) {
-- 
2.41.0

[nft PATCH 6/9] parser_json: Fix synproxy object mss/wscale parsing

[Phil Sutter]

The fields are 16 and 8 bits in size, introduce temporary variables to
parse into.

Fixes: f44ab88b1088e ("src: add synproxy stateful object support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 2b244783c442d..8ec11083f463c 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3447,7 +3447,7 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 {
 	const char *family, *tmp, *rate_unit = "packets", *burst_unit = "bytes";
 	uint32_t l3proto = NFPROTO_UNSPEC;
-	int inv = 0, flags = 0, i;
+	int inv = 0, flags = 0, i, j;
 	struct handle h = { 0 };
 	struct obj *obj;
 	json_t *jflags;
@@ -3634,11 +3634,12 @@ static struct cmd *json_parse_cmd_add_object(struct json_ctx *ctx,
 	case CMD_OBJ_SYNPROXY:
 		obj->type = NFT_OBJECT_SYNPROXY;
 		if (json_unpack_err(ctx, root, "{s:i, s:i}",
-				    "mss", &obj->synproxy.mss,
-				    "wscale", &obj->synproxy.wscale)) {
+				    "mss", &i, "wscale", &j)) {
 			obj_free(obj);
 			return NULL;
 		}
+		obj->synproxy.mss = i;
+		obj->synproxy.wscale = j;
 		obj->synproxy.flags |= NF_SYNPROXY_OPT_MSS;
 		obj->synproxy.flags |= NF_SYNPROXY_OPT_WSCALE;
 		if (!json_unpack(root, "{s:o}", "flags", &jflags)) {
-- 
2.41.0

[nft PATCH 7/9] parser_json: Wrong check in json_parse_ct_timeout_policy()

[Phil Sutter]

The conditional around json_unpack() was meant to accept a missing
policy attribute. But the accidentally inverted check made the function
either ignore a given policy or access uninitialized memory.

Fixes: c82a26ebf7e9f ("json: Add ct timeout support")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 8ec11083f463c..e33c470c7e224 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -3415,7 +3415,7 @@ static int json_parse_ct_timeout_policy(struct json_ctx *ctx,
 	json_t *tmp, *val;
 	const char *key;
 
-	if (!json_unpack(root, "{s:o}", "policy", &tmp))
+	if (json_unpack(root, "{s:o}", "policy", &tmp))
 		return 0;
 
 	if (!json_is_object(tmp)) {
-- 
2.41.0

[nft PATCH 8/9] parser_json: Catch nonsense ops in match statement

[Phil Sutter]

Since expr_op_symbols array includes binary operators and more, simply
checking the given string matches any of the elements is not sufficient.

Fixes: 586ad210368b7 ("libnftables: Implement JSON parser")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index e33c470c7e224..15bb79a52bcc0 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -1725,13 +1725,18 @@ static struct stmt *json_parse_match_stmt(struct json_ctx *ctx,
 		    !strcmp(opstr, expr_op_symbols[op]))
 			break;
 	}
-	if (op == __OP_MAX) {
+	switch (op) {
+	case OP_EQ ... OP_NEG:
+		break;
+	case __OP_MAX:
 		if (!strcmp(opstr, "in")) {
 			op = OP_IMPLICIT;
-		} else {
-			json_error(ctx, "Unknown relational op '%s'.", opstr);
-			return NULL;
+			break;
 		}
+		/* fall through */
+	default:
+		json_error(ctx, "Invalid relational op '%s'.", opstr);
+		return NULL;
 	}
 
 	left = json_parse_expr(ctx, jleft);
-- 
2.41.0

[nft PATCH 9/9] parser_json: Default meter size to zero

[Phil Sutter]

JSON parser was missed when performing the same change in standard
syntax parser.

Fixes: c2cad53ffc22a ("meters: do not set a defaut meter size from userspace")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 src/parser_json.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/parser_json.c b/src/parser_json.c
index 15bb79a52bcc0..7549fc85c48b2 100644
--- a/src/parser_json.c
+++ b/src/parser_json.c
@@ -2687,7 +2687,7 @@ static struct stmt *json_parse_meter_stmt(struct json_ctx *ctx,
 	json_t *jkey, *jstmt;
 	struct stmt *stmt;
 	const char *name;
-	uint32_t size = 0xffff;
+	uint32_t size = 0;
 
 	if (json_unpack_err(ctx, value, "{s:s, s:o, s:o}",
 			    "name", &name, "key", &jkey, "stmt", &jstmt))
-- 
2.41.0

Re: [nft PATCH 0/9] Misc JSON parser fixes

[Phil Sutter]

On Wed, Sep 20, 2023 at 10:57:18PM +0200, Phil Sutter wrote:
> This is a series of memory corruption fixes kindly reported by Secunet.
> The first six patches fix severe issues, patches seven and eight
> moderate problems and the last one a minor issue noticed along the way.
> 
> Phil Sutter (9):
>   parser_json: Catch wrong "reset" payload
>   parser_json: Fix typo in json_parse_cmd_add_object()
>   parser_json: Proper ct expectation attribute parsing
>   parser_json: Fix flowtable prio value parsing
>   parser_json: Fix limit object burst value parsing
>   parser_json: Fix synproxy object mss/wscale parsing
>   parser_json: Wrong check in json_parse_ct_timeout_policy()
>   parser_json: Catch nonsense ops in match statement
>   parser_json: Default meter size to zero

Series applied.