Chain name length inconsistent

Chain name length inconsistent

[Thomas Woerner]

Hello,

the size of a chain name is not consistent:

1) Adding a new chain name is checking for max length 30:

iptabels.c:1881 ( do_command):
         if (chain && strlen(chain) > IPT_FUNCTION_MAXNAMELEN)
                 xtables_error(PARAMETER_PROBLEM,
                            "chain name `%s' too long (must be under %i chars)",
                            chain, IPT_FUNCTION_MAXNAMELEN);

include/linux/netfilter_ipv4/ip_tables.h
#define IPT_FUNCTION_MAXNAMELEN XT_FUNCTION_MAXNAMELEN

include/linux/netfilter/x_tables.h:
#define XT_FUNCTION_MAXNAMELEN 30


2) Using a jump target results in a check for max length 31:

iptables.c:1564 (do_command):
                         jumpto = parse_target(optarg);


iptables.c:464 (parse_target):
         if (strlen(targetname)+1 > sizeof(ipt_chainlabel))
                 xtables_error(PARAMETER_PROBLEM,
                            "Invalid target name `%s' (%u chars max)",
                            targetname, (unsigned int)sizeof(ipt_chainlabel)-1);

include/libiptc/libiptc.h:
         typedef char ipt_chainlabel[32];


3) But setting the target copies the name in an array of size 29:

iptables.c:1576 (do_command):
                                 strcpy(target->t->u.user.name, jumpto);

include/linux/netfilter/x_tables.h:
struct xt_entry_match {
         union {
                 struct {
                         __u16 match_size;

                         /* Used by userspace */
                         char name[XT_FUNCTION_MAXNAMELEN-1];

                         __u8 revision;
                 } user;
                 struct {
                         __u16 match_size;

                         /* Used inside the kernel */
                         struct xt_match *match;
                 } kernel;

                 /* Total length */
                 __u16 match_size;
         } u;

         unsigned char data[0];
};

Therefore all the checks should be for max length 29, right?

Please also have a look at
http://bugzilla.netfilter.org/show_bug.cgi?id=641
https://bugzilla.redhat.com/show_bug.cgi?id=545600


Thanks,
Thomas

Re: Chain name length inconsistent

[Jan Engelhardt]

On Tuesday 2010-03-16 16:35, Thomas Woerner wrote:
>
> the size of a chain name is not consistent:
>
> 1) Adding a new chain name is checking for max length 30:

This is correct. Given a long enough name, you already get:

iptables-restore v1.4.7: error creating chain
'xxxabcdefghijklmnopqrstuvwxyz123':Invalid argument


> iptables.c:464 (parse_target):
>        if (strlen(targetname)+1 > sizeof(ipt_chainlabel))

Well, this isn't  :3

> Therefore all the checks should be for max length 29, right?

Nope; 30 chars for the name, +1 for '\0' and +1 for revision to make 32.


I thus have this patch in

	git://dev.medozas.de/iptables master

which now fends off illegal target names

iptables-restore v1.4.7: Invalid target name `xxxabcdefghijklmnopqrstuvwxyz123'
(31 chars max)


parent 89b6c32f88be47e83c3f6e7f8fee812088cb8c22 (v1.4.7-3-g89b6c32)
commit 565a1b6371b856df15970dbc4fcdabcb935e50ce
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Tue Mar 16 16:49:21 2010 +0100

iptables: correctly check for too-long target name

"-j foooo" was not being checked for the proper length (did 32
instead of 30.)

References: http://bugzilla.netfilter.org/show_bug.cgi?id=641
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 ip6tables.c |    2 +-
 iptables.c  |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ip6tables.c b/ip6tables.c
index e2359df..4200cf3 100644
--- a/ip6tables.c
+++ b/ip6tables.c
@@ -456,7 +456,7 @@ parse_target(const char *targetname)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name (too short)");
 
-	if (strlen(targetname)+1 > sizeof(ip6t_chainlabel))
+	if (strlen(targetname) > XT_FUNCTION_MAXNAMELEN)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name `%s' (%u chars max)",
 			   targetname, (unsigned int)sizeof(ip6t_chainlabel)-1);
diff --git a/iptables.c b/iptables.c
index 08eb134..5fab7d2 100644
--- a/iptables.c
+++ b/iptables.c
@@ -460,7 +460,7 @@ parse_target(const char *targetname)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name (too short)");
 
