[PATCH conntrack 1/3] conntrack: improve --secmark,--id,--zone parser

[PATCH conntrack 1/3] conntrack: improve --secmark,--id,--zone parser

[Pablo Neira Ayuso]

strtoul() is called with no error checking at all, add a helper
function to validate input is correct.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/conntrack.c | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index 9fa49869b553..f3725eefd5de 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -1213,6 +1213,25 @@ parse_parameter_mask(const char *arg, unsigned int *status, unsigned int *mask,
 		exit_error(PARAMETER_PROBLEM, "Bad parameter `%s'", arg);
 }
 
+static int parse_value(const char *str, uint32_t *ret, uint64_t max)
+{
+	char *endptr;
+	uint64_t val;
+
+	errno = 0;
+	val = strtoul(str, &endptr, 0);
+	if (endptr == str ||
+	    *endptr != '\0' ||
+	    (val == ULONG_MAX && errno == ERANGE) ||
+	    (val == 0 && errno == ERANGE) ||
+	    val > max)
+		return -1;
+
+	*ret = val;
+
+	return 0;
+}
+
 static void
 parse_u32_mask(const char *arg, struct u32_mask *m)
 {
@@ -2918,6 +2937,7 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
 	struct ct_tmpl *tmpl;
 	int res = 0, partial;
 	union ct_address ad;
+	uint32_t value;
 	int c, cmd;
 
 	/* we release these objects in the exit_error() path. */
@@ -3078,17 +3098,19 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
 		case 'w':
 		case '(':
 		case ')':
+			if (parse_value(optarg, &value, UINT16_MAX) < 0)
+				exit_error(OTHER_PROBLEM, "unexpected value '%s' with -%c option", optarg, c);
+
 			options |= opt2type[c];
-			nfct_set_attr_u16(tmpl->ct,
-					  opt2attr[c],
-					  strtoul(optarg, NULL, 0));
+			nfct_set_attr_u16(tmpl->ct, opt2attr[c], value);
 			break;
 		case 'i':
 		case 'c':
+			if (parse_value(optarg, &value, UINT32_MAX) < 0)
+				exit_error(OTHER_PROBLEM, "unexpected value '%s' with -%c option", optarg, c);
+
 			options |= opt2type[c];
-			nfct_set_attr_u32(tmpl->ct,
-					  opt2attr[c],
-					  strtoul(optarg, NULL, 0));
+			nfct_set_attr_u32(tmpl->ct, opt2attr[c], value);
 			break;
 		case 'm':
 			options |= opt2type[c];
-- 
2.30.2

[PATCH conntrack 2/3] conntrack: improve --mark parser

[Pablo Neira Ayuso]

Enhance helper function to parse mark and mask (if available), bail out
if input is not correct.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 src/conntrack.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/src/conntrack.c b/src/conntrack.c
index f3725eefd5de..1da98697a264 100644
--- a/src/conntrack.c
+++ b/src/conntrack.c
@@ -1232,17 +1232,37 @@ static int parse_value(const char *str, uint32_t *ret, uint64_t max)
 	return 0;
 }
 
-static void
+static int
 parse_u32_mask(const char *arg, struct u32_mask *m)
 {
-	char *end;
+	uint64_t val, mask;
+	char *endptr;
+
+	val = strtoul(arg, &endptr, 0);
+	if (endptr == arg ||
+	    (*endptr != '\0' && *endptr != '/') ||
+	    (val == ULONG_MAX && errno == ERANGE) ||
+	    (val == 0 && errno == ERANGE) ||
+	    val > UINT32_MAX)
+		return -1;
 
-	m->value = (uint32_t) strtoul(arg, &end, 0);
+	m->value = val;
 
-	if (*end == '/')
-		m->mask = (uint32_t) strtoul(end+1, NULL, 0);
-	else
+	if (*endptr == '/') {
+		mask = (uint32_t) strtoul(endptr + 1, &endptr, 0);
+		if (endptr == arg ||
+		    *endptr != '\0' ||
+		    (val == ULONG_MAX && errno == ERANGE) ||
+		    (val == 0 && errno == ERANGE) ||
+		    val > UINT32_MAX)
+			return -1;
+
+		m->mask = mask;
+	} else {
 		m->mask = ~0;
+	}
+
+	return 0;
 }
 
 static int
