Skip to contentSkip to conversation

conntrack ftp helper modification to use new infrastrure

PATCH In review Pablo Neira 3 participants 5 messages Jan 9, 2005 – Feb 1, 2005

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 4/4 conntrack ftp helper modification to use new infrastrure

Attached the modifications for the ftp helper to use the functions provided with the new infrastructure.

Two important notes:

* I must confess that I don't see the point of using the new infrastructure at all for the ftp helper (no way, I didn't drive nuts, not yet :o) but I see that it could be useful for the amanda helper which does a strstr search. So, please consider this just an example, read and drop it to /dev/null.
* This doesn't apply on top of lastest rusty's helper simplifications.

--
Pablo

===== net/ipv4/netfilter/ip_conntrack_ftp.c 1.24 vs edited =====

net/ipv4/netfilter/ip_conntrack_ftp.c +43 -24
@@ -20,17 +20,17 @@
 #include <linux/netfilter_ipv4/ip_conntrack_helper.h>
 #include <linux/netfilter_ipv4/ip_conntrack_ftp.h>
 #include <linux/moduleparam.h>
+#include <linux/netfilter_ipv4/nf_string_match.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Rusty Russell <[email protected]>");
 MODULE_DESCRIPTION("ftp connection tracking helper");
 
-/* This is slow, but it's simple. --RR */
-static char ftp_buffer[65536];
-
 static DECLARE_LOCK(ip_ftp_lock);
 struct module *ip_conntrack_ftp = THIS_MODULE;
 
+#define FTP_BUFSIZE 64
+
 #define MAX_PORTS 8
 static int ports[MAX_PORTS];
 static int ports_c;
@@ -56,30 +56,35 @@
 	char skip;
 	char term;
 	enum ip_ct_ftp_type ftptype;
+	struct nf_string_match *sm;
 	int (*getnum)(const char *, size_t, u_int32_t[], char);
 } search[] = {
 	{
 		IP_CT_DIR_ORIGINAL,
 		"PORT",	sizeof("PORT") - 1, ' ', '\r',
 		IP_CT_FTP_PORT,
+		NULL,
 		try_rfc959,
 	},
 	{
 		IP_CT_DIR_REPLY,
 		"227 ",	sizeof("227 ") - 1, '(', ')',
 		IP_CT_FTP_PASV,
+		NULL,
 		try_rfc959,
 	},
 	{
 		IP_CT_DIR_ORIGINAL,
 		"EPRT", sizeof("EPRT") - 1, ' ', '\r',
 		IP_CT_FTP_EPRT,
+		NULL,
 		try_eprt,
 	},
 	{
 		IP_CT_DIR_REPLY,
 		"229 ", sizeof("229 ") - 1, '(', ')',
 		IP_CT_FTP_EPSV,
+		NULL,
 		try_epsv_response,
 	},
 };
@@ -188,15 +193,19 @@
 }
 
 /* Return 1 for match, 0 for accept, -1 for partial. */
