From eed355b798ec77ed8b67555087fc5866b522c5fc Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 10 Apr 2015 18:02:58 +0200 Subject: [PATCH 1/6] [ticket/security-180] Check if redirect URL contains board URL SECURITY-180 --- phpBB/includes/functions.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index f0657b9016..f79a0a9e52 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -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) { From bca1b96b2e9235bbb4a3e7a104dd79e7f3761679 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 11 Apr 2015 16:41:20 +0200 Subject: [PATCH 2/6] [ticket/security-180] Make sure that redirect goes to full URL plus slash SECURITY-180 --- phpBB/includes/functions.php | 2 +- tests/security/redirect_test.php | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index f79a0a9e52..a6a98954de 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2580,7 +2580,7 @@ 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) + if (!$disable_cd_check && strpos($url, generate_board_url(true) . '/') !== 0) { trigger_error('Tried to redirect to potentially insecure url.', E_USER_ERROR); } diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php index 872a331dc7..46ec5c8edb 100644 --- a/tests/security/redirect_test.php +++ b/tests/security/redirect_test.php @@ -24,6 +24,9 @@ class phpbb_security_redirect_test extends phpbb_security_test_base 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', false, 'http://localhost/phpBB'), + 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), ); } From ee658bfe7bd284573d199c3c2a76007c5509695d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Sat, 11 Apr 2015 17:08:28 +0200 Subject: [PATCH 3/6] [ticket/security-180] Always fail when redirecting to an insecure URL SECURITY-180 --- phpBB/includes/functions.php | 2 +- tests/security/redirect_test.php | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index a6a98954de..f2bc63cf23 100644 --- 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] == '/') diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php index 46ec5c8edb..9161bcbfe8 100644 --- a/tests/security/redirect_test.php +++ b/tests/security/redirect_test.php @@ -18,13 +18,13 @@ class phpbb_security_redirect_test extends phpbb_security_test_base { // 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', false, 'http://localhost/phpBB'), + 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), ); From 18fc621d73757ef793fba08f7da4e048b293a059 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 23 Apr 2015 15:27:30 +0200 Subject: [PATCH 4/6] [ticket/security-180] Add tests for redirecting to main URL SECURITY-180 --- tests/security/redirect_test.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/security/redirect_test.php b/tests/security/redirect_test.php index 9161bcbfe8..9a24ba5d65 100644 --- a/tests/security/redirect_test.php +++ b/tests/security/redirect_test.php @@ -27,6 +27,8 @@ class phpbb_security_redirect_test extends phpbb_security_test_base 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/'), ); } From ec207d0a71ba2c15e7cdcb2b59acd93aaa011223 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 11 Apr 2015 16:48:57 +0200 Subject: [PATCH 5/6] [ticket/security-180] Merge if statement with previous one in 3.1.x SECURITY-180 --- phpBB/includes/functions.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index 0cf1ab0f24..fcfed8b2a4 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2347,15 +2347,9 @@ function redirect($url, $return = false, $disable_cd_check = false) // Clean URL and check if we go outside the forum directory $url = $phpbb_path_helper->clean_url($url); - if (!$disable_cd_check && strpos($url, generate_board_url(true)) === false) - { - trigger_error('INSECURE_REDIRECT', E_USER_ERROR); - } - - // 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); + trigger_error('INSECURE_REDIRECT', E_USER_ERROR); } // Make sure no linebreaks are there... to prevent http response splitting for PHP < 4.4.2 From d7c96cc60ca5836356a0c4f0bb9a9976a4a232ab Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 24 Apr 2015 15:09:30 +0200 Subject: [PATCH 6/6] [ticket/security-180] Use language variable for redirect error in 3.1+ SECURITY-180 --- phpBB/includes/functions.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/functions.php b/phpBB/includes/functions.php index e00231c360..cc5d185f4e 100644 --- a/phpBB/includes/functions.php +++ b/phpBB/includes/functions.php @@ -2309,7 +2309,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) { - trigger_error('Tried to redirect to potentially insecure url.', E_USER_ERROR); + trigger_error('INSECURE_REDIRECT', E_USER_ERROR); } } else if ($url[0] == '/')