@@ -3114,7 +3134,9 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
 			break;
 		case 'm':
 			options |= opt2type[c];
-			parse_u32_mask(optarg, &tmpl->mark);
+			if (parse_u32_mask(optarg, &tmpl->mark) < 0)
+				exit_error(OTHER_PROBLEM, "unexpected value '%s' with -%c option", optarg, c);
+
 			tmpl->filter_mark_kernel.val = tmpl->mark.value;
 			tmpl->filter_mark_kernel.mask = tmpl->mark.mask;
 			tmpl->filter_mark_kernel_set = true;
-- 
2.30.2

[PATCH conntrack 3/3] tests: conntrack: missing space before option

[Pablo Neira Ayuso]

Recent updates make the conntrack parser slightly more robust. A few
test lines include:

... -w 11-s 2001:DB8::1.1.1.1 ...

where space is missing. These are typos rather than valid input.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 tests/conntrack/testsuite/09dumpopt | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/conntrack/testsuite/09dumpopt b/tests/conntrack/testsuite/09dumpopt
index c1e0e6ed376d..9dcd51f81638 100644
--- a/tests/conntrack/testsuite/09dumpopt
+++ b/tests/conntrack/testsuite/09dumpopt
@@ -25,7 +25,7 @@
 # delete reverse
 -D -w 11 -r 2.2.2.2 -q 1.1.1.1 -p tcp --reply-port-src 11 --reply-port-dst 21 ; OK
 # delete v6 conntrack
--D -w 11-s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; OK
+-D -w 11 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; OK
 # delete icmp ping request entry
 -D -w 11 -u SEEN_REPLY -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p icmp --icmp-type 8 --icmp-code 0 --icmp-id 1226 ; OK
 # delete old entries
@@ -33,7 +33,7 @@
 # delete reverse
 -D -w 10 -r 2.2.2.2 -q 1.1.1.1 -p tcp --reply-port-src 11 --reply-port-dst 21 ; OK
 # delete v6 conntrack
--D -w 10-s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; OK
+-D -w 10 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; OK
 # delete icmp ping request entry
 -D -w 10 -u SEEN_REPLY -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p icmp --icmp-type 8 --icmp-code 0 --icmp-id 1226 ; OK
 #
@@ -64,7 +64,7 @@
 # delete reverse
 -D -w 11 -r 2.2.2.2 -q 1.1.1.1 -p tcp --reply-port-src 11 --reply-port-dst 21 ; OK
 # delete v6 conntrack
--D -w 11-s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; OK
+-D -w 11 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; OK
 # delete icmp ping request entry
 -D -w 11 -u SEEN_REPLY -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p icmp --icmp-type 8 --icmp-code 0 --icmp-id 1226 ; OK
 # delete old entries
@@ -72,7 +72,7 @@
 # delete reverse
 -D -w 10 -r 2.2.2.2 -q 1.1.1.1 -p tcp --reply-port-src 11 --reply-port-dst 21 ; BAD
 # delete v6 conntrack
--D -w 10-s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; BAD
+-D -w 10 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p tcp --sport 10 --dport 20 ; BAD
 # delete icmp ping request entry
 -D -w 10 -u SEEN_REPLY -s 1.1.1.1 -d 2.2.2.2 -r 2.2.2.2 -q 1.1.1.1 -p icmp --icmp-type 8 --icmp-code 0 --icmp-id 1226 ; BAD
 #
@@ -161,13 +161,13 @@
 # IGMP
 -D -w 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
 # Some fency protocol
