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

[Jeremy Sowden <jeremy@azazel.net>]

[-- 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 --]