[BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Matthias Andree]

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

You can use "bk receive" to patch with this mail.

BK: Parent repository is bk://linux.bkbits.net/linux-2.5

Patch description:
ChangeSet@1.2427, 2004-11-04 13:00:54+01:00, matthias.andree@gmx.de
    Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps.
  
    Fix a bug where the ip_conntrack_amanda module replaces the first LF
    after "CONNECT " by a NUL byte. This causes the UDP checksum to become
    corrupt and strips off the OPTIONS argument from the received packet,
    breaking amanda's sendbackup component altogether.  Replace the LF
    character before releasing the buffer.
  
    Signed-off-by: Matthias Andree <matthias.andree@gmx.de>

Matthias Andree

------------------------------------------------------------------------

##### DIFFSTAT #####
 ip_conntrack_amanda.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

##### GNUPATCH #####
--- 1.10/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-08-19 02:14:53 +02:00
+++ 1.11/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-04 12:59:26 +01:00
@@ -49,7 +49,7 @@
 {
 	struct ip_conntrack_expect *exp;
 	struct ip_ct_amanda_expect *exp_amanda_info;
-	char *amp, *data, *data_limit, *tmp;
+	char *amp, *data, *data_limit, *tmp, *le = 0;
 	unsigned int dataoff, i;
 	u_int16_t port, len;
 
@@ -83,9 +83,10 @@
 		goto out;
 	data += strlen("CONNECT ");
 
-	/* Only search first line. */	
-	if ((tmp = strchr(data, '\n')))
-		*tmp = '\0';
+	/* Only search first line.
+	 * NB: The change to the data must be reverted later! */
+	if ((le = strchr(data, '\n')))
+		*le = '\0';
 
 	for (i = 0; i < ARRAY_SIZE(conns); i++) {
 		char *match = strstr(data, conns[i]);
@@ -120,6 +121,9 @@
 	}
 
 out:
+	/* replace LF character to repair the packet */
+	if (le)
+	    *le = '\n';
 	UNLOCK_BH(&amanda_buffer_lock);
 	return NF_ACCEPT;
 }



##### BKPATCH #####

## Wrapped with gzip_b64 ##
H4sIADodikECA+1WWW/bRhB+5v6KafIgW4nIXV6SmSpQbCWtEFcyfDw1hbFcLiXWvLC7VCxA
P75D6mhauGhcBH2KJFJ7zH5zfTPYlzCbRpap1JrniZ5ws8qr0jaKl7qQhtuiKrYXK14u5Y00
W5dSF78BG3o0CLcspP5wK1jCGPeZTKjrj0KfvIQ7LVVkFdyYVca1zctESYnrP1faRNayeLST
dnpdVTh1Gq0crYSTZ2XzOHDtcHD+keD2FTdiBWupdGQx2zuumE0tI+v6/U93l++uCRmP4Wgh
jMfk2zpjJC9VVVcTIezfN42dZr8eFPz2VzCfMcpY4DN3uA1Y6IZkCsx2fXcI1HcYc6gPzIso
jQL/FWU4gL9FaLKLDLxiMKDkHL6tJxdEAMCH7BGy+l5UZYlw4uGeF6icQ8INB1Ep1dQmq0qI
myWYFTcQK8kfNBzEmqLWNiIdwXgn+nkllcQD8knwokqaXIKSdc6F1J1cmilt4PJDB8RTIxW8
uFjM5+8vbuEFxBsEnt9d4sBIG25XmQbBG70/fDe9ArGS4kE3BYYJYokRkR3U3gdAxaCNymoN
VZp2pxZXt7PF/Aa4WjaFLA2kqiq6HSWFzNYygRqtluZ1h9R5npXLve89DVqWSYwSTY1qiroq
WxCem2opEUXZANc7FzvQvW9ixTEWrX+xTCvVKssl1y1wKxU3aYpHDyG9yZalTAZo8iDeRPDL
niHwrmMI/Pg0Zd6SjxAwL6Tk6s9iIINnfgihnJK3UEqTZjlaPMlKfGNdPtqxsAXf4o6T1Wvf
OYo4T+TbFi0lPep5zPW8M6wHb0jpcMvdNKVuSgM6FCPXH/4D/5+lpa06fIIzN9z6qM/tGsLX
IrT94v/yliR8LYtJ2Rhtd8O8So2N0M/zl47YGaXMD7yt547Yrssw9kWPcaPgLHLDf+kxIxj4
33vM9x7zrB6zq7AFDNTn/Q8bztfS9z/0o2ngAiOz7m21TkKfF/Vr6LdU2v/d51mRGZyYbgc5
MAb6hkxHIXhkNhqBTyynD4sy32BwucIbxI4XWOnSJhb0YX4eYfplG0ake5vsNmodXYsGJeM2
oHgRMZi9nKODP0DfIVaWwslJpw9JIFbqZGdV71PZOz09JZa1M6b3ifbekBlGDg1qTdlzFJP3
ReJQKa7zTHW6dxw5asklwmHm4IBYIuLxQnRg6Zhzj8mQJuQPFIcUW9cJAAA=

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Patrick McHardy]