-	if (strlen(targetname)+1 > sizeof(ipt_chainlabel))
+	if (strlen(targetname) > XT_FUNCTION_MAXNAMELEN)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name `%s' (%u chars max)",
 			   targetname, (unsigned int)sizeof(ipt_chainlabel)-1);
-- 
# Created with git-export-patch

Re: Chain name length inconsistent

[Thomas Woerner]

1) Newer glibc versions are checking for overflows in targets of string functions. Therefore 
you should use memcpy instead of strcpy. The target is 29 chars while the source is up to 30 
chars.

2) If the name is 30 chars +1 for '\0' and +1 for revision to make 32, why is 
xt_entry_match.user.name 29 chars in size then? revision will contain the last byte then if 
the memory alignment is 1 byte, right? For 4 byte alignments this will be not the case. See 
ia64 and others.

On 03/16/2010 04:51 PM, Jan Engelhardt wrote:
>
> On Tuesday 2010-03-16 16:35, Thomas Woerner wrote:
>>
>> the size of a chain name is not consistent:
>>
>> 1) Adding a new chain name is checking for max length 30:
>
> This is correct. Given a long enough name, you already get:
>
> iptables-restore v1.4.7: error creating chain
> 'xxxabcdefghijklmnopqrstuvwxyz123':Invalid argument
>
>
>> iptables.c:464 (parse_target):
>>         if (strlen(targetname)+1>  sizeof(ipt_chainlabel))
>
> Well, this isn't  :3
>
>> Therefore all the checks should be for max length 29, right?
>
> Nope; 30 chars for the name, +1 for '\0' and +1 for revision to make 32.
>
>
> I thus have this patch in
>
> 	git://dev.medozas.de/iptables master
>
> which now fends off illegal target names
>
> iptables-restore v1.4.7: Invalid target name `xxxabcdefghijklmnopqrstuvwxyz123'
> (31 chars max)
>
>
> parent 89b6c32f88be47e83c3f6e7f8fee812088cb8c22 (v1.4.7-3-g89b6c32)
> commit 565a1b6371b856df15970dbc4fcdabcb935e50ce
> Author: Jan Engelhardt<jengelh@medozas.de>
> Date:   Tue Mar 16 16:49:21 2010 +0100
>
> iptables: correctly check for too-long target name
>
> "-j foooo" was not being checked for the proper length (did 32
> instead of 30.)
>
> References: http://bugzilla.netfilter.org/show_bug.cgi?id=641
> Signed-off-by: Jan Engelhardt<jengelh@medozas.de>
> ---
>   ip6tables.c |    2 +-
>   iptables.c  |    2 +-
>   2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ip6tables.c b/ip6tables.c
> index e2359df..4200cf3 100644
> --- a/ip6tables.c
> +++ b/ip6tables.c
> @@ -456,7 +456,7 @@ parse_target(const char *targetname)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name (too short)");
>
> -	if (strlen(targetname)+1>  sizeof(ip6t_chainlabel))
> +	if (strlen(targetname)>  XT_FUNCTION_MAXNAMELEN)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name `%s' (%u chars max)",
>   			   targetname, (unsigned int)sizeof(ip6t_chainlabel)-1);
> diff --git a/iptables.c b/iptables.c
> index 08eb134..5fab7d2 100644
> --- a/iptables.c
> +++ b/iptables.c
> @@ -460,7 +460,7 @@ parse_target(const char *targetname)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name (too short)");
>
> -	if (strlen(targetname)+1>  sizeof(ipt_chainlabel))
> +	if (strlen(targetname)>  XT_FUNCTION_MAXNAMELEN)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name `%s' (%u chars max)",
>   			   targetname, (unsigned int)sizeof(ipt_chainlabel)-1);


-- 
Thomas Woerner
Software Engineer            Phone: +49-711-96437-310
Red Hat GmbH                 Fax  : +49-711-96437-111
Hauptstaetterstr. 58         Email: Thomas Woerner <twoerner@redhat.com>
D-70178 Stuttgart            Web  : http://www.redhat.de/

Send packet back out on same interface it came in on

[Robert Szabo]

Hi all,