-static int find_pattern(const char *data, size_t dlen,
+static int find_pattern(const struct sk_buff *skb, size_t dlen,
 			const char *pattern, size_t plen,
 			char skip, char term,
 			unsigned int *numoff,
 			unsigned int *numlen,
 			u_int32_t array[6],
+			struct nf_string_match *sm,
 			int (*getnum)(const char *, size_t, u_int32_t[], char))
 {
-	size_t i;
+	size_t i, size;
+	unsigned char *data = NULL;
+	unsigned int offset = 0;
+	unsigned char __data[FTP_BUFSIZE];
 
 	DEBUGP("find_pattern `%s': dlen = %u\n", pattern, dlen);
 	if (dlen == 0)
@@ -209,33 +218,31 @@
 		else return 0;
 	}
 
-	if (strnicmp(data, pattern, plen) != 0) {
-#if 0
-		size_t i;
-
-		DEBUGP("ftp: string mismatch\n");
-		for (i = 0; i < plen; i++) {
-			DEBUGP("ftp:char %u `%c'(%u) vs `%c'(%u)\n",
-				i, data[i], data[i],
-				pattern[i], pattern[i]);
-		}
-#endif
+	if (!nf_string_match_search(skb, sm, &offset))
 		return 0;
-	}
 
+	if ((size = (skb->len - offset - 1)) > (FTP_BUFSIZE - 1))
+		size = FTP_BUFSIZE - 1;
+			
+	data = skb_header_pointer(skb, offset, size, __data);
+	BUG_ON(data == NULL);
+	
 	DEBUGP("Pattern matches!\n");
 	/* Now we've found the constant string, try to skip
 	   to the 'skip' character */
-	for (i = plen; data[i] != skip; i++)
+	for (i = plen; i < size && data[i] != skip; i++)
 		if (i == dlen - 1) return -1;
 
 	/* Skip over the last character */
 	i++;
 
+	if (i >= size)
+		return -1;
+
 	DEBUGP("Skipped up to `%c'!\n", skip);
 
 	*numoff = i;
-	*numlen = getnum(data + i, dlen - i, array, term);
+	*numlen = getnum(data + i, size - i, array, term);
 	if (!*numlen)
 		return -1;
 
@@ -249,7 +256,7 @@
 {
 	unsigned int dataoff, datalen;
 	struct tcphdr _tcph, *th;
-	char *fb_ptr;
+	char *fb_ptr, __fb_ptr;
 	u_int32_t old_seq_aft_nl;
 	int old_seq_aft_nl_set, ret;
 	u_int32_t array[6] = { 0 };
@@ -283,15 +290,14 @@
 	datalen = skb->len - dataoff;
 
 	LOCK_BH(&ip_ftp_lock);
-	fb_ptr = skb_header_pointer(skb, dataoff,
-				    skb->len - dataoff, ftp_buffer);
+	fb_ptr = skb_header_pointer(skb, skb->len - 1, 1, &__fb_ptr);
 	BUG_ON(fb_ptr == NULL);
 
 	old_seq_aft_nl_set = ct_ftp_info->seq_aft_nl_set[dir];
 	old_seq_aft_nl = ct_ftp_info->seq_aft_nl[dir];
 
 	DEBUGP("conntrack_ftp: datalen %u\n", datalen);
-	if (fb_ptr[datalen - 1] == '\n') {
+	if (*fb_ptr == '\n') {
 		DEBUGP("conntrack_ftp: datalen %u ends in \\n\n", datalen);
 		if (!old_seq_aft_nl_set
 		    || after(ntohl(th->seq) + datalen, old_seq_aft_nl)) {
@@ -321,13 +327,14 @@
 	for (i = 0; i < ARRAY_SIZE(search); i++) {
 		if (search[i].dir != dir) continue;
 
-		found = find_pattern(fb_ptr, skb->len - dataoff,
+		found = find_pattern(skb, skb->len - dataoff,
 				     search[i].pattern,
 				     search[i].plen,
 				     search[i].skip,
 				     search[i].term,
 				     &matchoff, &matchlen,
 				     array,
+				     search[i].sm,
 				     search[i].getnum);
 		if (found) break;
 	}
@@ -419,6 +426,8 @@
 				ports[i]);
 		ip_conntrack_helper_unregister(&ftp[i]);
 	}
+	for (i = 0; i < 4; i++)
+		nf_string_match_destroy(search[i].sm);
 }
 
 static int __init init(void)
@@ -428,6 +437,16 @@
 
 	if (ports_c == 0)
 		ports[ports_c++] = FTP_PORT;
+
+	for (i = 0; i < 4; i++) {
+		search[i].sm = nf_string_match_create(search[i].pattern,
+						      search[i].plen);
+		if (!search[i].sm) {
+			int k;
+			for (k = i - 1; k > 0; k--)
+				nf_string_match_destroy(search[k].sm);
+		}
+	}
 
 	for (i = 0; i < ports_c; i++) {
 		ftp[i].tuple.src.u.tcp.port = htons(ports[i]);
#1 lore raw
Quoted 37 lines · Pablo Neira

Attached the modifications for the ftp helper to use the functions provided with the new infrastructure.

Two important notes:

* I must confess that I don't see the point of using the new infrastructure at all for the ftp helper (no way, I didn't drive nuts, not yet :o) but I see that it could be useful for the amanda helper which does a strstr search. So, please consider this just an example, read and drop it to /dev/null.
* This doesn't apply on top of lastest rusty's helper simplifications.

Signature · 5 lines
Pablo

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

===== net/ipv4/netfilter/ip_conntrack_ftp.c 1.24 vs edited =====
net/ipv4/netfilter/ip_conntrack_ftp.c +3 -3
@@ -20,17 +20,17 @@
 #include <linux/netfilter_ipv4/ip_conntrack_helper.h>
 #include <linux/netfilter_ipv4/ip_conntrack_ftp.h>
 #include <linux/moduleparam.h>
+#include <linux/netfilter_ipv4/nf_string_match.h>
 
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Rusty Russell <[email protected]>");
 MODULE_DESCRIPTION("ftp connection tracking helper");
 
-/* This is slow, but it's simple. --RR */
-static char ftp_buffer[65536];
-
 static DECLARE_LOCK(ip_ftp_lock);
 struct module *ip_conntrack_ftp = THIS_MODULE;
 
+#define FTP_BUFSIZE 64
+
 #define MAX_PORTS 8
 static int ports[MAX_PORTS];
 static int ports_c;

net/ipv4/netfilter/ip_conntrack_ftp.c line 61

Quoted 6 lines
net/ipv4/netfilter/ip_conntrack_ftp.c +1 -0
@@ -56,30 +56,35 @@
 	char skip;
 	char term;
 	enum ip_ct_ftp_type ftptype;
+	struct nf_string_match *sm;
 
 

Why is is part of struct ftpsearch ? One struct nf_string_match per helper should be enough as long as we take the global lock.

Regards
Patrick

#2 lore raw
Quoted 5 lines · Patrick McHardy
Quoted 5 lines

#define MAX_PORTS 8
static int ports[MAX_PORTS];
static int ports_c;

net/ipv4/netfilter/ip_conntrack_ftp.c +0 -0
@@ -56,30 +56,35 @@
    char skip;
Quoted 8 lines

net/ipv4/netfilter/ip_conntrack_ftp.c

Quoted 5 lines
net/ipv4/netfilter/ip_conntrack_ftp.c +1 -0
    char term;
    enum ip_ct_ftp_type ftptype;
+    struct nf_string_match *sm;
 
 

Why is is part of struct ftpsearch ? One struct nf_string_match per helper should be enough as long as we take the global lock.

because the ftp helper looks for four different patterns. The goodshift and badshift arrays required by the boyer-moore algorithm are different for every pattern.

Actually, I started a version using lists of nf_string_match to look for a set of patterns iteratively but I finally decided to simplify the thing. If someone finds this feature interesting I could recover it in future.

--
Pablo

#3 lore raw
Quoted 7 lines · Pablo Neira
Quoted 2 lines · Patrick McHardy

Why is is part of struct ftpsearch ? One struct nf_string_match per helper should be enough as long as we take the global lock.

because the ftp helper looks for four different patterns. The goodshift and badshift arrays required by the boyer-moore algorithm are different for every pattern.

Thanks for the explanation.

Quoted 2 lines

Actually, I started a version using lists of nf_string_match to look for a set of patterns iteratively but I finally decided to simplify the thing. If someone finds this feature interesting I could recover it in future.

Since the FTP helper is currently the only possible user of such a feature I think the current way is fine.

Regards
Patrick

#4 lore raw
Quoted 7 lines · Patrick McHardy
Quoted 4 lines

Actually, I started a version using lists of nf_string_match to look for a set of patterns iteratively but I finally decided to simplify the thing. If someone finds this feature interesting I could recover it in future.

Since the FTP helper is currently the only possible user of such a feature I think the current way is fine.

It's not as rare as you might think. Even the IRC helper has to look for a list of different patterns...

Quoted 2 lines

Regards
Patrick

Signature · 5 lines
- Harald Welte <[email protected]>             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