Matthias Andree wrote:

>You can use "bk receive" to patch with this mail.
>
>BK: Parent repository is bk://linux.bkbits.net/linux-2.5
>
>Patch description:
>ChangeSet@1.2427, 2004-11-04 13:00:54+01:00, matthias.andree@gmx.de
>    Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps.
>  
>    Fix a bug where the ip_conntrack_amanda module replaces the first LF
>    after "CONNECT " by a NUL byte. This causes the UDP checksum to become
>    corrupt and strips off the OPTIONS argument from the received packet,
>    breaking amanda's sendbackup component altogether.  Replace the LF
>    character before releasing the buffer.
>  
>
The data that is changed is only a copy, the actual packet is not touched.

Regards
Patrick

>  
>    Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
>
>Matthias Andree
>
>------------------------------------------------------------------------
>
>##### DIFFSTAT #####
> ip_conntrack_amanda.c |   12 ++++++++----
> 1 files changed, 8 insertions(+), 4 deletions(-)
>
>##### GNUPATCH #####
>--- 1.10/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-08-19 02:14:53 +02:00
>+++ 1.11/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-04 12:59:26 +01:00
>@@ -49,7 +49,7 @@
> {
> 	struct ip_conntrack_expect *exp;
> 	struct ip_ct_amanda_expect *exp_amanda_info;
>-	char *amp, *data, *data_limit, *tmp;
>+	char *amp, *data, *data_limit, *tmp, *le = 0;
> 	unsigned int dataoff, i;
> 	u_int16_t port, len;
> 
>@@ -83,9 +83,10 @@
> 		goto out;
> 	data += strlen("CONNECT ");
> 
>-	/* Only search first line. */	
>-	if ((tmp = strchr(data, '\n')))
>-		*tmp = '\0';
>+	/* Only search first line.
>+	 * NB: The change to the data must be reverted later! */
>+	if ((le = strchr(data, '\n')))
>+		*le = '\0';
> 
> 	for (i = 0; i < ARRAY_SIZE(conns); i++) {
> 		char *match = strstr(data, conns[i]);
>@@ -120,6 +121,9 @@
> 	}
> 
> out:
>+	/* replace LF character to repair the packet */
>+	if (le)
>+	    *le = '\n';
> 	UNLOCK_BH(&amanda_buffer_lock);
> 	return NF_ACCEPT;
> }
>
>  
>

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Herbert Xu]

Patrick McHardy <kaber@trash.net> wrote:
>
> The data that is changed is only a copy, the actual packet is not touched.

Does it call skb_ip_make_writable anywhere? If not then it may be
shared/cloned and can't be written at all.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[David S. Miller]

On Fri, 05 Nov 2004 07:45:53 +1100
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Patrick McHardy <kaber@trash.net> wrote:
> >
> > The data that is changed is only a copy, the actual packet is not touched.
> 
> Does it call skb_ip_make_writable anywhere? If not then it may be
> shared/cloned and can't be written at all.

