On Thu, Sep 10, 2009 at 07:02:01PM +0200, Stefan Fritsch wrote:
> in case you haven't noticed yet, some new mod_proxy_ftp issues have
> been reported:
>
> http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-3094
>
> The ap_proxy_ftp_handler function in modules/proxy/proxy_ftp.c in the
> mod_proxy_ftp module in the Apache HTTP Server 2.0.63 and 2.2.13
> allows remote FTP servers to cause a denial of service (NULL pointer
> dereference and child process crash) via a malformed reply to an EPSV
> command.
Hi Stefan, thanks for posting. These changes look good.
Jeff noted on security@ that there is also a more specific problem with
the parsing of the PASV/EPSV responses, which can read past the end of
the string for a malformed response.
I propose the following patch which:
a) correctly parses EPSV responses following the RFC (I ripped off the
code I wrote for sitecopy to do this)
b) parses PASV responses less badly
c) fixes the apr_socket_close(NULL) error pathsper your patch, and a
later one which was pointed out by Tomas Hoger, though that one should
not strictly be unnecessary
I'll have a look at the other CVE next. This 1100-line trainwreck of a
function should definitely win some kind of prize.
Index: mod_proxy_ftp.c
===================================================================
--- mod_proxy_ftp.c (revision 813335)
+++ mod_proxy_ftp.c (working copy)
@@ -683,6 +683,34 @@
return APR_SUCCESS;
}
+/* Parse EPSV reply and return port, or zero on error. Modifies
+ * 'reply'. */
+static apr_port_t parse_epasv_reply(char *reply)
+{
+ char *p = ap_strchr(reply, '('), *ep, *term;
+ long port;
+
+ /* Reply syntax per RFC 2428: "229 blah blah (|||port|)" where '|'
+ * can be any character in ASCII from 33-126, obscurely. Verify
+ * the syntax. */
+ if (p == NULL || p[1] != p[2] || p[1] != p[3]
+ || (ep = strchr(p + 4, ')')) == NULL
+ || ep == p + 4 || ep[-1] != p[1]) {
+ return 0;
+ }
+
+ /* ...ep points to the closing ) */
+ errno = 0;
+ port = strtol(p + 4, &term, 10);
+ /* The integer portion must terminate at the byte before the
+ * closing ")". */
+ if (term != ep - 1 || errno || port < 1 || port > 65535) {
+ return 0;
+ }
+
+ return (apr_port_t)port;
+}
+
/*
* Generic "send FTP command to server" routine, using the control socket.
* Returns the FTP returncode (3 digit code)
@@ -1291,26 +1319,11 @@
return ftp_proxyerror(r, backend, HTTP_BAD_GATEWAY, ftpmessage);
}
else if (rc == 229) {
- char *pstr;
- char *tok_cntx;
+ /* Parse the port out of the EPSV reply. */
+ data_port = parse_epasv_reply(ftpmessage);
- pstr = ftpmessage;
- pstr = apr_strtok(pstr, " ", &tok_cntx); /* separate result
code */
- if (pstr != NULL) {
- if (*(pstr + strlen(pstr) + 1) == '=') {
- pstr += strlen(pstr) + 2;
- }
- else {
- pstr = apr_strtok(NULL, "(", &tok_cntx); /* separate
address &
- * port params
*/
- if (pstr != NULL)
- pstr = apr_strtok(NULL, ")", &tok_cntx);
- }
- }
-
- if (pstr) {
+ if (data_port) {
apr_sockaddr_t *epsv_addr;
- data_port = atoi(pstr + 3);
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"proxy: FTP: EPSV contacting remote host on port %d",
@@ -1351,10 +1364,6 @@
connect = 1;
}
}
- else {
- /* and try the regular way */
- apr_socket_close(data_sock);
- }
}
}
@@ -1381,26 +1390,13 @@
char *pstr;
char *tok_cntx;
-/* FIXME: Check PASV against RFC1123 */
+ /* Find the last opening bracket; this mostly works in
+ * practice though is not strictly required, as noted in
+ * RFC 1123 section 4.1.2.6, */
+ pstr = strrchr(ftpmessage, '(');
- pstr = ftpmessage;
- pstr = apr_strtok(pstr, " ", &tok_cntx); /* separate result
code */
- if (pstr != NULL) {
- if (*(pstr + strlen(pstr) + 1) == '=') {
- pstr += strlen(pstr) + 2;
- }
- else {
- pstr = apr_strtok(NULL, "(", &tok_cntx); /* separate
address &
- * port params
*/
- if (pstr != NULL)
- pstr = apr_strtok(NULL, ")", &tok_cntx);
- }
- }
-
-/* FIXME: Only supports IPV4 - fix in RFC2428 */
-
if (pstr != NULL && (sscanf(pstr,
- "%d,%d,%d,%d,%d,%d", &h3, &h2, &h1, &h0, &p1, &p0) == 6)) {
+ "(%d,%d,%d,%d,%d,%d", &h3, &h2, &h1, &h0, &p1, &p0) == 6)) {
apr_sockaddr_t *pasv_addr;
apr_port_t pasvport = (p1 << 8) + p0;
@@ -1441,10 +1437,6 @@
connect = 1;
}
}
- else {
- /* and try the regular way */
- apr_socket_close(data_sock);
- }
}
}
/*bypass:*/
@@ -1924,7 +1916,9 @@
* for a slow client to eat these bytes
*/
ap_flush_conn(data);
- apr_socket_close(data_sock);
+ if (data_sock) {
+ apr_socket_close(data_sock);
+ }
data_sock = NULL;
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, r->server,
"proxy: FTP: data connection closed");
