ftp security patch
From kadlec@blackhole.kfki.hu Mon Sep 17 10:50:38 2001
Date: Thu, 19 Apr 2001 09:16:59 +0200 (CEST)
From: Jozsef Kadlecsik
To: Kis-Szabo Andras
Cc: netfilter-devel@lists.samba.org
Subject: Re: [PATCH] was re: Security weakness in ip_conntrack_ftp
On Wed, 18 Apr 2001, Kis-Szabo Andras wrote:
> > Here follows a patch which implements a stronger FTP state tracking:
> > - PORT command is ignored if the server doesn't accept it
> > - PASV response is ignored if an explicit PASV doesn't precede it
Sorry, my patch didn't handle properly the resent requests.
Below you can find the fixed version.
> The '\r' as line-delimiter! :) (Where the other SPF engines
> sucked last year...)
> [The line-end is not at the end of the packet. It's at the '\r'('\r\n')
> delimiter. - The port checks this at the end, but before? And it is
> relevant at PASV-227 response.]
I don't see why it would be a problem. What the code accept is exactly the
(two) following possible sessions:
a. PORT
client: PORT ..... \r(\n)
server: 200 ....
b. PASV
client: PASV\r(\n)
server: 227 ...\r(\n)
What you are describing is the famous pasv vulnerability, in which case
the session is something like:
client: command ...227....\r\n
server: errorcode ....227...\r\n
and the MTU is forced to be small enough so that the server responses
in two packets:
server: errorcode ...
server: 227...\r\n
The current code opens (expects) the specified port only if
- client send the proper request
- server sends the proper answer.
The possibility of unusual small MTU values is (deliberately) not handled.
We don't try to reconstruct the FTP session from the packet flow for such
tiny packets.
Let's see the possible attacks:
client: small MTU is set
client: command ...227...\r\n
ignored, don't even check the server response
client: small MTU is set
client: command ...227...
client: PASV\r\n
server: errorcode ...
ignored, server doesn't send the proper answer.
Other possibilities?
Regards,
Jozsef
-
E-mail : kadlec@blackhole.kfki.hu, kadlec@sunserv.kfki.hu
WWW-Home: http://www.kfki.hu/~kadlec
Address : KFKI Research Institute for Particle and Nuclear Physics
H-1525 Budapest 114, POB. 49, Hungary
--- linux-2.4.0.base/include/linux/netfilter_ipv4/ip_conntrack_ftp.h Mon Dec 11 22:31:23 2000
+++ linux-2.4.0/include/linux/netfilter_ipv4/ip_conntrack_ftp.h Wed Apr 18 23:03:58 2001
@@ -11,12 +11,15 @@
/* Protects ftp part of conntracks */
DECLARE_LOCK_EXTERN(ip_ftp_lock);
-enum ip_ct_ftp_type
+enum ip_ct_ftp_state
{
+ FTP_STATE_INVALID,
/* PORT command from client */
- IP_CT_FTP_PORT = IP_CT_DIR_ORIGINAL,
- /* PASV response from server */
- IP_CT_FTP_PASV = IP_CT_DIR_REPLY
+ FTP_STATE_PORT_REQUEST,
+ FTP_STATE_PORT_RESPONSE,
+ /* PASV command from client */
+ FTP_STATE_PASV_REQUEST,
+ FTP_STATE_PASV_RESPONSE,
};
/* We record seq number and length of ftp ip/port text here: all in
@@ -28,7 +31,10 @@
u_int32_t seq;
/* 0 means not found yet */
u_int32_t len;
- enum ip_ct_ftp_type ftptype;
+ /* Direction of packet, which must be mangled by NAT */
+ enum ip_conntrack_dir dir;
+ /* Expected FTP state */
+ enum ip_ct_ftp_state expect;
/* Port that was to be used */
u_int16_t port;
/* Next valid seq position for cmd matching after newline */
--- linux-2.4.0.base/net/ipv4/netfilter/ip_conntrack_ftp.c Thu Aug 10 21:35:15 2000
+++ linux-2.4.0/net/ipv4/netfilter/ip_conntrack_ftp.c Thu Apr 19 06:37:11 2001
@@ -12,8 +12,14 @@
DECLARE_LOCK(ip_ftp_lock);
struct module *ip_conntrack_ftp = THIS_MODULE;
-#define SERVER_STRING "227 Entering Passive Mode ("
-#define CLIENT_STRING "PORT "
+#define PORT_REQUEST "PORT " /* PORT ,,,,,\r\n */
+#define PORT_RESPONSE "200 " /* 200 arbitrary text\r\n */
+#define PASV_REQUEST "PASV" /* PASV\r\n */
+#define PASV_RESPONSE "227 " /* 227 arbitrary text (,,,,,).\r\n */
+
+#define PORT_NONE 0
+#define PORT_SET 1
+#define PORT_ACCEPT 2
#if 0
#define DEBUGP printk
@@ -24,10 +30,26 @@
static struct {
const char *pattern;
size_t plen;
- char term;
-} search[2] = {
- [IP_CT_FTP_PORT] { CLIENT_STRING, sizeof(CLIENT_STRING) - 1, '\r' },
- [IP_CT_FTP_PASV] { SERVER_STRING, sizeof(SERVER_STRING) - 1, ')' }
+ char skip; /* 0: no skip char */
+ char term; /* 0: don't check termination */
+ enum ip_ct_ftp_state expect; /* expect state */
+ enum ip_conntrack_dir expected_dir; /* dir as expected state*/
+ enum ip_conntrack_dir dir; /* mangle NAT packets in this dir */
+ int port;
+} search[5] = {
+ [FTP_STATE_INVALID] { },
+ [FTP_STATE_PORT_REQUEST] { PORT_REQUEST, sizeof(PORT_REQUEST) - 1, 0, '\r',
+ FTP_STATE_PORT_RESPONSE, IP_CT_DIR_ORIGINAL, IP_CT_DIR_ORIGINAL,
+ PORT_SET},
+ [FTP_STATE_PORT_RESPONSE] { PORT_RESPONSE, sizeof(PORT_RESPONSE) - 1, 0, 0,
+ FTP_STATE_PORT_REQUEST, IP_CT_DIR_REPLY, IP_CT_DIR_ORIGINAL,
+ PORT_ACCEPT},
+ [FTP_STATE_PASV_REQUEST] { PASV_REQUEST, sizeof(PASV_REQUEST) - 1, 0, '\r',
+ FTP_STATE_PASV_RESPONSE, IP_CT_DIR_ORIGINAL, IP_CT_DIR_REPLY,
+ PORT_NONE},
+ [FTP_STATE_PASV_RESPONSE] { PASV_RESPONSE, sizeof(PASV_RESPONSE) - 1, '(', ')',
+ FTP_STATE_PASV_REQUEST, IP_CT_DIR_REPLY, IP_CT_DIR_REPLY,
+ PORT_SET|PORT_ACCEPT},
};
/* Returns 0, or length of numbers */
@@ -60,12 +82,17 @@
/* Return 1 for match, 0 for accept, -1 for partial. */
static int find_pattern(const char *data, size_t dlen,
- const char *pattern, size_t plen,
- char term,
unsigned int *numoff,
unsigned int *numlen,
- u_int32_t array[6])
+ u_int32_t array[6],
+ enum ip_ct_ftp_state state)
{
+ const char *pattern = search[state].pattern;
+ size_t plen = search[state].plen;
+ char skip = search[state].skip;
+ char term = search[state].term;
+ size_t i;
+
if (dlen == 0)
return 0;
@@ -82,19 +109,45 @@
DEBUGP("ftp: string mismatch\n");
for (i = 0; i < plen; i++) {
- DEBUGFTP("ftp:char %u `%c'(%u) vs `%c'(%u)\n",
- i, data[i], data[i],
- pattern[i], pattern[i]);
+ DEBUGP("ftp:char %u `%c'(%u) vs `%c'(%u)\n",
+ i, data[i], data[i],
+ pattern[i], pattern[i]);
}
#endif
return 0;
}
+
+ *numoff = 0;
+ *numlen = dlen;
+ i = plen;
+
+ /* Now we've found the constant string, try to skip
+ to the 'skip' character */
+ if (skip != 0) {
+ for (/* */; (i < dlen) && (data[i] != skip); i++)
+ ;
+ i++; /* skip over the last character */
+
+ if (i >= dlen)
+ return 0;
+
+ *numoff = i;
+ }
+ if (search[state].port & PORT_SET) {
+ *numlen = try_number(data + i, dlen - i, array, term);
+ if (!*numlen)
+ return -1;
+
+ return 1;
+ }
+ if (term != 0) {
+ if (*(data + plen) == term)
+ return 1;
- *numoff = plen;
- *numlen = try_number(data + plen, dlen - plen, array, term);
- if (!*numlen)
+ DEBUGP("term != 0, not matched: %s:%i %i\n", data, dlen, plen);
return -1;
-
+ }
+
return 1;
}
@@ -112,9 +165,11 @@
int old_seq_aft_nl_set;
u_int32_t array[6] = { 0 };
int dir = CTINFO2DIR(ctinfo);
- unsigned int matchlen, matchoff;
+ unsigned int matchlen, matchoff = 0;
struct ip_conntrack_tuple t, mask;
struct ip_ct_ftp *info = &ct->help.ct_ftp_info;
+ enum ip_ct_ftp_state expect, try;
+ u_int16_t port;
/* Until there's been traffic both ways, don't look in packets. */
if (ctinfo != IP_CT_ESTABLISHED
@@ -138,10 +193,14 @@
NIPQUAD(iph->daddr));
return NF_ACCEPT;
}
+ /* Dataless packet */
+ if (datalen == 0)
+ return NF_ACCEPT;
LOCK_BH(&ip_ftp_lock);
old_seq_aft_nl_set = info->seq_aft_nl_set[dir];
old_seq_aft_nl = info->seq_aft_nl[dir];
+ expect = info->expect;
DEBUGP("conntrack_ftp: datalen %u\n", datalen);
if ((datalen > 0) && (data[datalen-1] == '\n')) {
@@ -163,11 +222,16 @@
return NF_ACCEPT;
}
+ /* Check response/resent request depending on dir
+ or try matching a PASV request */
+ try = expect ?
+ (dir == search[expect].expected_dir ? expect : search[expect].expect)
+ : FTP_STATE_PASV_REQUEST;
+again:
switch (find_pattern(data, datalen,
- search[dir].pattern,
- search[dir].plen, search[dir].term,
&matchoff, &matchlen,
- array)) {
+ array,
+ try)) {
case -1: /* partial */
/* We don't usually drop packets. After all, this is
connection tracking, not packet filtering.
@@ -179,39 +243,66 @@
return NF_DROP;
case 0: /* no match */
- DEBUGP("ip_conntrack_ftp_help: no match\n");
- return NF_ACCEPT;
+ switch (try) {
+ case FTP_STATE_PASV_REQUEST: /* No PASV: is it a PORT request? */
+ try = FTP_STATE_PORT_REQUEST;
+ goto again;
+
+ case FTP_STATE_PORT_REQUEST: /* No PASV and neither a PORT request */
+ DEBUGP("ip_conntrack_ftp_help: no match\n");
+ return NF_ACCEPT;
+
+ default:
+ /* No match - clean ftp info, but accept the packet:
+ it can be a rejected FTP PASV/PORT command */
+ DEBUGP("ip_conntrack_ftp_help: no PORT/PASV response match\n");
+ LOCK_BH(&ip_ftp_lock);
+ info->expect = FTP_STATE_INVALID;
+ UNLOCK_BH(&ip_ftp_lock);
+
+ return NF_ACCEPT;
+ }
}
DEBUGP("conntrack_ftp: match `%.*s' (%u bytes at %u)\n",
(int)matchlen, data + matchoff,
matchlen, ntohl(tcph->seq) + matchoff);
- /* Update the ftp info */
+ /*
+ * Update the ftp info only if the source address matches the address specified
+ * in the PORT or PASV command. Closes hole where packets could be dangerously
+ * marked as RELATED to bypass filtering rules. Thanks to Cristiano Lincoln
+ * Mattos for the report.
+ */
LOCK_BH(&ip_ftp_lock);
- if (htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) | array[3])
- == ct->tuplehash[dir].tuple.src.ip) {
+ if (search[try].port & PORT_SET
+ && htonl((array[0] << 24) | (array[1] << 16) | (array[2] << 8) | array[3])
+ == ct->tuplehash[dir].tuple.src.ip) {
info->is_ftp = 1;
info->seq = ntohl(tcph->seq) + matchoff;
info->len = matchlen;
- info->ftptype = dir;
+ info->dir = search[try].dir;
+ info->expect = search[try].expect;
info->port = array[4] << 8 | array[5];
+ } else if (search[try].port == PORT_NONE)
+ info->expect = search[try].expect;
+
+ if (search[try].port & PORT_ACCEPT) {
+ info->expect = FTP_STATE_INVALID;
+ port = htons(info->port);
} else {
- /* Enrico Scholz's passive FTP to partially RNAT'd ftp
- server: it really wants us to connect to a
- different IP address. Simply don't record it for
- NAT. */
- DEBUGP("conntrack_ftp: NOT RECORDING: %u,%u,%u,%u != %u.%u.%u.%u\n",
- array[0], array[1], array[2], array[3],
- NIPQUAD(ct->tuplehash[dir].tuple.src.ip));
+ UNLOCK_BH(&ip_ftp_lock);
+ return NF_ACCEPT;
}
+ DEBUGP("expect %u.%u.%u.%u:%u->%u.%u.%u.%u:%u\n",
+ NIPQUAD(ct->tuplehash[!dir].tuple.src.ip), 0,
+ NIPQUAD(ct->tuplehash[dir].tuple.src.ip), ntohs(port));
t = ((struct ip_conntrack_tuple)
{ { ct->tuplehash[!dir].tuple.src.ip,
{ 0 } },
- { htonl((array[0] << 24) | (array[1] << 16)
- | (array[2] << 8) | array[3]),
- { htons(array[4] << 8 | array[5]) },
+ { ct->tuplehash[dir].tuple.src.ip,
+ { port },
IPPROTO_TCP }});
mask = ((struct ip_conntrack_tuple)
{ { 0xFFFFFFFF, { 0 } },
--- linux-2.4.0.base/net/ipv4/netfilter/ip_nat_ftp.c Tue Apr 3 09:48:01 2001
+++ linux-2.4.0/net/ipv4/netfilter/ip_nat_ftp.c Wed Apr 18 21:59:30 2001
@@ -50,7 +50,7 @@
return 0;
}
- if (ftpinfo->ftptype == IP_CT_FTP_PORT) {
+ if (ftpinfo->dir == IP_CT_DIR_ORIGINAL) {
/* PORT command: make connection go to the client. */
newdstip = master->tuplehash[IP_CT_DIR_ORIGINAL].tuple.src.ip;
newsrcip = master->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;
@@ -180,7 +180,7 @@
/* Change address inside packet to match way we're mapping
this connection. */
- if (ct_ftp_info->ftptype == IP_CT_FTP_PASV) {
+ if (ct_ftp_info->dir == IP_CT_DIR_REPLY) {
/* PASV response: must be where client thinks server
is */
newip = ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple.dst.ip;