--D -w 10  -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
+-D -w 10 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
 # Some fency protocol with IPv6
 -D -w 10 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p 200 ; OK
 # Delete stuff in zone 11, should succeed
 # IGMP
 -D -w 11 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 2 ; OK
 # Some fency protocol
--D -w 11  -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
+-D -w 11 -s 0.0.0.0 -d 224.0.0.22 -r 224.0.0.22 -q 0.0.0.0 -p 200 ; OK
 # Some fency protocol with IPv6
 -D -w 11 -s 2001:DB8::1.1.1.1 -d 2001:DB8::2.2.2.2 -p 200 ; OK
-- 
2.30.2

Re: [PATCH conntrack 1/3] conntrack: improve --secmark,--id,--zone parser

[Jeremy Sowden]

[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]

On 2024-10-12, at 17:29:55 +0200, Pablo Neira Ayuso wrote:
> strtoul() is called with no error checking at all, add a helper
> function to validate input is correct.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  src/conntrack.c | 34 ++++++++++++++++++++++++++++------
>  1 file changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/src/conntrack.c b/src/conntrack.c
> index 9fa49869b553..f3725eefd5de 100644
> --- a/src/conntrack.c
> +++ b/src/conntrack.c
> @@ -1213,6 +1213,25 @@ parse_parameter_mask(const char *arg, unsigned int *status, unsigned int *mask,
>  		exit_error(PARAMETER_PROBLEM, "Bad parameter `%s'", arg);
>  }
>  
> +static int parse_value(const char *str, uint32_t *ret, uint64_t max)
> +{
> +	char *endptr;
> +	uint64_t val;
> +
> +	errno = 0;
> +	val = strtoul(str, &endptr, 0);
> +	if (endptr == str ||
> +	    *endptr != '\0' ||
> +	    (val == ULONG_MAX && errno == ERANGE) ||
> +	    (val == 0 && errno == ERANGE) ||
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

TTBOMK, this won't happen.

> +	    val > max)

Given that `*ret` is a `uint32_t`, would it make sense also to check
that `max <= UINT32_MAX`?

> +		return -1;
> +
> +	*ret = val;

> +
> +	return 0;
> +}
> +
>  static void
>  parse_u32_mask(const char *arg, struct u32_mask *m)
>  {
> @@ -2918,6 +2937,7 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  	struct ct_tmpl *tmpl;
>  	int res = 0, partial;
>  	union ct_address ad;
> +	uint32_t value;
>  	int c, cmd;
>  
>  	/* we release these objects in the exit_error() path. */
> @@ -3078,17 +3098,19 @@ static void do_parse(struct ct_cmd *ct_cmd, int argc, char *argv[])
>  		case 'w':
>  		case '(':
>  		case ')':
> +			if (parse_value(optarg, &value, UINT16_MAX) < 0)
> +				exit_error(OTHER_PROBLEM, "unexpected value '%s' with -%c option", optarg, c);
> +
>  			options |= opt2type[c];
> -			nfct_set_attr_u16(tmpl->ct,
> -					  opt2attr[c],
> -					  strtoul(optarg, NULL, 0));
> +			nfct_set_attr_u16(tmpl->ct, opt2attr[c], value);
>  			break;
>  		case 'i':
>  		case 'c':
> +			if (parse_value(optarg, &value, UINT32_MAX) < 0)
> +				exit_error(OTHER_PROBLEM, "unexpected value '%s' with -%c option", optarg, c);
> +
>  			options |= opt2type[c];
> -			nfct_set_attr_u32(tmpl->ct,
> -					  opt2attr[c],
> -					  strtoul(optarg, NULL, 0));
> +			nfct_set_attr_u32(tmpl->ct, opt2attr[c], value);
>  			break;
>  		case 'm':
>  			options |= opt2type[c];
> -- 
> 2.30.2
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]