On Mon, Sep 14, 2009 at 10:16 AM, <jor...@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?
