Revert url decoding+simplifying before matching of mod_rewrite/mod_redirect

- Lot of regressions (we forgot to reencode the result)
  - Generic problem: after decode and rewrite "a?b?c": which '?' was the path?query seperator?
  - Possible solution: only decode printable characters (without '?'), and encode the result; do not encode the '%' of a not decoded character.
  - Still a problem with path simplifying, it seems many people use urls like this: http://server1/http%3a//server2/xxx
    and rewrite the path into the querystring.
  - Probably only usable with an extra config option

  => Do NOT use rewrite/redirect to protect specific urls.


git-svn-id: svn://svn.lighttpd.net/lighttpd/branches/lighttpd-1.4.x@2362 152afb58-edef-0310-8abb-c4023f1b3aa9

NEWS

@@ -20,6 +20,7 @@ NEWS
   * Fixed fix for round-robin in mod_proxy (forgot to increment the index) (#1715)
   * Fix fastcgi-authorizer handling; Status: 200 is now accepted as the doc requests
   * Compare address family in inet_ntop_cache
+  * Revert CVE-2008-4359 (#1720) fix "encoding+simplifying urls for rewrite/redirect": too many regressions.
 
 - 1.4.20 - 2008-09-30
 
@@ -67,7 +68,7 @@ NEWS
   * allow digits in [s]cgi env vars (#1712)
   * fixed dropping last character of evhost pattern (#161)
   * print helpful error message on conditionals in global block (#1550)
-  * decode url before matching in mod_rewrite (#1720)
+  * decode url before matching in mod_rewrite (#1720) -- (reverted for 1.4.21)
   * fixed conditional patching of ldap filter (#1564)
   * Match headers case insensitive in response (removing of X-{Sendfile,LIGHTTPD-*}, catching Date/Server) [2281]
   * fixed bug with case-insensitive filenames in mod_userdir (#1589), spotted by "anders1" (CVE-2008-4360)
@@ -82,7 +83,7 @@ NEWS
   * fix auth.backend.ldap.bind-dn/pw problems (only read from global context for temporary ldap reconnects, thx ruskie)
   * fix memleak in request header parsing (#1774, thx qhy) (CVE-2008-4298)
   * fix mod_rewrite memleak/endless loop detection (#1775, thx phy - again!)
-  * use decoded url for matching in mod_redirect (#1720) (CVE-2008-4359)
+  * use decoded url for matching in mod_redirect (#1720) (CVE-2008-4359) -- (reverted for 1.4.21)
 
 - 1.4.19 - 2008-03-10
 

doc/redirect.txt

@@ -39,3 +39,9 @@ url.redirect
     $HTTP["host"] =~ "^www\.(.*)" {
       url.redirect = ( "^/(.*)" => "http://%1/$1" )
     }
+
+Warning
+=======
+
+Do NOT use mod_redirect to protect specific urls, as the original url passed from the client
+is matched against your rules, for example strings like "/abc/../xyz%2f/path".

doc/rewrite.txt

@@ -43,6 +43,12 @@ url.rewrite-repeat
 The options ``url.rewrite`` and ``url.rewrite-final`` were mapped to ``url.rewrite-once``
 in 1.3.16.
 
+Warning
+=======
+
+Do NOT use mod_rewrite to protect specific urls, as the original url passed from the client
+is matched against your rules, for example strings like "/abc/../xyz%2f/path".
+
 Examples
 ========
 

src/mod_redirect.c

@@ -178,11 +178,7 @@ static handler_t mod_redirect_uri_handler(server *srv, connection *con, void *p_
 
 	mod_redirect_patch_connection(srv, con, p);
 
-	buffer_copy_string_buffer(p->match_buf, con->uri.path);
-	if (con->uri.query->used > 0) {
-		buffer_append_string_len(p->match_buf, CONST_STR_LEN("?"));
-		buffer_append_string_buffer(p->match_buf, con->uri.query);
-	}
+	buffer_copy_string_buffer(p->match_buf, con->request.uri);
 
 	for (i = 0; i < p->conf.redirect->used; i++) {
 		pcre *match;

src/mod_rewrite.c

@@ -350,11 +350,7 @@ URIHANDLER_FUNC(mod_rewrite_uri_handler) {
 
 	if (!p->conf.rewrite) return HANDLER_GO_ON;
 
-	buffer_copy_string_buffer(p->match_buf, con->uri.path);
-	if (con->uri.query->used > 0) {
-		buffer_append_string_len(p->match_buf, CONST_STR_LEN("?"));
-		buffer_append_string_buffer(p->match_buf, con->uri.query);
-	}
+	buffer_copy_string_buffer(p->match_buf, con->request.uri);
 
 	for (i = 0; i < p->conf.rewrite->used; i++) {
 		pcre *match;

src/response.c

@@ -232,6 +232,27 @@ handler_t http_response_prepare(server *srv, connection *con) {
 		}
 
 
+		/**
+		 *
+		 * call plugins
+		 *
+		 * - based on the raw URL
+		 *
+		 */
+
+		switch(r = plugins_call_handle_uri_raw(srv, con)) {
+		case HANDLER_GO_ON:
+			break;
+		case HANDLER_FINISHED:
+		case HANDLER_COMEBACK:
+		case HANDLER_WAIT_FOR_EVENT:
+		case HANDLER_ERROR:
+			return r;
+		default:
+			log_error_write(srv, __FILE__, __LINE__, "sd", "handle_uri_raw: unknown return value", r);
+			break;
+		}
+
 		/* build filename
 		 *
 		 * - decode url-encodings  (e.g. %20 -> ' ')
@@ -239,6 +260,7 @@ handler_t http_response_prepare(server *srv, connection *con) {
 		 */
 
 
+
 		if (con->request.http_method == HTTP_METHOD_OPTIONS &&
 		    con->uri.path_raw->ptr[0] == '*' && con->uri.path_raw->ptr[1] == '\0') {
 			/* OPTIONS * ... */
@@ -254,28 +276,6 @@ handler_t http_response_prepare(server *srv, connection *con) {
 			log_error_write(srv, __FILE__, __LINE__,  "sb", "URI-path     : ", con->uri.path);
 		}
 
-
-		/**
-		 *
-		 * call plugins
-		 *
-		 * - based on the raw URL
-		 *
-		 */
-
-		switch(r = plugins_call_handle_uri_raw(srv, con)) {
-		case HANDLER_GO_ON:
-			break;
-		case HANDLER_FINISHED:
-		case HANDLER_COMEBACK:
-		case HANDLER_WAIT_FOR_EVENT:
-		case HANDLER_ERROR:
-			return r;
-		default:
-			log_error_write(srv, __FILE__, __LINE__, "sd", "handle_uri_raw: unknown return value", r);
-			break;
-		}
-
 		/**
 		 *
 		 * call plugins

tests/mod-proxy.t

@@ -8,13 +8,23 @@ BEGIN {
 
 use strict;
 use IO::Socket;
-use Test::More tests => 6;
+use Test::More tests => 9;
 use LightyTest;
 
 my $tf_real = LightyTest->new();
 my $tf_proxy = LightyTest->new();
 
 my $t;
+my $php_child = -1;
+
+my $phpbin = (defined $ENV{'PHP'} ? $ENV{'PHP'} : '/usr/bin/php-cgi');
+$ENV{'PHP'} = $phpbin;
+
+SKIP: {
+	skip "PHP already running on port 1026", 1 if $tf_real->listening_on(1026);
+	skip "no php binary found", 1 unless -x $phpbin;
+	ok(-1 != ($php_child = $tf_real->spawnfcgi($phpbin, 1026)), "Spawning php");
+}
 
 ## we need two procs
 ## 1. the real webserver
@@ -26,9 +36,9 @@ $tf_real->{CONFIGFILE} = 'lighttpd.conf';
 $tf_proxy->{PORT} = 2050;
 $tf_proxy->{CONFIGFILE} = 'proxy.conf';
 
-ok($tf_real->start_proc == 0, "Starting lighttpd") or die();
+ok($tf_real->start_proc == 0, "Starting lighttpd") or goto cleanup;
 
-ok($tf_proxy->start_proc == 0, "Starting lighttpd as proxy") or die();
+ok($tf_proxy->start_proc == 0, "Starting lighttpd as proxy") or goto cleanup;
 
 $t->{REQUEST}  = ( <<EOF
 GET /index.html HTTP/1.0
@@ -46,6 +56,31 @@ EOF
 $t->{RESPONSE} = [ { 'HTTP-Protocol' => 'HTTP/1.0', 'HTTP-Status' => 200, 'Server' => 'Apache 1.3.29' } ];
 ok($tf_proxy->handle_http($t) == 0, 'drop Server from real server');
 
+SKIP: {
+	skip "no PHP running on port 1026", 1 unless $tf_real->listening_on(1026);
+	$t->{REQUEST}  = ( <<EOF
+GET /rewrite/all/some+test%3axxx%20with%20space HTTP/1.0
+Host: www.example.org
+EOF
+ );
+	$t->{RESPONSE} = [ { 'HTTP-Protocol' => 'HTTP/1.0', 'HTTP-Status' => 200, 'HTTP-Content' => '/some+test%3axxx%20with%20space' } ];
+	ok($tf_proxy->handle_http($t) == 0, 'rewrited urls work with encoded path');
+}
+
 ok($tf_proxy->stop_proc == 0, "Stopping lighttpd proxy");
 
 ok($tf_real->stop_proc == 0, "Stopping lighttpd");
+
+SKIP: {
+	skip "PHP not started, cannot stop it", 1 unless $php_child != -1;
+	ok(0 == $tf_real->endspawnfcgi($php_child), "Stopping php");
+	$php_child = -1;
+}
+
+exit 0;
+
+cleanup:
+
+$tf_real->endspawnfcgi($php_child) if $php_child != -1;
+
+die();

tests/mod-rewrite.t

@@ -8,7 +8,7 @@ BEGIN {
 
 use strict;
 use IO::Socket;
-use Test::More tests => 8;
+use Test::More tests => 7;
 use LightyTest;
 
 my $tf = LightyTest->new();
@@ -35,7 +35,7 @@ EOF
  );
 	$t->{RESPONSE} = [ { 'HTTP-Protocol' => 'HTTP/1.0', 'HTTP-Status' => 200, 'HTTP-Content' => '' } ];
 	ok($tf->handle_http($t) == 0, 'valid request');
-
+    
 	$t->{REQUEST}  = ( <<EOF
 GET /rewrite/foo?a=b HTTP/1.0
 Host: www.example.org
@@ -52,14 +52,6 @@ EOF
 	$t->{RESPONSE} = [ { 'HTTP-Protocol' => 'HTTP/1.0', 'HTTP-Status' => 200, 'HTTP-Content' => 'bar&a=b' } ];
 	ok($tf->handle_http($t) == 0, 'valid request');
 
-	$t->{REQUEST}  = ( <<EOF
-GET %2Frewrite/f%6Fo?a=b HTTP/1.0
-Host: www.example.org
-EOF
- );
-	$t->{RESPONSE} = [ { 'HTTP-Protocol' => 'HTTP/1.0', 'HTTP-Status' => 200, 'HTTP-Content' => 'a=b' } ];
-	ok($tf->handle_http($t) == 0, 'valid request with url encoded characters');
-
 	ok($tf->stop_proc == 0, "Stopping lighttpd");
 }
 

tests/proxy.conf

@@ -122,7 +122,8 @@ url.access-deny             = ( "~", ".inc")
 url.redirect                = ( "^/redirect/$" => "http://localhost:2048/" )
 
 url.rewrite		    = ( "^/rewrite/foo($|\?.+)" => "/indexfile/rewrite.php$1",
-				"^/rewrite/bar(?:$|\?(.+))" => "/indexfile/rewrite.php?bar&$1" )
+				"^/rewrite/bar(?:$|\?(.+))" => "/indexfile/rewrite.php?bar&$1",
+				"^/rewrite/all(/.*)$" => "/indexfile/rewrite.php?$1" )
 
 expire.url                  = ( "/expire/access" => "access 2 hours",
 				"/expire/modification" => "access plus 1 seconds 2 minutes")