[PATCH] netfilter: Implement RFC 1123 for FTP conntrack

[PATCH] netfilter: Implement RFC 1123 for FTP conntrack

[Jeff Mahoney]

 The FTP conntrack code currently only accepts the following format for
 the 227 response for PASV:
 227 Entering Passive Mode (148,100,81,40,31,161).

 It doesn't accept the following format from an obscure server:
 227 Data transfer will passively listen to 67,218,99,134,50,144

 From RFC 1123:
 The format of the 227 reply to a PASV command is not
 well standardized.  In particular, an FTP client cannot
 assume that the parentheses shown on page 40 of RFC-959
 will be present (and in fact, Figure 3 on page 43 omits
 them).  Therefore, a User-FTP program that interprets
 the PASV reply must scan the reply for the first digit
 of the host and port numbers.

 This patch adds support for the RFC 1123 clarification by:
 - Allowing a search filter to specify NUL as the terminator so that
   try_number will return successfully if the array of numbers has been
   filled when an unexpected character is encountered.
 - Using space as the separator for the 227 reply and then scanning for
   the first digit of the number sequence. The number sequence is parsed
   out using the existing try_rfc959 but with a NUL terminator.

 Tracked in: https://bugzilla.novell.com/show_bug.cgi?id=466279

Reported-by: Mark Post <mpost@novell.com>
Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---
 net/netfilter/nf_conntrack_ftp.c |   73 ++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 19 deletions(-)

--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -53,10 +53,14 @@ unsigned int (*nf_nat_ftp_hook)(struct s
 				struct nf_conntrack_expect *exp);
 EXPORT_SYMBOL_GPL(nf_nat_ftp_hook);
 
