Re: [PATCH v2 1/1] iptables: Fix crash on malformed iptables-restore

[Florian Westphal <fw@strlen.de>]

Oliver Ford <ojford@gmail.com> wrote:
> --- a/iptables/ip6tables-restore.c
> +++ b/iptables/ip6tables-restore.c
> @@ -165,14 +165,33 @@ static void add_param_to_argv(char *parsestart)
>  			param_buffer[param_len] = '\0';
>  
>  			/* check if table name specified */
> -			if (!strncmp(param_buffer, "-t", 2)
> -                            || !strncmp(param_buffer, "--table", 8)) {
> +			if (param_buffer[0] == '-' && param_buffer[1] != '-'
> +				&& strchr(param_buffer, 't')) {
>  				xtables_error(PARAMETER_PROBLEM,
> -				"The -t option (seen in line %u) cannot be "
> -				"used in ip6tables-restore.\n", line);
> +					"The -t option (seen in line %u) cannot be "
> +					"used in ip6tables-restore.\n", line);
> +				exit(1);
> +			} else if (!strncmp(param_buffer, "--", 2)
> +				&& strchr(param_buffer, 't')) {

Why this strchr() ?

if (!strncmp(param_buffer, "--t", 3) &&
    !strncmp(param_buffer, "--table", strlen(param_buffer))
       err();

should work.

> +				/* If we begin with a '--' and have a 't', check
> +				 * that the parameter is in the list of valid options */
> +				const char* t_options[] = {

If this is needed, I'd suggest

static const char * const t_options[] = {

> +					"delete", "insert", "list", "list-rules", "delete-chain",
> +					"destination", "dst", "protocol", "in-interface", "match",
> +					"out-interface", "wait", "wait-interval", "exact",
> +					"fragments", "set-counters", "goto"};
> +				int i, opt_len = ARRAY_SIZE(t_options);
> +				for (i = 0; i < opt_len; i++) {
> +					if (!strcmp(param_buffer + 2, t_options[i])) {
> +						goto t_passed;
> +					}
> +				}

If this t_options[] thing is really needed i'd try to stick this into
a helper function so we don't have to duplicate this in all 3
incarnations.

Thanks for working on this.