iptables: Mostly Parser Fixes

iptables: Mostly Parser Fixes

[Jan Engelhardt]

/*
 * Please review and if ok, merge.
 */

The following changes since commit 44191bdbd71e685fba9eab864b9df25e63905220:

  iptables-restore: move code to add_param_to_argv, cleanup (fix gcc-4.7) (2012-07-25 13:04:52 +0200)

are available in the git repository at:
  git://git.inai.de/iptables master

Jan Engelhardt (7):
      iptables-restore: warn about -t in rule lines
      doc: grammatical updates to libxt_SET
      libxt_u32: do bounds checking for @'s operands
      libxt_devgroup: consolidate devgroup specification parsing
      libxt_devgroup: guard against negative numbers
      libxt_LED: guard against negative numbers
      libxt_*limit: avoid division by zero

 extensions/libxt_LED.c       |    7 +++-
 extensions/libxt_SET.man     |   12 +++---
 extensions/libxt_devgroup.c  |   72 ++++++++++++++++++-----------------------
 extensions/libxt_hashlimit.c |   17 +++++++---
 extensions/libxt_limit.c     |   17 +++++++---
 extensions/libxt_u32.c       |   12 ++-----
 iptables/ip6tables-restore.c |    3 +-
 iptables/iptables-restore.c  |    3 +-
 8 files changed, 73 insertions(+), 70 deletions(-)

[PATCH 1/7] iptables-restore: warn about -t in rule lines

[Jan Engelhardt]

save-restore syntax uses *table, not -t table.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 iptables/ip6tables-restore.c |    3 +--
 iptables/iptables-restore.c  |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
index 3894d68..614ff0f 100644
--- a/iptables/ip6tables-restore.c
+++ b/iptables/ip6tables-restore.c
@@ -417,8 +417,7 @@ int ip6tables_restore_main(int argc, char *argv[])
 					if (!strncmp(param_buffer, "-t", 2)
                                             || !strncmp(param_buffer, "--table", 8)) {
 						xtables_error(PARAMETER_PROBLEM,
-						   "Line %u seems to have a "
-						   "-t table option.\n", line);
+						   "The -t option (seen in line %u) cannot be used in ip6tables-restore.\n", line);
 						exit(1);
 					}
 
diff --git a/iptables/iptables-restore.c b/iptables/iptables-restore.c
index 034f960..294e4b8 100644
--- a/iptables/iptables-restore.c
+++ b/iptables/iptables-restore.c
@@ -417,8 +417,7 @@ iptables_restore_main(int argc, char *argv[])
 					if (!strncmp(param_buffer, "-t", 2)
 					    || !strncmp(param_buffer, "--table", 8)) {
 						xtables_error(PARAMETER_PROBLEM,
-						   "Line %u seems to have a "
-						   "-t table option.\n", line);
+						   "The -t option (seen in line %u) cannot be used in iptables-restore.\n", line);
 						exit(1);
 					}
 
-- 
1.7.7

[PATCH 2/7] doc: grammatical updates to libxt_SET

[Jan Engelhardt]

Cherry-picked these from recent patches from Mr Dash Four.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/libxt_SET.man |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/extensions/libxt_SET.man b/extensions/libxt_SET.man
index 63eb383..c35ba93 100644
--- a/extensions/libxt_SET.man
+++ b/extensions/libxt_SET.man
@@ -1,24 +1,24 @@
-This modules adds and/or deletes entries from IP sets which can be defined 
+This module adds and/or deletes entries from IP sets which can be defined
 by ipset(8).
 .TP
 \fB\-\-add\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
-add the address(es)/port(s) of the packet to the sets
+add the address(es)/port(s) of the packet to the set
 .TP
 \fB\-\-del\-set\fP \fIsetname\fP \fIflag\fP[\fB,\fP\fIflag\fP...]
-delete the address(es)/port(s) of the packet from the sets
+delete the address(es)/port(s) of the packet from the set
 .IP