You're right... the bug was introduced by my skb_header_pointer() changes.
Look at this:

	amp = skb_header_pointer(skb, dataoff,
				 skb->len - dataoff, amanda_buffer);
	BUG_ON(amp == NULL);
	data = amp;
	data_limit = amp + skb->len - dataoff;
	*data_limit = '\0';

It should just use the amanda_buffer always.

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Patrick McHardy]

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

David S. Miller wrote:

>You're right... the bug was introduced by my skb_header_pointer() changes.
>Look at this:
>
>	amp = skb_header_pointer(skb, dataoff,
>				 skb->len - dataoff, amanda_buffer);
>	BUG_ON(amp == NULL);
>	data = amp;
>	data_limit = amp + skb->len - dataoff;
>	*data_limit = '\0';
>
>It should just use the amanda_buffer always.
>
Thanks Dave and Herbert, here is the patch in case you haven't fixed it
already.

Regards
Patrick


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1813 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/11/04 22:50:11+01:00 kaber@coreworks.de 
#   [NETFILTER]: Don't use skb_header_pointer in amanda conntrack helper
#   
#   Fixes broken packets, noticed by Matthias Andree <matthias.andree@gmx.de>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/netfilter/ip_conntrack_amanda.c
#   2004/11/04 22:50:04+01:00 kaber@coreworks.de +5 -7
#   [NETFILTER]: Don't use skb_header_pointer in amanda conntrack helper
#   
#   Fixes broken packets, noticed by Matthias Andree <matthias.andree@gmx.de>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/ipv4/netfilter/ip_conntrack_amanda.c b/net/ipv4/netfilter/ip_conntrack_amanda.c
--- a/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-04 22:50:37 +01:00
+++ b/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-04 22:50:37 +01:00
@@ -49,7 +49,7 @@
 {
 	struct ip_conntrack_expect *exp;
 	struct ip_ct_amanda_expect *exp_amanda_info;
-	char *amp, *data, *data_limit, *tmp;
+	char *data, *data_limit, *tmp;
 	unsigned int dataoff, i;
 	u_int16_t port, len;
 
@@ -70,11 +70,9 @@
 	}
 
 	LOCK_BH(&amanda_buffer_lock);
-	amp = skb_header_pointer(skb, dataoff,
-				 skb->len - dataoff, amanda_buffer);
-	BUG_ON(amp == NULL);
-	data = amp;
-	data_limit = amp + skb->len - dataoff;
+	skb_copy_bits(skb, dataoff, amanda_buffer, skb->len - dataoff);
+	data = amanda_buffer;
+	data_limit = amanda_buffer + skb->len - dataoff;
 	*data_limit = '\0';
 
 	/* Search for the CONNECT string */
@@ -110,7 +108,7 @@
 		exp->mask.dst.u.tcp.port = 0xFFFF;
 
 		exp_amanda_info = &exp->help.exp_amanda_info;
-		exp_amanda_info->offset = tmp - amp;
+		exp_amanda_info->offset = tmp - amanda_buffer;
 		exp_amanda_info->port   = port;
 		exp_amanda_info->len    = len;
 

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Matthias Andree]

On Thu, 04 Nov 2004, Patrick McHardy wrote:

> Matthias Andree wrote:
> 
> >You can use "bk receive" to patch with this mail.
> >
> >BK: Parent repository is bk://linux.bkbits.net/linux-2.5
> >
> >Patch description:
> >ChangeSet@1.2427, 2004-11-04 13:00:54+01:00, matthias.andree@gmx.de
> >   Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps.
> > 
> >   Fix a bug where the ip_conntrack_amanda module replaces the first LF
> >   after "CONNECT " by a NUL byte. This causes the UDP checksum to become
> >   corrupt and strips off the OPTIONS argument from the received packet,
> >   breaking amanda's sendbackup component altogether.  Replace the LF
> >   character before releasing the buffer.
> >
> The data that is changed is only a copy, the actual packet is not touched.