I am working on a kernel module that would allow me to send a packet back out on the interface it came in on. (i.e. a simple heartbeat response, where I have not control over the server as its not mine to play with.



I have currently configured a bridge using bridge-utils for eth0 and eth1 called br0.

I then created a netfilter kernel module with 2 hooks, one for NF_IP_FORWARD which intercepts the packet on the bridge and queues it, and the second one for NF_IP_POST_ROUTING to handle the altered payload. 

A user space program manipulates the queued packets and rebuilds the ip and tcp sections including the checksum calculations, ip address and port manipulation and  then passes it back in the verdict. 

The post routing hook then needs to detect that packet and send it back on the same interface the original request it was sent on. I have a simple routine that I was hoping would work to alter the mac addresses and input/output devices and put the response back on the queue but it fails to show up in tcpdump.


int swapAndQueuePacket(struct sk_buff *skb)
{
    u_char tmp[6];
    struct net_device *odev,*idev;
    struct ethhdr *ethdr;
    int rc = NOT_OK;
   

    odev = dev_get_by_name(getIngresIf());
    idev = dev_get_by_name(getEgresIf());

    ethdr = (struct ethhdr *)skb->mac.raw;
    if (ethdr != NULL)
        goto swap_finish;

    skb->dev=odev;
    skb->input_dev=idev;
    skb->pkt_type = PACKET_OTHERHOST;
    skb->protocol = __constant_htons(ETH_P_IP);
    skb->priority = 0;
    skb->csum = skb_checksum (skb, skb->nh.iph->ihl*4, skb->len - skb->nh.iph->ihl * 4, 0);

    memcpy(tmp,ethdr->h_dest,ETH_ALEN);
    memcpy (ethdr->h_dest, ethdr->h_source, ETH_ALEN);
    memcpy (ethdr->h_source, tmp, ETH_ALEN);
    if (0 > dev_queue_xmit(skb)) goto swap_out;
    goto swap_finish;

swap_out:
  rc = OK;

swap_finish:
    return rc;
}

Note: I am leery to rebuild the Ethernet header as I may have many QinQ vlan tags to consider.

The code that engineers the IP/TCP payload is sound as that code I have tested in my other bridge implementation which was completely done in user space.

Any help/pointers is greatly appreciated.
 
****************************************************************************************
This email and any files transmitted with are confidential and intended solely for the
use of the individual or entity to whom they are addressed.  If you have received this
email in error then please delete it and notify the sender. Do not make a copy or forward
it to anyone.  This footnote also confirms that this email message has been swept for the
presence of computer viruses.

Adaptive Mobile Security Ltd, Dublin Technology Centre, Taylors Lane, Dublin 8,Ireland
Directors: B. Collins, G. Maclachlan (UK), S. Tirtey (DE), J. Ennis (UK), D. Summers (UK).
Registered in Ireland, Company No. 370343, VAT Reg.No.IE63903430
****************************************************************************************

Re: Chain name length inconsistent

[Jan Engelhardt]

On Tuesday 2010-03-16 17:28, Thomas Woerner wrote:

>1) Newer glibc versions are checking for overflows in targets of string 
>functions. Therefore you should use memcpy instead of strcpy. The 
>target is 29 chars while the source is up to 30 chars.

So if it checks for *string* functions (I think it only does that when 
-D_FORTIFY_SOURCE is set), why should I use a *memory* function over
the string function that's already in?

>2) If the name is 30 chars +1 for '\0' and +1 for revision to make 32, 
>why is xt_entry_match.user.name 29 chars in size then?

Oh, for match_size/target_size. So it's 29 +1 +1 +2. I shall revise the 
patch immediately.

>revision will contain the last byte then if the memory alignment is 1 
>byte, right? For 4 byte alignments this will be not the case. See ia64 
>and others.

__u8 won't be aligned on a 4-byte boundary (ignoring the weird ARM 
ABI).


parent 89b6c32f88be47e83c3f6e7f8fee812088cb8c22 (v1.4.7-3-g89b6c32)
commit 21d1283750d9c4df7ca80165d2b9dc0b9bd214eb
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Tue Mar 16 16:49:21 2010 +0100

iptables: correctly check for too-long chain/target/match names

* iptables-restore was not checking for chain name length
* iptables was not checking for match name length
* target length was checked against 32, not 29.

References: http://bugzilla.netfilter.org/show_bug.cgi?id=641
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 ip6tables-restore.c |    6 ++++++
 ip6tables.c         |    4 ++--
 iptables-restore.c  |    6 ++++++
 iptables.c          |    4 ++--
 xtables.c           |    5 +++++
 5 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/ip6tables-restore.c b/ip6tables-restore.c