-where flags are
+where \fIflag\fP(s) are
 .BR "src"
 and/or
 .BR "dst"
 specifications and there can be no more than six of them.
 .TP
 \fB\-\-timeout\fP \fIvalue\fP
-when adding entry, the timeout value to use instead of the default
+when adding an entry, the timeout value to use instead of the default
 one from the set definition
 .TP
 \fB\-\-exist\fP
-when adding entry if it already exists, reset the timeout value
+when adding an entry if it already exists, reset the timeout value
 to the specified one or to the default from the set definition
 .PP
 Use of -j SET requires that ipset kernel support is provided, which, for
-- 
1.7.7

[PATCH 3/7] libxt_u32: do bounds checking for @'s operands

[Jan Engelhardt]

Using only strtoul is prone to accept all values, including negative
ones which are not explicitly allowed. Therefore, use xtables_strtoui
with bounds checking.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/libxt_u32.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/extensions/libxt_u32.c b/extensions/libxt_u32.c
index 6d024fb..2a7f5d8 100644
--- a/extensions/libxt_u32.c
+++ b/extensions/libxt_u32.c
@@ -88,17 +88,13 @@ static void u32_dump(const struct xt_u32 *data)
 /* string_to_number() is not quite what we need here ... */
 static uint32_t parse_number(const char **s, int pos)
 {
-	uint32_t number;
+	unsigned int number;
 	char *end;
 
-	errno  = 0;
-	number = strtoul(*s, &end, 0);
-	if (end == *s)
+	if (!xtables_strtoui(*s, &end, &number, 0, UINT32_MAX) ||
+	    end == *s)
 		xtables_error(PARAMETER_PROBLEM,
-			   "u32: at char %d: expected number", pos);
-	if (errno != 0)
-		xtables_error(PARAMETER_PROBLEM,
-			   "u32: at char %d: error reading number", pos);
+			"u32: at char %d: not a number or out of range", pos);
 	*s = end;
 	return number;
 }
-- 
1.7.7

[PATCH 4/7] libxt_devgroup: consolidate devgroup specification parsing

[Jan Engelhardt]

This is a small cleanup, reducing the two copies of X/Y parsing to
one.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/libxt_devgroup.c |   70 ++++++++++++++++++------------------------
 1 files changed, 30 insertions(+), 40 deletions(-)

diff --git a/extensions/libxt_devgroup.c b/extensions/libxt_devgroup.c
index 4487c83..69ae279 100644
--- a/extensions/libxt_devgroup.c
+++ b/extensions/libxt_devgroup.c
@@ -42,58 +42,48 @@ static void devgroup_init(struct xt_entry_match *match)
 		fprintf(stderr, "Warning: %s: %s\n", file, strerror(errno));
 }
 
