Minor modification of function dcc_parse in ip_conntrack_irc.c

Minor modification of function dcc_parse in ip_conntrack_irc.c

[Nix N. Nix]

Hi!

The dcc_parse function in ip_conntrack_irc.c has a shortcoming:

\1DCC <command> <param> <ip> <port> [<size>]\1\n

...is the format of a DCC command (where <size> is optional).

dcc_parse is responsible for finding the ip and port in this string.  It
gets the string starting at <param>.  Unfortunately, so far, dcc_parse
assumed that <param> does not contain any spaces.  Thus, the module
failed to track connections implied by a string of the form

\1DCC SEND "The Big Movie.avi" 10289477 1011 37546262\1\n

I have remedied this problem by having it check whether there's a double
quote and, if so, loop on to the corresponding closing double quote
before resuming normal processing:

--- ip_conntrack_irc.c  2003-11-28 13:26:21.000000000 -0500
+++ mod.ip_conntrack_irc.c      2004-08-10 16:07:49.000000000 -0400
@@ -83,6 +83,14 @@
        ad_end_p        returns pointer to last byte of addr data */
 {
  
+       /* file names with spaces in them are double-quoted */
+       if ('"' == *data && data <= data_end - 12) {
+               *data++ ;
+               while (*data++ != '"')
+                       if (data > data_end - 12)
+                               return -1;
+       }
+
        /* at least 12: "AAAAAAAA P\1\n" */
        while (*data++ != ' ')
                if (data > data_end - 12)

This fixes the problem with spaces in file names, and does not affect
DCC CHAT, because, in those cases, <param> always= chat .

I have tested this in kernel vanilla 2.4.24.

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Patrick McHardy]

Nix N. Nix wrote:

>Hi!
>
>The dcc_parse function in ip_conntrack_irc.c has a shortcoming:
>
>\1DCC <command> <param> <ip> <port> [<size>]\1\n
>
>...is the format of a DCC command (where <size> is optional).
>
>dcc_parse is responsible for finding the ip and port in this string.  It
>gets the string starting at <param>.  Unfortunately, so far, dcc_parse
>assumed that <param> does not contain any spaces.  Thus, the module
>failed to track connections implied by a string of the form
>
>\1DCC SEND "The Big Movie.avi" 10289477 1011 37546262\1\n
>
>I have remedied this problem by having it check whether there's a double
>quote and, if so, loop on to the corresponding closing double quote
>before resuming normal processing:
>  
>
How do filenames which contain a quotation mark look ? I assume the 
quotation
marks is escaped ? We should also handle this case.

Regards
Patrick

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Nix N. Nix]

On Wed, 2004-08-11 at 09:29, Patrick McHardy wrote:
> >\1DCC <command> <param> <ip> <port> [<size>]\1\n
> >
> >...is the format of a DCC command (where <size> is optional).
> >
> >dcc_parse is responsible for finding the ip and port in this string.  It
> >gets the string starting at <param>.  Unfortunately, so far, dcc_parse
> >assumed that <param> does not contain any spaces.  Thus, the module
> >failed to track connections implied by a string of the form
> >
> >\1DCC SEND "The Big Movie.avi" 10289477 1011 37546262\1\n
> >
> How do filenames which contain a quotation mark look ? I assume the 
> quotation
> marks is escaped ? We should also handle this case.

It turns out that quotes are sent unescaped or otherwise encoded.  Thus,
the transfer of a file named |The Big" Movie.avi| results in a DCC
string of the form

\1DCC SEND "The Big" Movie.avi" 10289477 1011 37546262\1\n

As a result, my previously proposed patch fails.  However, I have found
what I think is another useable rationale:

If <command> starts with a quote then the string is guaranteed to
describe a DCC file transfer and, since all strings indicating DCC file
transfers end in 3 numeric parameters (<ip> <port> <filesize>), we need
only find the beginning of the third-last parameter.

In all other cases, we can skip over the code embodying the above
rationale, directly to the existing code.  Thus, the patch:

--- orig.ip_conntrack_irc.c	2003-11-28 13:26:21.000000000 -0500
+++ ip_conntrack_irc.c	2004-08-13 18:13:20.000000000 -0400
@@ -82,6 +82,24 @@
 	ad_beg_p	returns pointer to first byte of addr data
 	ad_end_p	returns pointer to last byte of addr data */
 {
+	/* if the DCC command parameter starts with a quote then this is
+       a DCC file operation, which means it ends in 3 numeric
+       parameters: 
+       <ip> <port> <filesize> */
+	if ('"' == *data && data <= data_end - 12) {
+		char *third_last_p = data, 
+			 *second_last_p = data, 
+			 *last_p = data, *Nix;
+
+        for (Nix = data ; Nix < data_end - 1 ; Nix++) {
+        	if (' ' == *Nix) {
+                third_last_p = second_last_p ;
+                second_last_p = last_p ;
+                last_p = Nix + 1 ;
+			}
+		}
+		data = third_last_p - 1 ;
+	}
 
 	/* at least 12: "AAAAAAAA P\1\n" */
 	while (*data++ != ' ')

Please let me know what you think of it.

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Harald Welte]

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

