Merge remote-tracking branch 'phpbb-security/ticket/security-180' into prep-release-3.0.14

bantu · bantu · commit 1a3350619f42 · 2015-04-28T21:54:50.000+02:00
* phpbb-security/ticket/security-180:
  [ticket/security-180] Add tests for redirecting to main URL
  [ticket/security-180] Always fail when redirecting to an insecure URL
  [ticket/security-180] Make sure that redirect goes to full URL plus slash
  [ticket/security-180] Check if redirect URL contains board URL
diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php
@@ -2492,7 +2492,7 @@ function redirect($url, $return = false, $disable_cd_check = false)
 		// Attention: only able to redirect within the same domain if $disable_cd_check is false (yourdomain.com -> www.yourdomain.com will not work)
 		if (!$disable_cd_check && $url_parts['host'] !== $user->host)
 		{
-			$url = generate_board_url();
+			trigger_error('Tried to redirect to potentially insecure url.', E_USER_ERROR);
 		}
 	}
 	else if ($url[0] == '/')
@@ -2579,6 +2579,12 @@ function redirect($url, $return = false, $disable_cd_check = false)
 		}
 	}
 
+	// Make sure we don't redirect to external URLs
+	if (!$disable_cd_check && strpos($url, generate_board_url(true) . '/') !== 0)
+	{
+		trigger_error('Tried to redirect to potentially insecure url.', E_USER_ERROR);
+	}
+
 	// Make sure no linebreaks are there... to prevent http response splitting for PHP < 4.4.2
 	if (strpos(urldecode($url), "\n") !== false || strpos(urldecode($url), "\r") !== false || strpos($url, ';') !== false)
 	{
diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php
@@ -18,12 +18,17 @@ static public function provider()
 	{
 		// array(Input -> redirect(), expected triggered error (else false), expected returned result url (else false))
 		return array(
-			array('data://x', false, 'http://localhost/phpBB'),
+			array('data://x', 'Tried to redirect to potentially insecure url.', false),
 			array('bad://localhost/phpBB/index.php', 'Tried to redirect to potentially insecure url.', false),
-			array('http://www.otherdomain.com/somescript.php', false, 'http://localhost/phpBB'),
+			array('http://www.otherdomain.com/somescript.php', 'Tried to redirect to potentially insecure url.', false),
 			array("http://localhost/phpBB/memberlist.php\n\rConnection: close", 'Tried to redirect to potentially insecure url.', false),
 			array('javascript:test', false, 'http://localhost/phpBB/../javascript:test'),
 			array('http://localhost/phpBB/index.php;url=', 'Tried to redirect to potentially insecure url.', false),
+			array('https://foobar.com\@http://localhost/phpBB', 'Tried to redirect to potentially insecure url.', false),
+			array('https://foobar.com\@localhost/troll/http://localhost/', 'Tried to redirect to potentially insecure url.', false),
+			array('http://localhost.foobar.com\@localhost/troll/http://localhost/', 'Tried to redirect to potentially insecure url.', false),
+			array('http://localhost/phpBB', false, 'http://localhost/phpBB'),
+			array('http://localhost/phpBB/', false, 'http://localhost/phpBB/'),
 		);
 	}
 

Original file line number	Diff line number	Diff line change
`@@ -2492,7 +2492,7 @@ function redirect($url, $return = false, $disable_cd_check = false)`
`2492`	`2492`	`// Attention: only able to redirect within the same domain if $disable_cd_check is false (yourdomain.com -> www.yourdomain.com will not work)`
`2493`	`2493`	`if (!$disable_cd_check && $url_parts['host'] !== $user->host)`
`2494`	`2494`	`{`
`2495`		`- $url = generate_board_url();`
	`2495`	`+ trigger_error('Tried to redirect to potentially insecure url.', E_USER_ERROR);`
`2496`	`2496`	`}`
`2497`	`2497`	`}`
`2498`	`2498`	`else if ($url[0] == '/')`
`@@ -2579,6 +2579,12 @@ function redirect($url, $return = false, $disable_cd_check = false)`
`2579`	`2579`	`}`
`2580`	`2580`	`}`
`2581`	`2581`
	`2582`	`+ // Make sure we don't redirect to external URLs`
	`2583`	`+ if (!$disable_cd_check && strpos($url, generate_board_url(true) . '/') !== 0)`
	`2584`	`+ {`
	`2585`	`+ trigger_error('Tried to redirect to potentially insecure url.', E_USER_ERROR);`
	`2586`	`+ }`
	`2587`	`+`
`2582`	`2588`	`// Make sure no linebreaks are there... to prevent http response splitting for PHP < 4.4.2`
`2583`	`2589`	`if (strpos(urldecode($url), "\n") !== false \|\| strpos(urldecode($url), "\r") !== false \|\| strpos($url, ';') !== false)`
`2584`	`2590`	`{`