Why then does the application not see the packets as long as
ip_conntrack_amanda is loaded and starts seeing them again as soon as
"rmmod ip_conntrack_amanda" has completed?

And I'm not even arguing with tcpdump which may get the altered copy of the packet.

I am unaware of two things:
1. detailed packet flow and SKBs
2. internal ip_conntrack API.

I am however seeing that ip_conntrack_amanda
1. jams the application's protocol
2. modifies the packet.

So is there any evidence to support the "only a copy" theory?

-- 
Matthias Andree

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Matthias Andree]

On Thu, 04 Nov 2004, Patrick McHardy wrote:

> -	data = amp;
> -	data_limit = amp + skb->len - dataoff;
> +	skb_copy_bits(skb, dataoff, amanda_buffer, skb->len - dataoff);
> +	data = amanda_buffer;
> +	data_limit = amanda_buffer + skb->len - dataoff;

Does this mean the whole buffer is still copied?

If so: Making a local copy of the packet just to be able to stuff NUL
bytes to suit or "optimize" strstr functions is plain nonsense - amanda
pipes several GByte through the kernel at each run, and copying
gazillions of bits around, wasting millions of CPU cycles, just because
someone is too lazy to spell a more decent search function, is
bad design.

Same consideration applies to FTP connection tracking.

I wrote a memstr function for bogofilter (GPL v2) that we could use
inside the kernel, as a length-limited strstr replacement, as in "search
the first buffer_size bytes starting with buffer_base for the first
occurrence of const char *needle". That avoids all buffer modifications
in ip_conntrack_amanda.c AFAICS. It's also slow because it does a linear
search and not an optimized search as the sophisticated KMP and other
search algorithms would be able to do, but then again the generic strstr
inside the kernel is linear, too.

-- 
Matthias Andree

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Patrick McHardy]

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

Matthias Andree wrote:

>On Thu, 04 Nov 2004, Patrick McHardy wrote:
>
>>The data that is changed is only a copy, the actual packet is not touched.
>>    
>>
>
>Why then does the application not see the packets as long as
>ip_conntrack_amanda is loaded and starts seeing them again as soon as
>"rmmod ip_conntrack_amanda" has completed?
>  
>

Your observation and your patch were correct, thanks. It is supposed
to be just a copy, I missed that it wasn't anymore. While your patch
works too, and is even faster with non-linear skbs, I don't like the
idea of using the skb as a scratch-area, so I sent this patch to Dave
instead.

Regards
Patrick


[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1813 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2004/11/04 22:50:11+01:00 kaber@coreworks.de 
#   [NETFILTER]: Don't use skb_header_pointer in amanda conntrack helper
#   
#   Fixes broken packets, noticed by Matthias Andree <matthias.andree@gmx.de>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
# net/ipv4/netfilter/ip_conntrack_amanda.c
#   2004/11/04 22:50:04+01:00 kaber@coreworks.de +5 -7
#   [NETFILTER]: Don't use skb_header_pointer in amanda conntrack helper
#   
#   Fixes broken packets, noticed by Matthias Andree <matthias.andree@gmx.de>
#   
#   Signed-off-by: Patrick McHardy <kaber@trash.net>
# 
diff -Nru a/net/ipv4/netfilter/ip_conntrack_amanda.c b/net/ipv4/netfilter/ip_conntrack_amanda.c
--- a/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-04 22:50:37 +01:00
+++ b/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-04 22:50:37 +01:00
@@ -49,7 +49,7 @@
 {
 	struct ip_conntrack_expect *exp;
 	struct ip_ct_amanda_expect *exp_amanda_info;
-	char *amp, *data, *data_limit, *tmp;
+	char *data, *data_limit, *tmp;
 	unsigned int dataoff, i;
 	u_int16_t port, len;
 
@@ -70,11 +70,9 @@
 	}
 
 	LOCK_BH(&amanda_buffer_lock);
-	amp = skb_header_pointer(skb, dataoff,
-				 skb->len - dataoff, amanda_buffer);
-	BUG_ON(amp == NULL);
-	data = amp;
-	data_limit = amp + skb->len - dataoff;
+	skb_copy_bits(skb, dataoff, amanda_buffer, skb->len - dataoff);
+	data = amanda_buffer;
+	data_limit = amanda_buffer + skb->len - dataoff;
 	*data_limit = '\0';
 
 	/* Search for the CONNECT string */
@@ -110,7 +108,7 @@
 		exp->mask.dst.u.tcp.port = 0xFFFF;
 
 		exp_amanda_info = &exp->help.exp_amanda_info;
-		exp_amanda_info->offset = tmp - amp;
+		exp_amanda_info->offset = tmp - amanda_buffer;
 		exp_amanda_info->port   = port;
 		exp_amanda_info->len    = len;
 

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Patrick McHardy]