On Fri, Aug 13, 2004 at 06:31:23PM -0400, Nix N. Nix wrote:
> On Wed, 2004-08-11 at 09:29, Patrick McHardy wrote:
> > >\1DCC <command> <param> <ip> <port> [<size>]\1\n
> > >
> > >...is the format of a DCC command (where <size> is optional).
> > >
> > >dcc_parse is responsible for finding the ip and port in this string.  It
> > >gets the string starting at <param>.  Unfortunately, so far, dcc_parse
> > >assumed that <param> does not contain any spaces.  Thus, the module
> > >failed to track connections implied by a string of the form
> > >
> > >\1DCC SEND "The Big Movie.avi" 10289477 1011 37546262\1\n
> > >
> > How do filenames which contain a quotation mark look ? I assume the 
> > quotation
> > marks is escaped ? We should also handle this case.
> 
> It turns out that quotes are sent unescaped or otherwise encoded.  Thus,
> the transfer of a file named |The Big" Movie.avi| results in a DCC
> string of the form

Which IRC clients did you test this with?  If we put a change in
ip_conntrack_irc, we should make sure that it suports all major clients
in both linux and windows world.

> As a result, my previously proposed patch fails.  However, I have found
> what I think is another useable rationale:

Mh. I don't like this heuristics and I'd rather rewrite that portion of
the IRC parser in order to be more intelligent.  We have parsed the
command just before, so we know whether it is a SEND or CHAT or watever.

So we should make this whole quotation handlign conditional to the SEND
operation.

btw: Your patch didn't declare 'Nix'.  Did you actually try to run this?

-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Nix N. Nix]

On Thu, 2004-08-19 at 07:35, Harald Welte wrote:
> On Fri, Aug 13, 2004 at 06:31:23PM -0400, Nix N. Nix wrote:
> > On Wed, 2004-08-11 at 09:29, Patrick McHardy wrote:
> > > >\1DCC <command> <param> <ip> <port> [<size>]\1\n
> > > >
> > > >...is the format of a DCC command (where <size> is optional).
> > > >
> > > >\1DCC SEND "The Big Movie.avi" 10289477 1011 37546262\1\n
> > > >
> > It turns out that quotes are sent unescaped or otherwise encoded.  Thus,
> > the transfer of a file named |The Big" Movie.avi| results in a DCC
> > string of the form
> 
> Which IRC clients did you test this with?  If we put a change in
> ip_conntrack_irc, we should make sure that it suports all major clients
> in both linux and windows world.

I have tested this with irssi and xchat.  Also, since this is strictly a
refinement on the existing handling, it should work at least as well as
the unmodified code.

> Mh. I don't like this heuristics and I'd rather rewrite that portion of
> the IRC parser in order to be more intelligent.  We have parsed the
> command just before, so we know whether it is a SEND or CHAT or watever.
> 
> So we should make this whole quotation handlign conditional to the SEND
> operation.

Consider the following (slightly more involved) alternative to my
previous patch:

--- orig.ip_conntrack_irc.c	2004-08-22 17:51:00.070119021 -0400
+++ ip_conntrack_irc.c	2004-08-22 17:59:05.692492504 -0400
@@ -64,17 +64,33 @@
 #define DEBUGP(format, args...)
 #endif
 
