httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
Subject Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues
Date Fri, 11 Sep 2009 14:30:27 GMT
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");

Mime
View raw message