You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by jo...@apache.org on 2009/09/14 16:16:14 UTC
svn commit: r814652 - /httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
Author: jorton
Date: Mon Sep 14 14:16:14 2009
New Revision: 814652
URL: http://svn.apache.org/viewvc?rev=814652&view=rev
Log:
Security fix - this is presumed to fix CVE-2009-3094 (the disclosed
information was limited so this has not been confirmed):
* modules/proxy/mod_proxy_ftp.c (parse_epsv_reply): New function.
(proxy_ftp_handler): Fix possible NULL pointer deference in
apr_socket_close(NULL) on error paths. Fix possible buffer overread
in EPSV response parser; use parse_epsv_reply instead. Thanks to
Jeff Trawick and Stefan Fritsch for analysis of this issue.
Submitted by: Stefan Fritsch <sf fritsch.de>, jorton
Modified:
httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c?rev=814652&r1=814651&r2=814652&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c (original)
+++ httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c Mon Sep 14 14:16:14 2009
@@ -683,6 +683,31 @@
return APR_SUCCESS;
}
+/* Parse EPSV reply and return port, or zero on error. Modifies
+ * 'reply'. */
+static apr_port_t parse_epsv_reply(char *reply)
+{
+ char *p, *ep;
+ 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. */
+ p = ap_strchr(reply, '(');
+ if (p == NULL || !p[0] || !p[1] || p[1] != p[2] || p[1] != p[3]
+ || p[4] == p[1]) {
+ return 0;
+ }
+
+ errno = 0;
+ port = strtol(p + 4, &ep, 10);
+ if (errno || port < 1 || port > 65535 || ep[0] != p[1] || ep[1] != ')') {
+ 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)
@@ -1296,26 +1321,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_epsv_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",
@@ -1356,10 +1366,6 @@
connect = 1;
}
}
- else {
- /* and try the regular way */
- apr_socket_close(data_sock);
- }
}
}
@@ -1446,10 +1452,6 @@
connect = 1;
}
}
- else {
- /* and try the regular way */
- apr_socket_close(data_sock);
- }
}
}
/*bypass:*/
@@ -1929,7 +1931,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");
Re: svn commit: r814652 - /httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Mon, Sep 14, 2009 at 10:16 AM, <jo...@apache.org> wrote:
> Author: jorton
> Date: Mon Sep 14 14:16:14 2009
> New Revision: 814652
>
> URL: http://svn.apache.org/viewvc?rev=814652&view=rev
> Log:
> Security fix - this is presumed to fix CVE-2009-3094 (the disclosed
> information was limited so this has not been confirmed):
>
> * modules/proxy/mod_proxy_ftp.c (parse_epsv_reply): New function.
> (proxy_ftp_handler): Fix possible NULL pointer deference in
> apr_socket_close(NULL) on error paths. Fix possible buffer overread
> in EPSV response parser; use parse_epsv_reply instead. Thanks to
> Jeff Trawick and Stefan Fritsch for analysis of this issue.
>
> Submitted by: Stefan Fritsch <sf fritsch.de>, jorton
>
> Modified:
> httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c?rev=814652&r1=814651&r2=814652&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c Mon Sep 14 14:16:14
> 2009
> @@ -683,6 +683,31 @@
> return APR_SUCCESS;
> }
>
> +/* Parse EPSV reply and return port, or zero on error. Modifies
> + * 'reply'. */
> +static apr_port_t parse_epsv_reply(char *reply)
> +{
> + char *p, *ep;
> + 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. */
> + p = ap_strchr(reply, '(');
> + if (p == NULL || !p[0] || !p[1] || p[1] != p[2] || p[1] != p[3]
> + || p[4] == p[1]) {
> + return 0;
> + }
> +
> + errno = 0;
> + port = strtol(p + 4, &ep, 10);
>
Isn't the check "p[4] == p[1]" superfluous since p[4-n] will be checked by
strtol() and following code?
Re: svn commit: r814652 -
/httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
Posted by Joe Orton <jo...@redhat.com>.
On Mon, Sep 14, 2009 at 09:04:08PM +0200, Ruediger Pluem wrote:
> On 09/14/2009 04:16 PM, jorton@apache.org wrote:
> > + /* Reply syntax per RFC 2428: "229 blah blah (|||port|)" where '|'
> > + * can be any character in ASCII from 33-126, obscurely. Verify
> > + * the syntax. */
> > + p = ap_strchr(reply, '(');
> > + if (p == NULL || !p[0] || !p[1] || p[1] != p[2] || p[1] != p[3]
> > + || p[4] == p[1]) {
>
> Hm. Isn't p[0] always == '(' if p != NULL?
> If yes !p[0] || could go away.
Yes, thanks - http://svn.apache.org/viewvc?view=rev&revision=814792
Re: svn commit: r814652 - /httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
Posted by Ruediger Pluem <rp...@apache.org>.
On 09/14/2009 04:16 PM, jorton@apache.org wrote:
> Author: jorton
> Date: Mon Sep 14 14:16:14 2009
> New Revision: 814652
>
> URL: http://svn.apache.org/viewvc?rev=814652&view=rev
> Log:
> Security fix - this is presumed to fix CVE-2009-3094 (the disclosed
> information was limited so this has not been confirmed):
>
> * modules/proxy/mod_proxy_ftp.c (parse_epsv_reply): New function.
> (proxy_ftp_handler): Fix possible NULL pointer deference in
> apr_socket_close(NULL) on error paths. Fix possible buffer overread
> in EPSV response parser; use parse_epsv_reply instead. Thanks to
> Jeff Trawick and Stefan Fritsch for analysis of this issue.
>
> Submitted by: Stefan Fritsch <sf fritsch.de>, jorton
>
> Modified:
> httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c?rev=814652&r1=814651&r2=814652&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_ftp.c Mon Sep 14 14:16:14 2009
> @@ -683,6 +683,31 @@
> return APR_SUCCESS;
> }
>
> +/* Parse EPSV reply and return port, or zero on error. Modifies
> + * 'reply'. */
> +static apr_port_t parse_epsv_reply(char *reply)
> +{
> + char *p, *ep;
> + 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. */
> + p = ap_strchr(reply, '(');
> + if (p == NULL || !p[0] || !p[1] || p[1] != p[2] || p[1] != p[3]
> + || p[4] == p[1]) {
Hm. Isn't p[0] always == '(' if p != NULL?
If yes !p[0] || could go away.
Regards
Rüdiger