-static int try_rfc959(const char *, size_t, struct nf_conntrack_man *, char);
-static int try_eprt(const char *, size_t, struct nf_conntrack_man *, char);
+static int try_rfc959(const char *, size_t, struct nf_conntrack_man *,
+		      char, unsigned int *);
+static int try_rfc1123(const char *, size_t, struct nf_conntrack_man *,
+		       char, unsigned int *);
+static int try_eprt(const char *, size_t, struct nf_conntrack_man *,
+		    char, unsigned int *);
 static int try_epsv_response(const char *, size_t, struct nf_conntrack_man *,
-			     char);
+			     char, unsigned int *);
 
 static struct ftp_search {
 	const char *pattern;
@@ -64,7 +68,7 @@ static struct ftp_search {
 	char skip;
 	char term;
 	enum nf_ct_ftp_type ftptype;
-	int (*getnum)(const char *, size_t, struct nf_conntrack_man *, char);
+	int (*getnum)(const char *, size_t, struct nf_conntrack_man *, char, unsigned int *);
 } search[IP_CT_DIR_MAX][2] = {
 	[IP_CT_DIR_ORIGINAL] = {
 		{
@@ -88,10 +92,8 @@ static struct ftp_search {
 		{
 			.pattern	= "227 ",
 			.plen		= sizeof("227 ") - 1,
-			.skip		= '(',
-			.term		= ')',
 			.ftptype	= NF_CT_FTP_PASV,
-			.getnum		= try_rfc959,
+			.getnum		= try_rfc1123,
 		},
 		{
 			.pattern	= "229 ",
@@ -130,8 +132,9 @@ static int try_number(const char *data,
 			i++;
 		else {
 			/* Unexpected character; true if it's the
-			   terminator and we're finished. */
-			if (*data == term && i == array_size - 1)
+			   terminator (or we don't care about one)
+			   and we're finished. */
+			if ((*data == term || !term) && i == array_size - 1)
 				return len;
 
 			pr_debug("Char %u (got %u nums) `%u' unexpected\n",
@@ -146,7 +149,8 @@ static int try_number(const char *data,
 
 /* Returns 0, or length of numbers: 192,168,1,1,5,6 */
 static int try_rfc959(const char *data, size_t dlen,
-		      struct nf_conntrack_man *cmd, char term)
+		      struct nf_conntrack_man *cmd, char term,
+		      unsigned int *offset)
 {
 	int length;
 	u_int32_t array[6];
@@ -161,6 +165,33 @@ static int try_rfc959(const char *data,
 	return length;
 }
 
+/*
+ * From RFC 1123:
+ * The format of the 227 reply to a PASV command is not
+ * well standardized.  In particular, an FTP client cannot
+ * assume that the parentheses shown on page 40 of RFC-959
+ * will be present (and in fact, Figure 3 on page 43 omits
+ * them).  Therefore, a User-FTP program that interprets
+ * the PASV reply must scan the reply for the first digit
+ * of the host and port numbers.
+ */
+static int try_rfc1123(const char *data, size_t dlen,
+		       struct nf_conntrack_man *cmd, char term,
+		       unsigned int *offset)
+{
+	int i;
+	for (i = 0; i < dlen; i++)
+		if (isdigit(data[i]))
+			break;
+
+	if (i == dlen)
+		return 0;
+
+	*offset += i;
+
+	return try_rfc959(data + i, dlen - i, cmd, 0, offset);
+}
+
 /* Grab port: number up to delimiter */
 static int get_port(const char *data, int start, size_t dlen, char delim,
 		    __be16 *port)
@@ -189,7 +220,7 @@ static int get_port(const char *data, in
 
 /* Returns 0, or length of numbers: |1|132.235.1.2|6275| or |2|3ffe::1|6275| */
 static int try_eprt(const char *data, size_t dlen, struct nf_conntrack_man *cmd,
-		    char term)
+		    char term, unsigned int *offset)
 {
 	char delim;
 	int length;
@@ -237,7 +268,8 @@ static int try_eprt(const char *data, si
 
 /* Returns 0, or length of numbers: |||6446| */
 static int try_epsv_response(const char *data, size_t dlen,
-			     struct nf_conntrack_man *cmd, char term)
+			     struct nf_conntrack_man *cmd, char term,
+			     unsigned int *offset)
 {
 	char delim;
 
@@ -259,9 +291,10 @@ static int find_pattern(const char *data
 			unsigned int *numlen,
 			struct nf_conntrack_man *cmd,
 			int (*getnum)(const char *, size_t,
-				      struct nf_conntrack_man *, char))
+				      struct nf_conntrack_man *, char,
+				      unsigned int *))
 {
-	size_t i;
+	size_t i = plen;
 
 	pr_debug("find_pattern `%s': dlen = %Zu\n", pattern, dlen);
 	if (dlen == 0)
@@ -291,16 +324,18 @@ static int find_pattern(const char *data
 	pr_debug("Pattern matches!\n");
 	/* Now we've found the constant string, try to skip
 	   to the 'skip' character */
-	for (i = plen; data[i] != skip; i++)
-		if (i == dlen - 1) return -1;
+	if (skip) {
+		for (i = plen; data[i] != skip; i++)
+			if (i == dlen - 1) return -1;
 
-	/* Skip over the last character */
-	i++;
+		/* Skip over the last character */
+		i++;
+	}
 
 	pr_debug("Skipped up to `%c'!\n", skip);
 
 	*numoff = i;
-	*numlen = getnum(data + i, dlen - i, cmd, term);
+	*numlen = getnum(data + i, dlen - i, cmd, term, numoff);
 	if (!*numlen)
 		return -1;
 
-- 
Jeff Mahoney
SUSE Labs

Re: [PATCH] netfilter: Implement RFC 1123 for FTP conntrack

[Jan Engelhardt]

On Friday 2011-05-06 19:38, Jeff Mahoney wrote:

>The FTP conntrack code currently only accepts the following format for
>the 227 response for PASV:
>227 Entering Passive Mode (148,100,81,40,31,161).
>
>It doesn't accept the following format from an obscure server:
>227 Data transfer will passively listen to 67,218,99,134,50,144
>
>From RFC 1123: The format of the 227 reply to a PASV command is not 
>well standardized.  In particular, an FTP client cannot assume that the 
>parentheses shown on page 40 of RFC-959 will be present (and in fact, 
>Figure 3 on page 43 omits them).  Therefore, a User-FTP program that 
>interprets the PASV reply must scan the reply for the first digit of 
>the host and port numbers.
>
>This patch adds support for the RFC 1123 clarification[...]
>
>Tracked in: https://bugzilla.novell.com/show_bug.cgi?id=466279

"You are not authorized to access bug #466279."

Re: [PATCH] netfilter: Implement RFC 1123 for FTP conntrack

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/06/2011 01:45 PM, Jan Engelhardt wrote:
> On Friday 2011-05-06 19:38, Jeff Mahoney wrote:
> 
>> The FTP conntrack code currently only accepts the following format for
>> the 227 response for PASV:
>> 227 Entering Passive Mode (148,100,81,40,31,161).
>>
>> It doesn't accept the following format from an obscure server:
>> 227 Data transfer will passively listen to 67,218,99,134,50,144
>>
>>From RFC 1123: The format of the 227 reply to a PASV command is not 
>> well standardized.  In particular, an FTP client cannot assume that the 
>> parentheses shown on page 40 of RFC-959 will be present (and in fact, 
>> Figure 3 on page 43 omits them).  Therefore, a User-FTP program that 
>> interprets the PASV reply must scan the reply for the first digit of 
>> the host and port numbers.
>>
>> This patch adds support for the RFC 1123 clarification[...]
>>
>> Tracked in: https://bugzilla.novell.com/show_bug.cgi?id=466279
> 
> "You are not authorized to access bug #466279."

Hrm. I'm afraid I can't do anything about that. I don't have permissions
to open it up. It's mostly just clarification.

The summary is just that the second 227 response above doesn't get
properly handled without the patch and does with it. I can re-submit
without the reference if needed.

- -Jeff

- -- 
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.17 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org/

iEYEARECAAYFAk3ENO4ACgkQLPWxlyuTD7LjrQCfcncHTbLxovQ2UqXfxJ1EiJAQ
BlUAnikJGLCGOrrzeP1ryohuovFvFT/m
=9xWX
-----END PGP SIGNATURE-----