index d0efbee..f0725d1 100644
--- a/ip6tables-restore.c
+++ b/ip6tables-restore.c
@@ -253,6 +253,12 @@ int main(int argc, char *argv[])
 				exit(1);
 			}
 
+			if (strlen(chain) > XT_FUNCTION_MAXNAMELEN - 1)
+				xtables_error(PARAMETER_PROBLEM,
+					   "Invalid chain name `%s' "
+					   "(%u chars max)",
+					   chain, XT_FUNCTION_MAXNAMELEN - 1);
+
 			if (ip6tc_builtin(chain, handle) <= 0) {
 				if (noflush && ip6tc_is_chain(chain, handle)) {
 					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
diff --git a/ip6tables.c b/ip6tables.c
index e2359df..6ee4281 100644
--- a/ip6tables.c
+++ b/ip6tables.c
@@ -456,10 +456,10 @@ parse_target(const char *targetname)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name (too short)");
 
-	if (strlen(targetname)+1 > sizeof(ip6t_chainlabel))
+	if (strlen(targetname) > XT_FUNCTION_MAXNAMELEN - 1)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name `%s' (%u chars max)",
-			   targetname, (unsigned int)sizeof(ip6t_chainlabel)-1);
+			   targetname, XT_FUNCTION_MAXNAMELEN - 1);
 
 	for (ptr = targetname; *ptr; ptr++)
 		if (isspace(*ptr))
diff --git a/iptables-restore.c b/iptables-restore.c
index 86d63e2..4a74485 100644
--- a/iptables-restore.c
+++ b/iptables-restore.c
@@ -259,6 +259,12 @@ main(int argc, char *argv[])
 				exit(1);
 			}
 
+			if (strlen(chain) > XT_FUNCTION_MAXNAMELEN - 1)
+				xtables_error(PARAMETER_PROBLEM,
+					   "Invalid chain name `%s' "
+					   "(%u chars max)",
+					   chain, XT_FUNCTION_MAXNAMELEN - 1);
+
 			if (iptc_builtin(chain, handle) <= 0) {
 				if (noflush && iptc_is_chain(chain, handle)) {
 					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
diff --git a/iptables.c b/iptables.c
index 08eb134..25bc8cc 100644
--- a/iptables.c
+++ b/iptables.c
@@ -460,10 +460,10 @@ parse_target(const char *targetname)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name (too short)");
 
-	if (strlen(targetname)+1 > sizeof(ipt_chainlabel))
+	if (strlen(targetname) > XT_FUNCTION_MAXNAMELEN - 1)
 		xtables_error(PARAMETER_PROBLEM,
 			   "Invalid target name `%s' (%u chars max)",
-			   targetname, (unsigned int)sizeof(ipt_chainlabel)-1);
+			   targetname, XT_FUNCTION_MAXNAMELEN - 1);
 
 	for (ptr = targetname; *ptr; ptr++)
 		if (isspace(*ptr))
diff --git a/xtables.c b/xtables.c
index f3baf84..7340c87 100644
--- a/xtables.c
+++ b/xtables.c
@@ -545,6 +545,11 @@ xtables_find_match(const char *name, enum xtables_tryload tryload,
 	struct xtables_match *ptr;
 	const char *icmp6 = "icmp6";
 
+	if (strlen(name) > XT_FUNCTION_MAXNAMELEN - 1)
+		xtables_error(PARAMETER_PROBLEM,
+			   "Invalid match name \"%s\" (%u chars max)",
+			   name, XT_FUNCTION_MAXNAMELEN - 1);
+
 	/* This is ugly as hell. Nonetheless, there is no way of changing
 	 * this without hurting backwards compatibility */
 	if ( (strcmp(name,"icmpv6") == 0) ||
-- 
# Created with git-export-patch

Re: Chain name length inconsistent

[Thomas Woerner]

Hello Jan,

there are still two problems:

1) You could create a chain with length 30:

diff --git a/ip6tables.c b/ip6tables.c
index 6ee4281..80af032 100644
--- a/ip6tables.c
+++ b/ip6tables.c
@@ -1840,7 +1840,7 @@ int do_command6(int argc, char *argv[], char 
**table, stru

         generic_opt_check(command, options);

-       if (chain && strlen(chain) > IP6T_FUNCTION_MAXNAMELEN)
+       if (chain && strlen(chain) > IP6T_FUNCTION_MAXNAMELEN - 1)
                 xtables_error(PARAMETER_PROBLEM,
                            "chain name `%s' too long (must be under %i 
chars)",
                            chain, IP6T_FUNCTION_MAXNAMELEN);
diff --git a/iptables.c b/iptables.c
index 25bc8cc..2139b98 100644
--- a/iptables.c
+++ b/iptables.c
@@ -1878,7 +1878,7 @@ int do_command(int argc, char *argv[], char 
**table, struc

         generic_opt_check(command, options);

-       if (chain && strlen(chain) > IPT_FUNCTION_MAXNAMELEN)
+       if (chain && strlen(chain) > IPT_FUNCTION_MAXNAMELEN - 1)
                 xtables_error(PARAMETER_PROBLEM,
                            "chain name `%s' too long (must be under %i 
chars)",
                            chain, IPT_FUNCTION_MAXNAMELEN);


On 03/16/2010 05:55 PM, Jan Engelhardt wrote:
> On Tuesday 2010-03-16 17:28, Thomas Woerner wrote:
>
>> 1) Newer glibc versions are checking for overflows in targets of string
>> functions. Therefore you should use memcpy instead of strcpy. The
>> target is 29 chars while the source is up to 30 chars.
>
> So if it checks for *string* functions (I think it only does that when
> -D_FORTIFY_SOURCE is set), why should I use a *memory* function over
> the string function that's already in?
>
2) _FORTIFY_SOURCE is enabled on all newer Fedora and Red Hat 
distributions, therefore this could lead into an abort

strcpy will copy the string (29 chars max) and the '\0' which makes up 
to 30 into a field of length 29. Therefore this is an overflow. Either 
you have to reduce the max length to 28 or you have to use another 
function to copy the string (memcpy or strncpy with max length 29). BTW: 
Is it needed that xt_entry_match.u.name is null terminated?

>> 2) If the name is 30 chars +1 for '\0' and +1 for revision to make 32,
>> why is xt_entry_match.user.name 29 chars in size then?
>
> Oh, for match_size/target_size. So it's 29 +1 +1 +2. I shall revise the
> patch immediately.
>
>> revision will contain the last byte then if the memory alignment is 1
>> byte, right? For 4 byte alignments this will be not the case. See ia64
>> and others.
>
> __u8 won't be aligned on a 4-byte boundary (ignoring the weird ARM
> ABI).
>
>
> parent 89b6c32f88be47e83c3f6e7f8fee812088cb8c22 (v1.4.7-3-g89b6c32)
> commit 21d1283750d9c4df7ca80165d2b9dc0b9bd214eb
> Author: Jan Engelhardt<jengelh@medozas.de>
> Date:   Tue Mar 16 16:49:21 2010 +0100
>
> iptables: correctly check for too-long chain/target/match names
>
> * iptables-restore was not checking for chain name length
> * iptables was not checking for match name length
> * target length was checked against 32, not 29.
>
> References: http://bugzilla.netfilter.org/show_bug.cgi?id=641
> Signed-off-by: Jan Engelhardt<jengelh@medozas.de>
> ---
>   ip6tables-restore.c |    6 ++++++
>   ip6tables.c         |    4 ++--
>   iptables-restore.c  |    6 ++++++
>   iptables.c          |    4 ++--
>   xtables.c           |    5 +++++
>   5 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/ip6tables-restore.c b/ip6tables-restore.c
> index d0efbee..f0725d1 100644
> --- a/ip6tables-restore.c
> +++ b/ip6tables-restore.c
> @@ -253,6 +253,12 @@ int main(int argc, char *argv[])
>   				exit(1);
>   			}
>
> +			if (strlen(chain)>  XT_FUNCTION_MAXNAMELEN - 1)
> +				xtables_error(PARAMETER_PROBLEM,
> +					   "Invalid chain name `%s' "
> +					   "(%u chars max)",
> +					   chain, XT_FUNCTION_MAXNAMELEN - 1);
> +
>   			if (ip6tc_builtin(chain, handle)<= 0) {
>   				if (noflush&&  ip6tc_is_chain(chain, handle)) {
>   					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
> diff --git a/ip6tables.c b/ip6tables.c
> index e2359df..6ee4281 100644
> --- a/ip6tables.c
> +++ b/ip6tables.c
> @@ -456,10 +456,10 @@ parse_target(const char *targetname)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name (too short)");
>
> -	if (strlen(targetname)+1>  sizeof(ip6t_chainlabel))
> +	if (strlen(targetname)>  XT_FUNCTION_MAXNAMELEN - 1)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name `%s' (%u chars max)",
> -			   targetname, (unsigned int)sizeof(ip6t_chainlabel)-1);
> +			   targetname, XT_FUNCTION_MAXNAMELEN - 1);
>
>   	for (ptr = targetname; *ptr; ptr++)
>   		if (isspace(*ptr))
> diff --git a/iptables-restore.c b/iptables-restore.c
> index 86d63e2..4a74485 100644
> --- a/iptables-restore.c
> +++ b/iptables-restore.c
> @@ -259,6 +259,12 @@ main(int argc, char *argv[])
>   				exit(1);
>   			}
>
> +			if (strlen(chain)>  XT_FUNCTION_MAXNAMELEN - 1)
> +				xtables_error(PARAMETER_PROBLEM,
> +					   "Invalid chain name `%s' "
> +					   "(%u chars max)",
> +					   chain, XT_FUNCTION_MAXNAMELEN - 1);
> +
>   			if (iptc_builtin(chain, handle)<= 0) {
>   				if (noflush&&  iptc_is_chain(chain, handle)) {
>   					DEBUGP("Flushing existing user defined chain '%s'\n", chain);
> diff --git a/iptables.c b/iptables.c
> index 08eb134..25bc8cc 100644
> --- a/iptables.c
> +++ b/iptables.c
> @@ -460,10 +460,10 @@ parse_target(const char *targetname)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name (too short)");
>
> -	if (strlen(targetname)+1>  sizeof(ipt_chainlabel))
> +	if (strlen(targetname)>  XT_FUNCTION_MAXNAMELEN - 1)
>   		xtables_error(PARAMETER_PROBLEM,
>   			   "Invalid target name `%s' (%u chars max)",
> -			   targetname, (unsigned int)sizeof(ipt_chainlabel)-1);
> +			   targetname, XT_FUNCTION_MAXNAMELEN - 1);
>
>   	for (ptr = targetname; *ptr; ptr++)
>   		if (isspace(*ptr))
> diff --git a/xtables.c b/xtables.c
> index f3baf84..7340c87 100644
> --- a/xtables.c
> +++ b/xtables.c
> @@ -545,6 +545,11 @@ xtables_find_match(const char *name, enum xtables_tryload tryload,
>   	struct xtables_match *ptr;
>   	const char *icmp6 = "icmp6";
>
> +	if (strlen(name)>  XT_FUNCTION_MAXNAMELEN - 1)
> +		xtables_error(PARAMETER_PROBLEM,
> +			   "Invalid match name \"%s\" (%u chars max)",
> +			   name, XT_FUNCTION_MAXNAMELEN - 1);
> +
>   	/* This is ugly as hell. Nonetheless, there is no way of changing
>   	 * this without hurting backwards compatibility */
>   	if ( (strcmp(name,"icmpv6") == 0) ||

Thanks,
Thomas

Re: Chain name length inconsistent

[Jan Engelhardt]

On Thursday 2010-03-18 17:13, Thomas Woerner wrote:

> 1) You could create a chain with length 30:
>
> diff --git a/ip6tables.c b/ip6tables.c
> index 6ee4281..80af032 100644
> --- a/ip6tables.c
> +++ b/ip6tables.c
> @@ -1840,7 +1840,7 @@ int do_command6(int argc, char *argv[], char **table,
> stru
>
>        generic_opt_check(command, options);
>
> -       if (chain && strlen(chain) > IP6T_FUNCTION_MAXNAMELEN)
> +       if (chain && strlen(chain) > IP6T_FUNCTION_MAXNAMELEN - 1)
>                xtables_error(PARAMETER_PROBLEM,
>                           "chain name `%s' too long (must be under %i chars)",
>                           chain, IP6T_FUNCTION_MAXNAMELEN);

I'll take care of it.


>>> 1) Newer glibc versions are checking for overflows in targets of string
>>> functions. Therefore you should use memcpy instead of strcpy. The
>>> target is 29 chars while the source is up to 30 chars.
>>
>> So if it checks for *string* functions (I think it only does that when
>> -D_FORTIFY_SOURCE is set), why should I use a *memory* function over
>> the string function that's already in?
>>
> 2) _FORTIFY_SOURCE is enabled on all newer Fedora and Red Hat distributions,
> therefore this could lead into an abort

You have to think outside of your preferred distro sometimes. :)

> strcpy will copy the string (29 chars max) and the '\0' which makes up to 30
> into a field of length 29. Therefore this is an overflow. Either you have to
> reduce the max length to 28 or you have to use another function to copy the
> string (memcpy or strncpy with max length 29). BTW: Is it needed that
> xt_entry_match.u.name is null terminated?

Yes, the kernel's x_tables.c will use plain strcmp when looking
for ".name" in struct xt_{match,target}.

Re: Chain name length inconsistent

[Thomas Woerner]

On 03/22/2010 07:18 PM, Jan Engelhardt wrote:
>
> On Thursday 2010-03-18 17:13, Thomas Woerner wrote:
>
>> 1) You could create a chain with length 30:
>>
>> diff --git a/ip6tables.c b/ip6tables.c
>> index 6ee4281..80af032 100644
>> --- a/ip6tables.c
>> +++ b/ip6tables.c
>> @@ -1840,7 +1840,7 @@ int do_command6(int argc, char *argv[], char **table,
>> stru
>>
>>         generic_opt_check(command, options);
>>
>> -       if (chain&&  strlen(chain)>  IP6T_FUNCTION_MAXNAMELEN)
>> +       if (chain&&  strlen(chain)>  IP6T_FUNCTION_MAXNAMELEN - 1)
>>                 xtables_error(PARAMETER_PROBLEM,
>>                            "chain name `%s' too long (must be under %i chars)",
>>                            chain, IP6T_FUNCTION_MAXNAMELEN);
>
> I'll take care of it.
>
>
>>>> 1) Newer glibc versions are checking for overflows in targets of string
>>>> functions. Therefore you should use memcpy instead of strcpy. The
>>>> target is 29 chars while the source is up to 30 chars.
>>>
>>> So if it checks for *string* functions (I think it only does that when
>>> -D_FORTIFY_SOURCE is set), why should I use a *memory* function over
>>> the string function that's already in?
>>>
>> 2) _FORTIFY_SOURCE is enabled on all newer Fedora and Red Hat distributions,
>> therefore this could lead into an abort
>
> You have to think outside of your preferred distro sometimes. :)


