ftp security patch
From kadlec@blackhole.kfki.hu Mon Sep 17 10:50:03 2001
Date: Wed, 18 Apr 2001 19:33:08 +0200 (CEST)
From: Jozsef Kadlecsik
To: Kis-Szabo Andras
Cc: netfilter-devel@lists.samba.org
Subject: [PATCH] was re: Security weakness in ip_conntrack_ftp
On Tue, 17 Apr 2001, Kis-Szabo Andras wrote:
> Hi,
>
> I read the ip_conntrack_ftp.c.
> I think that code is one-state based: no checks for the answers.
> Checks:
> PORT - but 220 not checked
> 227 str () - but no checks for prev. PASV command
> (And the '227 (ip,ip,ip,ip,port,port)' is a good response!)
>
> Potential weeknesses:
> - send PORT requests to the dastination ip with the wanted port
> . the conntrack opens the connection.
> ex: only ftp is allowed to the computer from outside
> telnet out 21
> PORT computer,0,22
> ssh computer
> and I can SSH to the computer ... (the server resp. is deny!)
> - the same with the PASV:
> comp#nc -l computer 21
> outs#telnet computer 21
> outs: PASV
> comp: 227 Entering Passive Mode (comp,0,22)
> outs#ssh computer
> (We can't protecting the net from this 'feature')
> But: we don't need to send the PASV req. Only the response is enough...
> We hasn't got root access to the computer:
> - There was a bug in SPF firewalls in 2000 ...
> Set the MTU 4 a small value,
> login to the target ftp server,
> try get a file "RETR filenemalongenough4mtu-errormsg227 Entering Passive Mode (comp,0,22)" ...
> the response: 3xx Cant find filename227 ..., but in 2 TCP packet,
> the 227 is at the begining of the 2nd packet ...
> - there's no ftp protocol state information tracked.
> The PORT and PASV is only valid after the successful authentication
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
The security fix and the the ftp-pasv-fix patch are merged into it.
Regards,
Jozsef
--- 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 07:56:47 2001
@@ -14,11 +14,20 @@
enum ip_ct_ftp_type
{
/* 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
};
+/* Invalid state: try again */
+#define FTP_STATE_INVALID FTP_STATE_PORT_REQUEST
+
+/* Backward compatibility with ip_nat_ftp.c */
+#define IP_CT_FTP_PORT IP_CT_DIR_ORIGINAL
+#define IP_CT_FTP_PASV IP_CT_DIR_REPLY
+
/* We record seq number and length of ftp ip/port text here: all in
host order. */
struct ip_ct_ftp
@@ -28,7 +37,9 @@
u_int32_t seq;
/* 0 means not found yet */
u_int32_t len;
- enum ip_ct_ftp_type ftptype;
+ /* in reality: direction */
+ enum ip_conntrack_dir ftptype;
+ enum ip_ct_ftp_type 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 Wed Apr 18 18:32:00 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,20 @@
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_type expect;
+ enum ip_conntrack_dir dir;
+ int port;
+} search[4] = {
+ [FTP_STATE_PORT_REQUEST] { PORT_REQUEST, sizeof(PORT_REQUEST) - 1, 0, '\r',
+ FTP_STATE_PORT_RESPONSE, IP_CT_DIR_ORIGINAL, PORT_SET},
+ [FTP_STATE_PORT_RESPONSE] { PORT_RESPONSE, sizeof(PORT_RESPONSE) - 1, 0, 0,
+ FTP_STATE_INVALID, IP_CT_DIR_REPLY, PORT_ACCEPT},
+ [FTP_STATE_PASV_REQUEST] { PASV_REQUEST, sizeof(PASV_REQUEST) - 1, 0, '\r',
+ FTP_STATE_PASV_RESPONSE, IP_CT_DIR_REPLY, PORT_NONE},
+ [FTP_STATE_PASV_RESPONSE] { PASV_RESPONSE, sizeof(PASV_RESPONSE) - 1, '(', ')',
+ FTP_STATE_INVALID, IP_CT_DIR_ORIGINAL, PORT_SET|PORT_ACCEPT},
};
/* Returns 0, or length of numbers */
@@ -61,11 +77,14 @@
/* 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,
+ char skip, char term,
unsigned int *numoff,
unsigned int *numlen,
- u_int32_t array[6])
+ u_int32_t array[6],
+ enum ip_ct_ftp_type type)
{
+ size_t i;
+
if (dlen == 0)
return 0;
@@ -82,19 +101,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[type].port & PORT_SET) {
+ *numlen = try_number(data + i, dlen - i, array, term);
+ if (!*numlen)
+ return -1;
+
+ return 1;
+ }
+ if (search[type].term != 0) {
+ if (*(data + plen) == search[type].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 +157,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_type 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 +185,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 +214,16 @@
return NF_ACCEPT;
}
+ /* Check response or try matching a PASV request */
+ try = expect ? expect : FTP_STATE_PASV_REQUEST;
+again:
switch (find_pattern(data, datalen,
- search[dir].pattern,
- search[dir].plen, search[dir].term,
+ search[try].pattern,
+ search[try].plen,
+ search[try].skip, search[try].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 +235,65 @@
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: /* Check wether it's 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->ftptype = search[try].dir;
+ info->expect = search[try].expect;
info->port = array[4] << 8 | array[5];
- } 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));
+ } else if (search[try].port == PORT_NONE)
+ info->expect = search[try].expect;
+
+ if (search[try].port & PORT_ACCEPT)
+ port = htons(info->port);
+ else {
+ 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 } },