Matthias Andree wrote:

>On Thu, 04 Nov 2004, Patrick McHardy wrote:
>
>
>>-	data = amp;
>>-	data_limit = amp + skb->len - dataoff;
>>+	skb_copy_bits(skb, dataoff, amanda_buffer, skb->len - dataoff);
>>+	data = amanda_buffer;
>>+	data_limit = amanda_buffer + skb->len - dataoff;
>>
>
>Does this mean the whole buffer is still copied?
>
Yes.

>
>If so: Making a local copy of the packet just to be able to stuff NUL
>bytes to suit or "optimize" strstr functions is plain nonsense - amanda
>pipes several GByte through the kernel at each run, and copying
>gazillions of bits around, wasting millions of CPU cycles, just because
>someone is too lazy to spell a more decent search function, is
>bad design.
>
This is just the UDP control connection, the data is not copied
or scanned. Feel free to send a patch that doesn't need to
copy linear skbs and doesn't need to modify the skb.

>Same consideration applies to FTP connection tracking.
>
>I wrote a memstr function for bogofilter (GPL v2) that we could use
>inside the kernel, as a length-limited strstr replacement, as in "search
>the first buffer_size bytes starting with buffer_base for the first
>occurrence of const char *needle". That avoids all buffer modifications
>in ip_conntrack_amanda.c AFAICS. It's also slow because it does a linear
>search and not an optimized search as the sophisticated KMP and other
>search algorithms would be able to do, but then again the generic strstr
>inside the kernel is linear, too.
>
non-linear skb have fragments scattered in memory, you have to
copy them or scan with a function that is aware of how the data
is layed out in memory. Look at Harald's notes from the netfilter
workshop for details on current work in this area.

http://www.netfilter.org/documentation/conferences/nf-workshop-2004-summary.html#AEN499

Regards
Patrick

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[David S. Miller]

On Fri, 05 Nov 2004 00:53:22 +0100
Patrick McHardy <kaber@trash.net> wrote:

> Your observation and your patch were correct, thanks. It is supposed
> to be just a copy, I missed that it wasn't anymore. While your patch
> works too, and is even faster with non-linear skbs, I don't like the
> idea of using the skb as a scratch-area, so I sent this patch to Dave
> instead.

His patch isn't correct, even making a temporary change to
a shared SKB is illegal.  Things like tcpdump could see
corrupt SKB contents if they look during that tiny window
when the newline character has been changed to NULL by
the amanda conntrack module.

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Patrick McHardy]

David S. Miller wrote:

>On Fri, 05 Nov 2004 00:53:22 +0100
>Patrick McHardy <kaber@trash.net> wrote:
>
>  
>
>>Your observation and your patch were correct, thanks. It is supposed
>>to be just a copy, I missed that it wasn't anymore. While your patch
>>works too, and is even faster with non-linear skbs, I don't like the
>>idea of using the skb as a scratch-area, so I sent this patch to Dave
>>instead.
>>    
>>
>
>His patch isn't correct, even making a temporary change to
>a shared SKB is illegal.  Things like tcpdump could see
>corrupt SKB contents if they look during that tiny window
>when the newline character has been changed to NULL by
>the amanda conntrack module.
>  
>

