[PATCH 2.4/2.6]: TTL target

[PATCH 2.4/2.6]: TTL target

[Nicolas Bouliane]

Hi guys,

We attached three patches that fixe some problems with the TTL target.

1) When using a value > 255 or < 0, there's an overflow with
   u_int8_t value;

   This means that giving 256 as a value will create a rule with
   a value of 0.

   The first patch adds some error controls in TTL library avoiding this.

2) There is a little, but significative, bug in the TTL target module.
   Instead of decreasing the TTL, this is increasing it.

   The second patch is for 2.4

3) Same as second patch, but for 2.6

Let us know if there is some problems. Thanks you.

Patches below.

Signed-off-by: Nicolas Bouliane <nib@cookinglinux.org>


diff -urpN extensions/libipt_TTL.c.orig extensions/libipt_TTL.c
--- extensions/libipt_TTL.c.orig	2004-07-24 21:17:24.000000000 -0400
+++ extensions/libipt_TTL.c	2004-07-25 18:00:39.000000000 -0400
@@ -24,9 +24,9 @@ static void help(void)
 {
 	printf(
 "TTL target v%s options\n"
-"  --ttl-set value		Set TTL to <value>\n"
-"  --ttl-dec value		Decrement TTL by <value>\n"
-"  --ttl-inc value		Increment TTL by <value>\n"
+"  --ttl-set value		Set TTL to <value 0-255>\n"
+"  --ttl-dec value		Decrement TTL by <value 1-255>\n"
+"  --ttl-inc value		Increment TTL by <value 1-255>\n"
 , IPTABLES_VERSION);
 }