+static void devgroup_parse_groupspec(const char *arg, unsigned int *group,
+				     unsigned int *mask)
+{
+	char *end;
+
+	*group = strtoul(arg, &end, 0);
+	if (end != arg && (*end == '/' || *end == '\0')) {
+		if (*end == '/')
+			*mask = strtoul(end + 1, &end, 0);
+		else
+			*mask = ~0U;
+		if (*end != '\0' || end == arg)
+			xtables_error(PARAMETER_PROBLEM,
+				      "Bad group value \"%s\"", arg);
+	} else {
+		*group = xtables_lmap_name2id(devgroups, arg);
+		if (*group == -1)
+			xtables_error(PARAMETER_PROBLEM,
+				      "Device group \"%s\" not found", arg);
+		*mask = ~0U;
+	}
+}
+
 static void devgroup_parse(struct xt_option_call *cb)
 {
 	struct xt_devgroup_info *info = cb->data;
-	unsigned int id;
-	char *end;
+	unsigned int id, mask;
 
 	xtables_option_parse(cb);
 	switch (cb->entry->id) {
 	case O_SRC_GROUP:
-		info->src_group = strtoul(cb->arg, &end, 0);
-		if (end != cb->arg && (*end == '/' || *end == '\0')) {
-			if (*end == '/')
-				info->src_mask = strtoul(end+1, &end, 0);
-			else
-				info->src_mask = 0xffffffff;
-			if (*end != '\0' || end == cb->arg)
-				xtables_error(PARAMETER_PROBLEM,
-					      "Bad src-group value `%s'",
-					      cb->arg);
-		} else {
-			id = xtables_lmap_name2id(devgroups, cb->arg);
-			if (id == -1)
-				xtables_error(PARAMETER_PROBLEM,
-					      "Device group `%s' not found",
-					      cb->arg);
-			info->src_group = id;
-			info->src_mask  = 0xffffffff;
-		}
+		devgroup_parse_groupspec(cb->arg, &id, &mask);
+		info->src_group = id;
+		info->src_mask  = mask;
 		info->flags |= XT_DEVGROUP_MATCH_SRC;
 		if (cb->invert)
 			info->flags |= XT_DEVGROUP_INVERT_SRC;
 		break;
 	case O_DST_GROUP:
-		info->dst_group = strtoul(cb->arg, &end, 0);
-		if (end != cb->arg && (*end == '/' || *end == '\0')) {
-			if (*end == '/')
-				info->dst_mask = strtoul(end+1, &end, 0);
-			else
-				info->dst_mask = 0xffffffff;
-			if (*end != '\0' || end == cb->arg)
-				xtables_error(PARAMETER_PROBLEM,
-					      "Bad dst-group value `%s'",
-					      cb->arg);
-		} else {
-			id = xtables_lmap_name2id(devgroups, cb->arg);
-			if (id == -1)
-				xtables_error(PARAMETER_PROBLEM,
-					      "Device group `%s' not found",
-					      cb->arg);
-			info->dst_group = id;
-			info->dst_mask  = 0xffffffff;
-		}
+		devgroup_parse_groupspec(cb->arg, &id, &mask);
+		info->dst_group = id;
+		info->dst_mask  = mask;
 		info->flags |= XT_DEVGROUP_MATCH_DST;
 		if (cb->invert)
 			info->flags |= XT_DEVGROUP_INVERT_DST;
-- 
1.7.7

[PATCH 5/7] libxt_devgroup: guard against negative numbers

[Jan Engelhardt]

More corrections of the strtoul kind.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/libxt_devgroup.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/extensions/libxt_devgroup.c b/extensions/libxt_devgroup.c
index 69ae279..4a69c82 100644
--- a/extensions/libxt_devgroup.c
+++ b/extensions/libxt_devgroup.c
@@ -46,14 +46,16 @@ static void devgroup_parse_groupspec(const char *arg, unsigned int *group,
 				     unsigned int *mask)
 {
 	char *end;
+	bool ok;
 
-	*group = strtoul(arg, &end, 0);
-	if (end != arg && (*end == '/' || *end == '\0')) {
+	ok = xtables_strtoui(arg, &end, group, 0, UINT32_MAX);
+	if (ok && (*end == '/' || *end == '\0')) {
 		if (*end == '/')
-			*mask = strtoul(end + 1, &end, 0);
+			ok = xtables_strtoui(end + 1, NULL, mask,
+			                     0, UINT32_MAX);
 		else
 			*mask = ~0U;
-		if (*end != '\0' || end == arg)
+		if (!ok)
 			xtables_error(PARAMETER_PROBLEM,
 				      "Bad group value \"%s\"", arg);
 	} else {
-- 
1.7.7

[PATCH 6/7] libxt_LED: guard against negative numbers

[Jan Engelhardt]

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/libxt_LED.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/extensions/libxt_LED.c b/extensions/libxt_LED.c
index 9d68fa2..e6cf849 100644
--- a/extensions/libxt_LED.c
+++ b/extensions/libxt_LED.c
@@ -49,6 +49,7 @@ static void LED_help(void)
 static void LED_parse(struct xt_option_call *cb)
 {
 	struct xt_led_info *led = cb->data;
+	unsigned int delay;
 
 	xtables_option_parse(cb);
 	switch (cb->entry->id) {
@@ -59,8 +60,10 @@ static void LED_parse(struct xt_option_call *cb)
 	case O_LED_DELAY:
 		if (strncasecmp(cb->arg, "inf", 3) == 0)
 			led->delay = -1;
-		else
-			led->delay = strtoul(cb->arg, NULL, 0);
+		else if (!xtables_strtoui(cb->arg, NULL, &delay, 0, UINT32_MAX))
+			xtables_error(PARAMETER_PROBLEM,
+				"Delay value must be within range 0..%u",
+				UINT32_MAX);
 		break;
 	case O_LED_ALWAYS_BLINK:
 		led->always_blink = 1;
-- 
1.7.7

[PATCH 7/7] libxt_*limit: avoid division by zero

[Jan Engelhardt]

It was possible to specify -A mychain -m hashlimit --hashlimit
600059/minute; this would convert to r->avg=0, which subsequently
causes a division by zero when printing with -S mychain.

1. Avoid division by zero in print_rate by printing infinity
   instead.
2. Rewrite the test in parse_rate to properly reject too high rates.

Signed-off-by: Jan Engelhardt <jengelh@inai.de>
---
 extensions/libxt_hashlimit.c |   17 ++++++++++++-----
 extensions/libxt_limit.c     |   17 ++++++++++++-----
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/extensions/libxt_hashlimit.c b/extensions/libxt_hashlimit.c
index 37a3148..831345b 100644
--- a/extensions/libxt_hashlimit.c
+++ b/extensions/libxt_hashlimit.c
@@ -10,6 +10,7 @@
  * 
  * Error corections by nmalykh@bilim.com (22.01.2005)
  */
+#include <math.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -250,12 +251,13 @@ int parse_rate(const char *rate, uint32_t *val, struct hashlimit_mt_udata *ud)
 	if (!r)
 		return 0;
 
-	/* This would get mapped to infinite (1/day is minimum they
-           can specify, so we're ok at that end). */
-	if (r / ud->mult > XT_HASHLIMIT_SCALE)
-		xtables_error(PARAMETER_PROBLEM, "Rate too fast \"%s\"\n", rate);
-
 	*val = XT_HASHLIMIT_SCALE * ud->mult / r;
+	if (*val == 0)
+		/*
+		 * The rate maps to infinity. (1/day is the minimum they can
+		 * specify, so we are ok at that end).
+		 */
+		xtables_error(PARAMETER_PROBLEM, "Rate too fast \"%s\"\n", rate);
 	return 1;
 }
 
@@ -434,6 +436,11 @@ static uint32_t print_rate(uint32_t period)
 {
 	unsigned int i;
 
+	if (period == 0) {
+		printf(" %f", INFINITY);
+		return 0;
+	}
+
 	for (i = 1; i < ARRAY_SIZE(rates); ++i)
 		if (period > rates[i].mult
             || rates[i].mult/period < rates[i].mult%period)
diff --git a/extensions/libxt_limit.c b/extensions/libxt_limit.c
index b15b02f..023500c 100644
--- a/extensions/libxt_limit.c
+++ b/extensions/libxt_limit.c
@@ -3,6 +3,7 @@
  * Jérôme de Vivie   <devivie@info.enserb.u-bordeaux.fr>
  * Hervé Eychenne    <rv@wallfire.org>
  */
+#include <math.h>
 #include <stdio.h>
 #include <string.h>
 #include <stdlib.h>
@@ -64,12 +65,13 @@ int parse_rate(const char *rate, uint32_t *val)
 	if (!r)
 		return 0;
 
-	/* This would get mapped to infinite (1/day is minimum they
-           can specify, so we're ok at that end). */
-	if (r / mult > XT_LIMIT_SCALE)
-		xtables_error(PARAMETER_PROBLEM, "Rate too fast \"%s\"\n", rate);
-
 	*val = XT_LIMIT_SCALE * mult / r;
+	if (*val == 0)
+		/*
+		 * The rate maps to infinity. (1/day is the minimum they can
+		 * specify, so we are ok at that end).
+		 */
+		xtables_error(PARAMETER_PROBLEM, "Rate too fast \"%s\"\n", rate);
 	return 1;
 }
 
@@ -118,6 +120,11 @@ static void print_rate(uint32_t period)
 {
 	unsigned int i;
 
+	if (period == 0) {
+		printf(" %f", INFINITY);
+		return;
+	}
+
 	for (i = 1; i < ARRAY_SIZE(rates); ++i)
 		if (period > rates[i].mult
             || rates[i].mult/period < rates[i].mult%period)
-- 
1.7.7

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] iptables-restore: warn about -t in rule lines

[Pablo Neira Ayuso]

On Sat, Jul 28, 2012 at 07:21:03PM +0200, Jan Engelhardt wrote:
> save-restore syntax uses *table, not -t table.
> 
> Signed-off-by: Jan Engelhardt <jengelh@inai.de>
> ---
>  iptables/ip6tables-restore.c |    3 +--
>  iptables/iptables-restore.c  |    3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/iptables/ip6tables-restore.c b/iptables/ip6tables-restore.c
> index 3894d68..614ff0f 100644
> --- a/iptables/ip6tables-restore.c
> +++ b/iptables/ip6tables-restore.c
> @@ -417,8 +417,7 @@ int ip6tables_restore_main(int argc, char *argv[])
>  					if (!strncmp(param_buffer, "-t", 2)
>                                              || !strncmp(param_buffer, "--table", 8)) {
>  						xtables_error(PARAMETER_PROBLEM,
> -						   "Line %u seems to have a "
> -						   "-t table option.\n", line);
> +						   "The -t option (seen in line %u) cannot be used in ip6tables-restore.\n", line);

Lines should break at 80 chars.

I know we sometimes slightly violate this limit, but I don't want to
have wrapping code in my 80-chars terminals.

No need to resend, I did it myself.

Re: iptables: Mostly Parser Fixes

[Pablo Neira Ayuso]

On Sat, Jul 28, 2012 at 07:21:02PM +0200, Jan Engelhardt wrote:
> 
> /*
>  * Please review and if ok, merge.
>  */
> 
> The following changes since commit 44191bdbd71e685fba9eab864b9df25e63905220:
> 
>   iptables-restore: move code to add_param_to_argv, cleanup (fix gcc-4.7) (2012-07-25 13:04:52 +0200)
> 
> are available in the git repository at:
>   git://git.inai.de/iptables master
> 
> Jan Engelhardt (7):
>       iptables-restore: warn about -t in rule lines
>       doc: grammatical updates to libxt_SET
>       libxt_u32: do bounds checking for @'s operands
>       libxt_devgroup: consolidate devgroup specification parsing
>       libxt_devgroup: guard against negative numbers
>       libxt_LED: guard against negative numbers
>       libxt_*limit: avoid division by zero

I have cherry-picked from 2 to 7. Patch 1 was modified as I indicated
in my previous email. This is now in master.

Thanks a lot Jan.

Re: [PATCH 1/7] iptables-restore: warn about -t in rule lines

[Jan Engelhardt]

On Tuesday 2012-07-31 13:39, Pablo Neira Ayuso wrote:
>> @@ -417,8 +417,7 @@ int ip6tables_restore_main(int argc, char *argv[])
>>  					if (!strncmp(param_buffer, "-t", 2)
>>                                              || !strncmp(param_buffer, "--table", 8)) {
>>  						xtables_error(PARAMETER_PROBLEM,
>> -						   "Line %u seems to have a "
>> -						   "-t table option.\n", line);
>> +						   "The -t option (seen in line %u) cannot be used in ip6tables-restore.\n", line);
>
>Lines should break at 80 chars.
>
>I know we sometimes slightly violate this limit, but I don't want to
>have wrapping code in my 80-chars terminals.

I know; however, if there already are 5 levels *and* you have a text
string (people grep for that!), it is in principle nicer to have it
non-wrapped.