True, I'm stupid sometimes :)

Regards
Patrick

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[David S. Miller]

On Fri, 5 Nov 2004 02:04:27 +0100
Matthias Andree <matthias.andree@gmx.de> wrote:

> On Thu, 04 Nov 2004, David S. Miller wrote:
> 
> > His patch isn't correct, even making a temporary change to
> > a shared SKB is illegal.
> 
> So the original ip_conntrack_amanda was already illegal. If only such
> nonsense caused heavy kernel logging (let it oops or GPF or whatver),
> that's a much quicker way to pinpoint the bug than run amanda with a
> special devnull configuration some dozen times.

The original ip_conntrack_amanda was correct before
my skb_header_pointer() changes.  Patrick's patch, which
I'll of course apply, simply reverted those changes back
to the original code which uses the amanda_buffer for
the UDP control stream always.

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Matthias Andree]

On Thu, 04 Nov 2004, David S. Miller wrote:

> His patch isn't correct, even making a temporary change to
> a shared SKB is illegal.

So the original ip_conntrack_amanda was already illegal. If only such
nonsense caused heavy kernel logging (let it oops or GPF or whatver),
that's a much quicker way to pinpoint the bug than run amanda with a
special devnull configuration some dozen times.

> Things like tcpdump could see corrupt SKB contents if they look during
> that tiny window when the newline character has been changed to NULL
> by the amanda conntrack module.

Where is the SKB stuff documented?

-- 
Matthias Andree

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Matthias Andree]

On Thu, 04 Nov 2004, David S. Miller wrote:

> The original ip_conntrack_amanda was correct before
> my skb_header_pointer() changes.  Patrick's patch, which
> I'll of course apply, simply reverted those changes back
> to the original code which uses the amanda_buffer for
> the UDP control stream always.

OK, thanks to both of you for handling this.

One question remains though, where is the SKB stuff documented?  "make
htmldocs" didn't elucidate me, neither did "grep -r skb_header_pointer
Documentation"

-- 
Matthias Andree

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Pablo Neira]

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

Patrick McHardy wrote:

> Matthias Andree wrote:
>
>> On Thu, 04 Nov 2004, Patrick McHardy wrote:
>>
>>> The data that is changed is only a copy, the actual packet is not 
>>> touched.
>>>   
>>
>>
>> Why then does the application not see the packets as long as
>> ip_conntrack_amanda is loaded and starts seeing them again as soon as
>> "rmmod ip_conntrack_amanda" has completed?
>>  
>>
>
> Your observation and your patch were correct, thanks. It is supposed
> to be just a copy, I missed that it wasn't anymore. While your patch
> works too, and is even faster with non-linear skbs, I don't like the
> idea of using the skb as a scratch-area, so I sent this patch to Dave
> instead.


Patrick, what about this? this way we save a copy to a buffer for linear 
skbs.

Signed-off-by: Pablo Neira Ayuso <pablo@eurodev.net>

[-- Attachment #2: xxx --]
[-- Type: text/plain, Size: 1029 bytes --]

===== net/ipv4/netfilter/ip_conntrack_amanda.c 1.10 vs edited =====
--- 1.10/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-08-19 02:14:53 +02:00
+++ edited/net/ipv4/netfilter/ip_conntrack_amanda.c	2004-11-05 17:32:04 +01:00
@@ -49,9 +49,10 @@
 {
 	struct ip_conntrack_expect *exp;
 	struct ip_ct_amanda_expect *exp_amanda_info;
-	char *amp, *data, *data_limit, *tmp;
+	char *amp, *data, *tmp;
 	unsigned int dataoff, i;
 	u_int16_t port, len;
+	int found = 0;
 
 	/* Only look at packets from the Amanda server */
 	if (CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL)
@@ -74,12 +75,17 @@
 				 skb->len - dataoff, amanda_buffer);
 	BUG_ON(amp == NULL);
 	data = amp;
-	data_limit = amp + skb->len - dataoff;
-	*data_limit = '\0';
 
 	/* Search for the CONNECT string */
-	data = strstr(data, "CONNECT ");
-	if (!data)
+	while((data = memchr(data, 'C', skb->len - dataoff)) != NULL) {
+		if (strncmp(data, "CONNECT ", 8) == 0) {
+			found = 1;
+			break;
+		}
+		data++;
+	}
+
+	if (!found)
 		goto out;
 	data += strlen("CONNECT ");
 

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Patrick McHardy]