@@ -35,7 +35,8 @@ static int parse(int c, char **argv, int
 		struct ipt_entry_target **target)
 {
 	struct ipt_TTL_info *info = (struct ipt_TTL_info *) (*target)->data;
-	u_int8_t value;
+   u_int16_t value;
+   u_int8_t alpha;

 	if (*flags & IPT_TTL_USED) {
 		exit_error(PARAMETER_PROBLEM,
@@ -49,28 +50,36 @@ static int parse(int c, char **argv, int
 	if (check_inverse(optarg, &invert, NULL, 0))
 		exit_error(PARAMETER_PROBLEM,
 				"TTL: unexpected `!'");
-
-	value = atoi(optarg);
-
+
+   if ((strlen(optarg) > 3) ||
+      (sscanf(optarg, "%hd%c", &value, &alpha) != 1))
+      exit_error(PARAMETER_PROBLEM,
+            "TTL: Invalid value");
+
 	switch (c) {

 		case '1':
+         if (value > 255) {
+            exit_error(PARAMETER_PROBLEM,
+               "TTL: Invalid value");
+         }
+
 			info->mode = IPT_TTL_SET;
 			break;

 		case '2':
-			if (value == 0) {
-				exit_error(PARAMETER_PROBLEM,
-					"TTL: decreasing by 0?");
+         if (value > 255 || value < 1) {
+   			exit_error(PARAMETER_PROBLEM,
+					"TTL: Invalid value");
 			}

 			info->mode = IPT_TTL_DEC;
 			break;

 		case '3':
-			if (value == 0) {
+         if (value > 255 || value < 1) {
 				exit_error(PARAMETER_PROBLEM,
-					"TTL: increasing by 0?");
+   				"TTL: Invalid value");
 			}

 			info->mode = IPT_TTL_INC;



diff -urpN linux/net/ipv4/netfilter/ipt_TTL.c.orig
linux/net/ipv4/netfilter/ipt_TTL.c
--- linux/net/ipv4/netfilter/ipt_TTL.c.orig	2004-07-24 21:09:23.000000000
-0400
+++ linux/net/ipv4/netfilter/ipt_TTL.c	2004-07-25 18:15:08.000000000 -0400
@@ -37,7 +37,7 @@ static unsigned int ipt_ttl_target(struc
 				new_ttl = 255;
 			break;
 		case IPT_TTL_DEC:
-			new_ttl = iph->ttl + info->ttl;
+			new_ttl = iph->ttl - info->ttl;
 			if (new_ttl < 0)
 				new_ttl = 0;
 			break;
@@ -85,11 +85,6 @@ static int ipt_ttl_checkentry(const char
 		return 0;
 	}

-	if ((info->mode != IPT_TTL_SET) && (info->ttl == 0)) {
-		printk(KERN_WARNING "TTL: increment/decrement doesn't make sense with
value 0\n");
-		return 0;
-	}
-
 	return 1;
 }



diff -urpN linux-2.6/net/ipv4/netfilter/ipt_TTL.c.orig
linux-2.6/net/ipv4/netfilter/ipt_TTL.c
--- linux-2.6/net/ipv4/netfilter/ipt_TTL.c.orig	2004-07-25
18:30:56.000000000 -0400
+++ linux-2.6/net/ipv4/netfilter/ipt_TTL.c	2004-07-25 18:31:52.000000000
-0400
@@ -43,7 +43,7 @@ ipt_ttl_target(struct sk_buff **pskb, co
 				new_ttl = 255;
 			break;
 		case IPT_TTL_DEC:
-			new_ttl = iph->ttl + info->ttl;
+			new_ttl = iph->ttl - info->ttl;
 			if (new_ttl < 0)
 				new_ttl = 0;
 			break;
@@ -91,11 +91,6 @@ static int ipt_ttl_checkentry(const char
 		return 0;
 	}

-	if ((info->mode != IPT_TTL_SET) && (info->ttl == 0)) {
-		printk(KERN_WARNING "TTL: increment/decrement doesn't make sense with
value 0\n");
-		return 0;
-	}
-
 	return 1;
 }

--
Nicolas Bouliane,
Samuel Jean
at cookinglinux.org

Re: [PATCH 2.4/2.6]: TTL target

[Patrick McHardy]

Hi Nicolas,

Nicolas Bouliane wrote:
> Hi guys,
> 
> We attached three patches that fixe some problems with the TTL target.
> 
> 1) When using a value > 255 or < 0, there's an overflow with
>    u_int8_t value;
> 
>    This means that giving 256 as a value will create a rule with
>    a value of 0.
> 
>    The first patch adds some error controls in TTL library avoiding this.
> 
> 2) There is a little, but significative, bug in the TTL target module.
>    Instead of decreasing the TTL, this is increasing it.
> 
>    The second patch is for 2.4
> 
> 3) Same as second patch, but for 2.6
> 
> Let us know if there is some problems. Thanks you.

The kernel-part is fine, the userspace has some minor problems,
see my comments below. Could you please send a new version ?

Thanks,
Patrick

> 
> Patches below.
> 
> Signed-off-by: Nicolas Bouliane <nib@cookinglinux.org>
> 
> 
> diff -urpN extensions/libipt_TTL.c.orig extensions/libipt_TTL.c
> --- extensions/libipt_TTL.c.orig	2004-07-24 21:17:24.000000000 -0400
> +++ extensions/libipt_TTL.c	2004-07-25 18:00:39.000000000 -0400
> @@ -24,9 +24,9 @@ static void help(void)
>  {
>  	printf(
>  "TTL target v%s options\n"
> -"  --ttl-set value		Set TTL to <value>\n"
> -"  --ttl-dec value		Decrement TTL by <value>\n"
> -"  --ttl-inc value		Increment TTL by <value>\n"
> +"  --ttl-set value		Set TTL to <value 0-255>\n"
> +"  --ttl-dec value		Decrement TTL by <value 1-255>\n"
> +"  --ttl-inc value		Increment TTL by <value 1-255>\n"
>  , IPTABLES_VERSION);
>  }
> 
> @@ -35,7 +35,8 @@ static int parse(int c, char **argv, int
>  		struct ipt_entry_target **target)
>  {
>  	struct ipt_TTL_info *info = (struct ipt_TTL_info *) (*target)->data;
> -	u_int8_t value;
> +   u_int16_t value;
> +   u_int8_t alpha;

^^^ Indentation is broken, we use tabs not spaces

> 
>  	if (*flags & IPT_TTL_USED) {
>  		exit_error(PARAMETER_PROBLEM,
> @@ -49,28 +50,36 @@ static int parse(int c, char **argv, int
>  	if (check_inverse(optarg, &invert, NULL, 0))
>  		exit_error(PARAMETER_PROBLEM,
>  				"TTL: unexpected `!'");
> -
> -	value = atoi(optarg);
> -
> +
> +   if ((strlen(optarg) > 3) ||
> +      (sscanf(optarg, "%hd%c", &value, &alpha) != 1))

^^^ Why scan for characters and check for exactly one parsed item ?
     Use string_to_number with 0-255 as limit.

> +      exit_error(PARAMETER_PROBLEM,
> +            "TTL: Invalid value");
> +
>  	switch (c) {
> 
>  		case '1':
> +         if (value > 255) {
> +            exit_error(PARAMETER_PROBLEM,
> +               "TTL: Invalid value");
> +         }
> +
>  			info->mode = IPT_TTL_SET;
>  			break;
> 
>  		case '2':
> -			if (value == 0) {
> -				exit_error(PARAMETER_PROBLEM,
> -					"TTL: decreasing by 0?");
> +         if (value > 255 || value < 1) {
> +   			exit_error(PARAMETER_PROBLEM,
> +					"TTL: Invalid value");
^^^ The old message contained more information

>  			}
> 
>  			info->mode = IPT_TTL_DEC;
>  			break;
> 
>  		case '3':
> -			if (value == 0) {
> +         if (value > 255 || value < 1) {
>  				exit_error(PARAMETER_PROBLEM,
> -					"TTL: increasing by 0?");
> +   				"TTL: Invalid value");
>  			}
> 
>  			info->mode = IPT_TTL_INC;
> 
> 
> 

Re: [PATCH 2.4/2.6]: TTL target

[Nicolas Bouliane]

Hi Patrick,

Patrick McHardy wrote:
>
> The kernel-part is fine, the userspace has some minor problems,
> see my comments below. Could you please send a new version ?
>

Sure, thanks for having taken a look.
Your comments was appreciated and instructive.

New patch below.


Signed-off-by: Nicolas Bouliane <nib@cookinglinux.org>


diff -urpN extensions/libipt_TTL.c.orig extensions/libipt_TTL.c
--- extensions/libipt_TTL.c.orig	2004-07-25 21:03:19.000000000 -0400
+++ extensions/libipt_TTL.c	2004-07-25 21:10:45.000000000 -0400
@@ -24,9 +24,9 @@ static void help(void)
 {
 	printf(
 "TTL target v%s options\n"
-"  --ttl-set value		Set TTL to <value>\n"
-"  --ttl-dec value		Decrement TTL by <value>\n"
-"  --ttl-inc value		Increment TTL by <value>\n"
+"  --ttl-set value		Set TTL to <value 0-255>\n"
+"  --ttl-dec value		Decrement TTL by <value 1-255>\n"
+"  --ttl-inc value		Increment TTL by <value 1-255>\n"
 , IPTABLES_VERSION);
 }

@@ -35,7 +35,7 @@ static int parse(int c, char **argv, int
 		struct ipt_entry_target **target)
 {
 	struct ipt_TTL_info *info = (struct ipt_TTL_info *) (*target)->data;
-	u_int8_t value;
+	u_int32_t value;

 	if (*flags & IPT_TTL_USED) {
 		exit_error(PARAMETER_PROBLEM,
@@ -50,7 +50,9 @@ static int parse(int c, char **argv, int
 		exit_error(PARAMETER_PROBLEM,
 				"TTL: unexpected `!'");

-	value = atoi(optarg);
+	if (string_to_number(optarg, 0, 255, &value) == -1)
+      exit_error(PARAMETER_PROBLEM,
+            "TTL: Expected value between 0 and 255");

 	switch (c) {

--
Nicolas Bouliane,
Samuel Jean
at cookinglinux.org

Re: [PATCH 2.4/2.6]: TTL target

[Patrick McHardy]

Nicolas Bouliane wrote:
> New patch below.

Thanks. There are still some spaces used for indentation, but I'll
fix it up by hand.

Regards
Patrick


> 
> 
> +      exit_error(PARAMETER_PROBLEM,
> +            "TTL: Expected value between 0 and 255");
> 
>  	switch (c) {
> 
> --
> Nicolas Bouliane,
> Samuel Jean
> at cookinglinux.org
> 

Re: [PATCH 2.4/2.6]: TTL target

[Patrick McHardy]

Nicolas Bouliane wrote:
> 2) There is a little, but significative, bug in the TTL target module.
>    Instead of decreasing the TTL, this is increasing it.
> 
>    The second patch is for 2.4
> 
> 3) Same as second patch, but for 2.6
> 
> Let us know if there is some problems. Thanks you.

The patches for 2.4 and 2.6 don't apply, your mailer may have mangled
them. Please send them again as attachment.

Regards
Patrick

Re: [PATCH 2.4/2.6]: TTL target

[Samuel Jean]

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

Hi Patrick,

Patrick wrote:
>The patches for 2.4 and 2.6 don't apply, your mailer may have mangled
>them. Please send them again as attachment.

I attached both patches. Sorry for all the complications.


>Regards
>Patrick

Cheers,

-- 
Samuel Jean
SysAdmin & NetAdmin
at cookinglinux.org

[-- Attachment #2: ipt_TTL.c.patch-2.4 --]
[-- Type: text/plain, Size: 668 bytes --]

--- linux/net/ipv4/netfilter/ipt_TTL.c.orig	2004-07-24 21:09:23.000000000 -0400
+++ linux/net/ipv4/netfilter/ipt_TTL.c	2004-07-25 18:15:08.000000000 -0400
@@ -37,7 +37,7 @@ static unsigned int ipt_ttl_target(struc
 				new_ttl = 255;
 			break;
 		case IPT_TTL_DEC:
-			new_ttl = iph->ttl + info->ttl;
+			new_ttl = iph->ttl - info->ttl;
 			if (new_ttl < 0)
 				new_ttl = 0;
 			break;
@@ -85,11 +85,6 @@ static int ipt_ttl_checkentry(const char
 		return 0;
 	}
 
-	if ((info->mode != IPT_TTL_SET) && (info->ttl == 0)) {
-		printk(KERN_WARNING "TTL: increment/decrement doesn't make sense with value 0\n");
-		return 0;
-	}
-	
 	return 1;
 }
 

[-- Attachment #3: ipt_TTL.c.patch-2.6 --]
[-- Type: text/plain, Size: 676 bytes --]

--- linux-2.6/net/ipv4/netfilter/ipt_TTL.c.orig	2004-07-25 18:30:56.000000000 -0400
+++ linux-2.6/net/ipv4/netfilter/ipt_TTL.c	2004-07-25 18:31:52.000000000 -0400
@@ -43,7 +43,7 @@ ipt_ttl_target(struct sk_buff **pskb, co
 				new_ttl = 255;
 			break;
 		case IPT_TTL_DEC:
-			new_ttl = iph->ttl + info->ttl;
+			new_ttl = iph->ttl - info->ttl;
 			if (new_ttl < 0)
 				new_ttl = 0;
 			break;
@@ -91,11 +91,6 @@ static int ipt_ttl_checkentry(const char
 		return 0;
 	}
 
-	if ((info->mode != IPT_TTL_SET) && (info->ttl == 0)) {
-		printk(KERN_WARNING "TTL: increment/decrement doesn't make sense with value 0\n");
-		return 0;
-	}
-	
 	return 1;
 }
 

Re: [PATCH 2.4/2.6]: TTL target

[Patrick McHardy]

Samuel Jean wrote:
> 
> I attached both patches. Sorry for all the complications.

All three patches applied, thanks.

Regards
Patrick