>
>> strcpy will copy the string (29 chars max) and the '\0' which makes up to 30
>> into a field of length 29. Therefore this is an overflow. Either you have to
>> reduce the max length to 28 or you have to use another function to copy the
>> string (memcpy or strncpy with max length 29). BTW: Is it needed that
>> xt_entry_match.u.name is null terminated?
>
> Yes, the kernel's x_tables.c will use plain strcmp when looking
> for ".name" in struct xt_{match,target}.

1) If your chain name is 29 chars in length, you are copying the chain 
name plus '\0' into xt_entry_match.u.name, but xt_entry_match.u.name is 
only 29 chars long. Is it intended that '\0' will be placed into 
revision in this case if there is no destination size check?

2) If there are these chains:

chain1: "123456789012345678901234567"
chain2: "1234567890123456789012345678"
chain3: "12345678901234567890123456789"

What will be the value of xt_entry_match.u.user.name after the call

iptables.c:1576:, ip6tables.c:1561: (do_command jump case)
	xtables_set_revision(target->t->u.user.name, target->revision);
and
iptables.c:1635: , ip6tables.c:1614: (do_command match case)
	xtables_set_revision(m->m->u.user.name, m->revision);

target->revision defaults to '\0', right?

   void xtables_set_revision(char *name, u_int8_t revision)
   {
         /* Old kernel sources don't have ".revision" field,
         *            but we stole a byte from name. */
         name[XT_FUNCTION_MAXNAMELEN - 2] = '\0';
         name[XT_FUNCTION_MAXNAMELEN - 1] = revision;
   }

I would say it will be the same for all these chains.

Therefore it would be good to check for max chain name length < 28.

Otherwise this could end up in using the wrong chain, right?


Thomas

-- 
Thomas Woerner
Software Engineer            Phone: +49-711-96437-310
Red Hat GmbH                 Fax  : +49-711-96437-111
Hauptstaetterstr. 58         Email: Thomas Woerner <twoerner@redhat.com>
D-70178 Stuttgart            Web  : http://www.redhat.de/