Pablo Neira wrote:

> Patrick, what about this? this way we save a copy to a buffer for 
> linear skbs.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@eurodev.net>
>
>@@ -74,12 +75,17 @@
> 				 skb->len - dataoff, amanda_buffer);
> 	BUG_ON(amp == NULL);
> 	data = amp;
>-	data_limit = amp + skb->len - dataoff;
>-	*data_limit = '\0';
> 
> 	/* Search for the CONNECT string */
>-	data = strstr(data, "CONNECT ");
>-	if (!data)
>+	while((data = memchr(data, 'C', skb->len - dataoff)) != NULL) {
>+		if (strncmp(data, "CONNECT ", 8) == 0) {
>  
>
What if the C is the last byte ? There are also more str* commands below
that need a terminating 0-byte.

Regards
Patrick

>+			found = 1;
>+			break;
>+		}
>+		data++;
>+	}
>+
>+	if (!found)
> 		goto out;
> 	data += strlen("CONNECT ");
> 
>  
>

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Henrik Nordstrom]

On Fri, 5 Nov 2004, Pablo Neira wrote:

> Patrick, what about this? this way we save a copy to a buffer for linear 
> skbs.

You need to make sure you or any of the later string match/extract 
functions are not reading outside of the skb, after the current data 
segment.

>From what I could tell this was missing in your proposed change. If the 
helper sees a packet with a C as last byte it would read past the end of 
the skb, and without looking at the whole source I see it very likely 
there is operations on the data further down assuming a null terminated 
string..

Regards
Henrik

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Pablo Neira]

Patrick McHardy wrote:

> Pablo Neira wrote:
>
>> Patrick, what about this? this way we save a copy to a buffer for 
>> linear skbs.
>>
>> Signed-off-by: Pablo Neira Ayuso <pablo@eurodev.net>
>>
>> @@ -74,12 +75,17 @@
>>                  skb->len - dataoff, amanda_buffer);
>>     BUG_ON(amp == NULL);
>>     data = amp;
>> -    data_limit = amp + skb->len - dataoff;
>> -    *data_limit = '\0';
>>
>>     /* Search for the CONNECT string */
>> -    data = strstr(data, "CONNECT ");
>> -    if (!data)
>> +    while((data = memchr(data, 'C', skb->len - dataoff)) != NULL) {
>> +        if (strncmp(data, "CONNECT ", 8) == 0) {
>>  
>>
> What if the C is the last byte ? There are also more str* commands below
> that need a terminating 0-byte.


you both are right. Patrick's patch is the best way to fix this problem.

Pablo

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Matthias Andree]

On Fri, 05 Nov 2004, Patrick McHardy wrote:

> What if the C is the last byte ? There are also more str* commands below
> that need a terminating 0-byte.

IMNSHO, str* functions have no business in arbitrary data. I have a
simple memstr() function in bogofilter that is GPL'd and can be imported
into the kernel, I believe:

http://cvs.sourceforge.net/viewcvs.py/*checkout*/bogofilter/bogofilter/src/memstr.c?rev=HEAD

Yes, there is room for optimization, but as long as strstr isn't
optimized, who cares.

-- 
Matthias Andree

Re: [BK PATCH] Fix ip_conntrack_amanda data corruption bug that breaks amanda dumps

[Patrick McHardy]

Matthias Andree wrote:

>IMNSHO, str* functions have no business in arbitrary data. I have a
>simple memstr() function in bogofilter that is GPL'd and can be imported
>into the kernel, I believe:
>
>  
>
It's not only strstr, it also uses simple_strtoul.
If you don't like it, send a patch.