-int parse_dcc(char *data, char *data_end, u_int32_t * ip, u_int16_t *
port,
-	      char **ad_beg_p, char **ad_end_p)
+int parse_dcc(int dcc_proto_idx, char *data, char *data_end, u_int32_t
* ip,
+	      u_int16_t * port, char **ad_beg_p, char **ad_end_p)
 /* tries to get the ip_addr and port out of a dcc command
    return value: -1 on failure, 0 on success 
-	data		pointer to first byte of DCC command data
-	data_end	pointer to last byte of dcc command data
-	ip		returns parsed ip of dcc command
-	port		returns parsed port of dcc command
-	ad_beg_p	returns pointer to first byte of addr data
-	ad_end_p	returns pointer to last byte of addr data */
+    dcc_proto_idx	index into the array "dccprotos"
+	data			pointer to first byte of DCC command data
+	data_end		pointer to last byte of dcc command data
+	ip				returns parsed ip of dcc command
+	port			returns parsed port of dcc command
+	ad_beg_p		returns pointer to first byte of addr data
+	ad_end_p		returns pointer to last byte of addr data */
 {
+	/* if this is a DCC SEND, then it ends in 3 numeric parameters: <ip>
<port> <filesize> */
+	if (0 == dcc_proto_idx && '"' == *data && data <= data_end - 12) {
+		char *third_last_p = data, 
+			 *second_last_p = data, 
+			 *last_p = data, *Nix;
+
+        for (Nix = data ; Nix < data_end - 1 ; Nix++) {
+        	if (' ' == *Nix) {
+                third_last_p = second_last_p ;
+                second_last_p = last_p ;
+                last_p = Nix + 1 ;
+			}
+		}
+		data = third_last_p - 1 ;
+	}
 
 	/* at least 12: "AAAAAAAA P\1\n" */
 	while (*data++ != ' ')
@@ -166,7 +182,7 @@
 			/* we have at least 
 			 * (19+MINMATCHLEN)-5-dccprotos[i].matchlen bytes valid
 			 * data left (== 14/13 bytes) */
-			if (parse_dcc((char *)data, data_limit, &dcc_ip,
+			if (parse_dcc(i, (char *)data, data_limit, &dcc_ip,
 				       &dcc_port, &addr_beg_p, &addr_end_p)) {
 				/* unable to parse */
 				DEBUGP("unable to parse dcc command\n");
> 
The function calling parse_dcc first computes an index into an array
consisting of DCC <command>s.  This index can be passed to parse_dcc. 
This will restrict my parse_dcc modification to DCC SENDs without making
use of my previous heuristic.

> btw: Your patch didn't declare 'Nix'.  Did you actually try to run this?
I /did/ declare Nix, and I am currently running this code.  Please have
another look at the statement that reads char *third_last_p ...

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Nix N. Nix]

So ... uuuhh ... what is to become of this patch ?

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Nix N. Nix]

On Thu, 2004-08-19 at 07:35, Harald Welte wrote:
> Which IRC clients did you test this with?  If we put a change in
> ip_conntrack_irc, we should make sure that it suports all major clients
> in both linux and windows world.

I just realized that, since I run an fserve behind the firewall
containing the modified ip_conntrack_irc code (the failure of the fserve
to transmit files prompted me to look into the problem in the first
place), a multitude of IRC clients must have been able to grab files
from my fserve (since I have already completed quite a few uploads). 
All the files I serve contain spaces, and one of them even has a double
quote.

The fserve is fserve.pl an irssi script.

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Nix N. Nix]

On Wed, 2004-08-11 at 09:29, Patrick McHardy wrote:
> How do filenames which contain a quotation mark look ? I assume the 
> quotation
> marks is escaped ? We should also handle this case.

I submitted another patch which addresses this issue in a previous
message a few days ago, however, I have received no feedback on it. 
Could somebody with a few minutes to spare please look it over ?

I'm fairly new to this list, so I'm not familiar with the process of
submitting patches, however, I would like this fix to eventually make it
into ip_conntrack_irc.c.

Please let me know what you think !

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Harald Welte]

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

On Tue, Aug 17, 2004 at 04:34:03PM -0400, Nix N. Nix wrote:
> On Wed, 2004-08-11 at 09:29, Patrick McHardy wrote:
> > How do filenames which contain a quotation mark look ? I assume the 
> > quotation
> > marks is escaped ? We should also handle this case.
> 
> I submitted another patch which addresses this issue in a previous
> message a few days ago, however, I have received no feedback on it. 
> Could somebody with a few minutes to spare please look it over ?

Those few minutes are exactly the problem.  I'm currently sitting about
14-16h per day in front of my workstation, but still it sometimes takes
a couple fo days until i catch up with netfilter-devel.

> I'm fairly new to this list, so I'm not familiar with the process of
> submitting patches, however, I would like this fix to eventually make it
> into ip_conntrack_irc.c.

Yes, the process is posting patches to this list.
-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: Minor modification of function dcc_parse in ip_conntrack_irc.c

[Harald Welte]

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

On Tue, Aug 10, 2004 at 04:34:41PM -0400, Nix N. Nix wrote:

> dcc_parse is responsible for finding the ip and port in this string.  It
> gets the string starting at <param>.  Unfortunately, so far, dcc_parse
> assumed that <param> does not contain any spaces.  Thus, the module
> failed to track connections implied by a string of the form
> 
> \1DCC SEND "The Big Movie.avi" 10289477 1011 37546262\1\n

I think I came across this issue once before.  I somewhere found a
document specifying the dcc protocol, and IIRC it stated that spaces are
not allowed.

So my take was always: well, fix the broken irc clients.

However, I think we should integrate a fix.  (see my other mail)
-- 
- Harald Welte <laforge@netfilter.org>             http://www.netfilter.org/
============================================================================
  "Fragmentation is like classful addressing -- an interesting early
   architectural error that shows how much experimentation was going
   on while IP was being designed."                    -- Paul Vixie

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]