From 3efa8648c565f7a4bd24436a55336e7ca70fcb06 Mon Sep 17 00:00:00 2001 From: Hypolite Petovan Date: Tue, 8 Sep 2020 10:44:27 -0400 Subject: [PATCH] Fix security vulnerability in admin modules - The Module\BaseAdmin::post method checked credentials but didn't abort the process when it failed - Created Module\BaseAdmin::checkAdminAccess method --- src/Module/Admin/Addons/Details.php | 2 +- src/Module/Admin/Blocklist/Contact.php | 2 +- src/Module/Admin/Blocklist/Server.php | 2 +- src/Module/Admin/Features.php | 2 +- src/Module/Admin/Item/Delete.php | 2 +- src/Module/Admin/Logs/Settings.php | 2 +- src/Module/Admin/PhpInfo.php | 2 +- src/Module/Admin/Site.php | 2 +- src/Module/Admin/Themes/Embed.php | 2 +- src/Module/Admin/Tos.php | 2 +- src/Module/Admin/Users.php | 2 +- src/Module/BaseAdmin.php | 43 +++++++++++--------------- 12 files changed, 29 insertions(+), 36 deletions(-) diff --git a/src/Module/Admin/Addons/Details.php b/src/Module/Admin/Addons/Details.php index 85b17130c..4c1fe2df9 100644 --- a/src/Module/Admin/Addons/Details.php +++ b/src/Module/Admin/Addons/Details.php @@ -32,7 +32,7 @@ class Details extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); $addon = Strings::sanitizeFilePathItem($parameters['addon']); diff --git a/src/Module/Admin/Blocklist/Contact.php b/src/Module/Admin/Blocklist/Contact.php index 5a7d138b2..c4eedc5a8 100644 --- a/src/Module/Admin/Blocklist/Contact.php +++ b/src/Module/Admin/Blocklist/Contact.php @@ -32,7 +32,7 @@ class Contact extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); self::checkFormSecurityTokenRedirectOnError('/admin/blocklist/contact', 'admin_contactblock'); diff --git a/src/Module/Admin/Blocklist/Server.php b/src/Module/Admin/Blocklist/Server.php index b4be591e7..1290662f2 100644 --- a/src/Module/Admin/Blocklist/Server.php +++ b/src/Module/Admin/Blocklist/Server.php @@ -30,7 +30,7 @@ class Server extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); if (empty($_POST['page_blocklist_save']) && empty($_POST['page_blocklist_edit'])) { return; diff --git a/src/Module/Admin/Features.php b/src/Module/Admin/Features.php index 51ba9140e..5054da3fb 100644 --- a/src/Module/Admin/Features.php +++ b/src/Module/Admin/Features.php @@ -30,7 +30,7 @@ class Features extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); self::checkFormSecurityTokenRedirectOnError('/admin/features', 'admin_manage_features'); diff --git a/src/Module/Admin/Item/Delete.php b/src/Module/Admin/Item/Delete.php index 028e228d3..9e2bc90d9 100644 --- a/src/Module/Admin/Item/Delete.php +++ b/src/Module/Admin/Item/Delete.php @@ -31,7 +31,7 @@ class Delete extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); if (empty($_POST['page_deleteitem_submit'])) { return; diff --git a/src/Module/Admin/Logs/Settings.php b/src/Module/Admin/Logs/Settings.php index 0b5993798..7730b487d 100644 --- a/src/Module/Admin/Logs/Settings.php +++ b/src/Module/Admin/Logs/Settings.php @@ -31,7 +31,7 @@ class Settings extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); if (empty($_POST['page_logs'])) { return; diff --git a/src/Module/Admin/PhpInfo.php b/src/Module/Admin/PhpInfo.php index f282e1008..61a004618 100644 --- a/src/Module/Admin/PhpInfo.php +++ b/src/Module/Admin/PhpInfo.php @@ -27,7 +27,7 @@ class PhpInfo extends BaseAdmin { public static function rawContent(array $parameters = []) { - parent::rawContent($parameters); + self::checkAdminAccess(); phpinfo(); exit(); diff --git a/src/Module/Admin/Site.php b/src/Module/Admin/Site.php index 6380f3d93..9f3905da5 100644 --- a/src/Module/Admin/Site.php +++ b/src/Module/Admin/Site.php @@ -43,7 +43,7 @@ class Site extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); self::checkFormSecurityTokenRedirectOnError('/admin/site', 'admin_site'); diff --git a/src/Module/Admin/Themes/Embed.php b/src/Module/Admin/Themes/Embed.php index 71824c6cc..a308b43cb 100644 --- a/src/Module/Admin/Themes/Embed.php +++ b/src/Module/Admin/Themes/Embed.php @@ -38,7 +38,7 @@ class Embed extends BaseAdmin public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); $theme = Strings::sanitizeFilePathItem($parameters['theme']); if (is_file("view/theme/$theme/config.php")) { diff --git a/src/Module/Admin/Tos.php b/src/Module/Admin/Tos.php index fef199c35..aac81264b 100644 --- a/src/Module/Admin/Tos.php +++ b/src/Module/Admin/Tos.php @@ -29,7 +29,7 @@ class Tos extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); if (empty($_POST['page_tos'])) { return; diff --git a/src/Module/Admin/Users.php b/src/Module/Admin/Users.php index 751b618af..74fee7230 100644 --- a/src/Module/Admin/Users.php +++ b/src/Module/Admin/Users.php @@ -34,7 +34,7 @@ class Users extends BaseAdmin { public static function post(array $parameters = []) { - parent::post($parameters); + self::checkAdminAccess(); self::checkFormSecurityTokenRedirectOnError('/admin/users', 'admin_users'); diff --git a/src/Module/BaseAdmin.php b/src/Module/BaseAdmin.php index a7b38a503..67de97f85 100644 --- a/src/Module/BaseAdmin.php +++ b/src/Module/BaseAdmin.php @@ -26,7 +26,7 @@ use Friendica\Core\Addon; use Friendica\Core\Renderer; use Friendica\Core\Session; use Friendica\DI; -use Friendica\Network\HTTPException\ForbiddenException; +use Friendica\Network\HTTPException; require_once 'boot.php'; @@ -42,42 +42,35 @@ require_once 'boot.php'; */ abstract class BaseAdmin extends BaseModule { - public static function post(array $parameters = []) + /** + * @param bool $interactive + * @throws HTTPException\ForbiddenException + * @throws HTTPException\InternalServerErrorException + */ + public static function checkAdminAccess(bool $interactive = false) { - if (!is_site_admin()) { - return; + if (!local_user()) { + if ($interactive) { + notice(DI::l10n()->t('Please login to continue.')); + Session::set('return_path', DI::args()->getQueryString()); + DI::baseUrl()->redirect('login'); + } else { + throw new HTTPException\UnauthorizedException(DI::l10n()->t('Please login to continue.')); + } } - // do not allow a page manager to access the admin panel at all. - if (!empty($_SESSION['submanage'])) { - return; - } - } - - public static function rawContent(array $parameters = []) - { if (!is_site_admin()) { - return ''; + throw new HTTPException\ForbiddenException(DI::l10n()->t('You don\'t have access to administration pages.')); } if (!empty($_SESSION['submanage'])) { - return ''; + throw new HTTPException\ForbiddenException(DI::l10n()->t('Submanaged account can\'t access the administation pages. Please log back in as the main account.')); } - - return ''; } public static function content(array $parameters = []) { - if (!is_site_admin()) { - notice(DI::l10n()->t('Please login to continue.')); - Session::set('return_path', DI::args()->getQueryString()); - DI::baseUrl()->redirect('login'); - } - - if (!empty($_SESSION['submanage'])) { - throw new ForbiddenException(DI::l10n()->t('Submanaged account can\'t access the administation pages. Please log back in as the main account.')); - } + self::checkAdminAccess(true); // Header stuff DI::page()['htmlhead'] .= Renderer::replaceMacros(Renderer::getMarkupTemplate('admin/settings_head.tpl'), []);