Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-14 Thread Joe Orton 
On Sat, Sep 12, 2009 at 10:43:29PM +0200, Stefan Fritsch wrote:
 On Fri, 11 Sep 2009, Joe Orton wrote:
 +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;
 +}

 Shouldn't you also check for p[1] != 0 before p[1] != p[2], to catch the  
 case where reply ends after the opening bracket?

Yes indeed!  Thanks a lot.  I've rewritten that code slightly, tested 
again, and committed here:

  http://svn.apache.org/viewvc?view=revrevision=814652

I've not touched the PASV code in that commit, since there doesn't seem 
to be a security issue there, ugly as the code is.

Regards, Joe

Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-13 Thread Stefan Fritsch 
Shouldn't you also check for p[1] != 0 before p[1] != p[2], to catch the case 
where reply ends after the opening bracket?


This should be p[1] == 0, of course.

Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-12 Thread Stefan Fritsch 

On Fri, 11 Sep 2009, Joe Orton wrote:

+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;
+}


Shouldn't you also check for p[1] != 0 before p[1] != p[2], to catch the 
case where reply ends after the opening bracket?


Apart from that, both this patch and the one you have already 
commited look fine. I haven't actually tested them, though.


Stefan

Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-11 Thread Joe Orton 
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)) {
+

Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-11 Thread Ruediger Pluem 


On 09/11/2009 04:52 PM, Joe Orton wrote:
 On Thu, Sep 10, 2009 at 07:02:01PM +0200, Stefan Fritsch wrote:
 The (untested) patch below should fix CVE-2009-3094. For CVE-2009-3095 
 there is only little information. But looking at the code, it seems 
 the username and password sent by the browser are sent to the ftp 
 server without sanitization (i.e. they can contain LF characters).
 
 Spot on, good catch.
 
 using Authorization: Basic RkVBVApGRUFUCg== results in:
 
 read(11, 220 (vsFTPd 2.0.6)\r\n, 8000) = 20
 write(2, [Fri Sep 11 15:46:18 2009] [debu..., 88) = 88
 writev(11, [{USER FEAT\nFEAT\n\r\n, 17}], 1) = 17
 write(2, [Fri Sep 11 15:46:18 2009] [debu..., 87) = 87
 
 I think this should be sufficient - any other characters it's worth 
 filtering for?

Seems sufficient to me. Don't we need to this as well a few lines
below for username and password fetched from the URL?

   else if ((user = r-parsed_uri.user) != NULL) {
user = apr_pstrdup(p, user);
decodeenc(user);
if ((password = r-parsed_uri.password) != NULL) {
char *tmp = apr_pstrdup(p, password);
decodeenc(tmp);
password = tmp;
}
}

Couldn't the username / password part of the URL contain %0A / %OD?

Regards

Rüdiger

Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-11 Thread Joe Orton 
On Fri, Sep 11, 2009 at 10:51:11PM +0200, Ruediger Pluem wrote:
 On 09/11/2009 04:52 PM, Joe Orton wrote:
  On Thu, Sep 10, 2009 at 07:02:01PM +0200, Stefan Fritsch wrote:
  The (untested) patch below should fix CVE-2009-3094. For CVE-2009-3095 
  there is only little information. But looking at the code, it seems 
  the username and password sent by the browser are sent to the ftp 
  server without sanitization (i.e. they can contain LF characters).
  
  Spot on, good catch.
  
  using Authorization: Basic RkVBVApGRUFUCg== results in:
  
  read(11, 220 (vsFTPd 2.0.6)\r\n, 8000) = 20
  write(2, [Fri Sep 11 15:46:18 2009] [debu..., 88) = 88
  writev(11, [{USER FEAT\nFEAT\n\r\n, 17}], 1) = 17
  write(2, [Fri Sep 11 15:46:18 2009] [debu..., 87) = 87
  
  I think this should be sufficient - any other characters it's worth 
  filtering for?
 
 Seems sufficient to me. Don't we need to this as well a few lines
 below for username and password fetched from the URL?
...
 Couldn't the username / password part of the URL contain %0A / %OD?

It seems that the code already catches that case -

proxy_ftp_canon extracts username/password and does:

if (user != NULL  !ftp_check_string(user))
return HTTP_BAD_REQUEST;
if (password != NULL  !ftp_check_string(password))
return HTTP_BAD_REQUEST;

where ftp_check_string() checks specifically for CR/LF.

I hadn't spotted ftp_check_string() before but clearly that should be 
used for the check here too - I'll adjust the patch and commit that part 
since it's less complicated than the other issue.

Regards, Joe

CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-10 Thread Stefan Fritsch 
Hi,

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.


http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-3095

The mod_proxy_ftp module in the Apache HTTP Server allows remote 
attackers to bypass intended access restrictions and send arbitrary 
commands to an FTP server via vectors related to the embedding of 
these commands in the Authorization HTTP header, as demonstrated by a 
certain module in VulnDisco Pack Professional 8.11.


The (untested) patch below should fix CVE-2009-3094. For CVE-2009-3095 
there is only little information. But looking at the code, it seems 
the username and password sent by the browser are sent to the ftp 
server without sanitization (i.e. they can contain LF characters).

Cheers,
Stefan

--- a/modules/proxy/mod_proxy_ftp.c
+++ b/modules/proxy/mod_proxy_ftp.c
@@ -1351,10 +1351,6 @@ static int proxy_ftp_handler(request_rec *r, 
proxy_worker *worker,
 connect = 1;
 }
 }
-else {
-/* and try the regular way */
-apr_socket_close(data_sock);
-}
 }
 }

@@ -1441,10 +1437,6 @@ static int proxy_ftp_handler(request_rec *r, 
proxy_worker *worker,
 connect = 1;
 }
 }
-else {
-/* and try the regular way */
-apr_socket_close(data_sock);
-}
 }
 }
 /*bypass:*/

Re: CVE-2009-3094, CVE-2009-3095: mod_proxy_ftp issues

2009-09-10 Thread William A. Rowe, Jr. 
Stefan Fritsch wrote:
 Hi,
 
 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.

The security list is discussing this issue.

 http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-3095
 
 The mod_proxy_ftp module in the Apache HTTP Server allows remote 
 attackers to bypass intended access restrictions and send arbitrary 
 commands to an FTP server via vectors related to the embedding of 
 these commands in the Authorization HTTP header, as demonstrated by a 
 certain module in VulnDisco Pack Professional 8.11.
 
 
 The (untested) patch below should fix CVE-2009-3094. For CVE-2009-3095 
 there is only little information. But looking at the code, it seems 
 the username and password sent by the browser are sent to the ftp 
 server without sanitization (i.e. they can contain LF characters).

While we should fix this, this is likely to be a FTP bug; FTP commands
must be CRLF terminated, as spelled out in RFC854, RFC959 and later
clarified further by RFC1123.

If we transliterate LF to CRLF then this is an httpd mod_proxy_ftp
issue, of course, and we should sanitize the data.

As these are already disclosed, the final patches will simply be committed
and discussion will move to this list.