From 2810342f6525f30e0fb3c832f00784862bbccf5e Mon Sep 17 00:00:00 2001 From: TheWitness Date: Sun, 5 Apr 2026 10:56:17 -0400 Subject: [PATCH] fix: Simplify redirect handling in Cacti and Fix Multi-Sort (#6955) (#6978) * fix: Simplify redirect handling in Cacti and Fix Multi-Sort (#6955) * fix: Simplify redirect handling in Cacti * fix: Slight improvements from Copilot review * fix: Fix for compatibility * fix: Fix small syntax error * fix: Remove comments * fix: Make the query case insensitive * fix: Encode the reference url * fix: More copilot review comments * fix: More copilot comments * fix: Double semicolon * fix: Missed one commit * fix: Encode the correct url * Update lib/html_utility.php Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * fix: Add additional checks * fix: Fix order string updates for multiple sort * fix: Add a sanitization function and do further checks * fix:Switch to local variables --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Refactor authentication method check in auth.php * Fix HTML encoding in goBack variable assignment * Change get_order_string_page parameter to false * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix error message capitalization for aggregate graph * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Apply suggestion from @Copilot Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Fix goBack link formatting in permission_denied.php * Update lib/html_utility.php Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Add type hints to sanitize_uri and validation functions * Fix comment formatting in validate_redirect_url * Fix header location formatting in auth.php * fix: Fixing PSR Issues with remote agent * fix: Additional psr comments * fix: More psr comments * fix: PHPStan Issues --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- aggregate_graphs.php | 23 ++++----- auth_changepassword.php | 61 ++++++++++------------- auth_profile.php | 18 +++---- graphs_new.php | 22 ++------ include/auth.php | 5 +- lib/functions.php | 50 +++++++++++++++++-- lib/html_utility.php | 108 +++++++++++++++++++++++++++++++++++++--- link.php | 22 ++------ permission_denied.php | 5 +- remote_agent.php | 2 + 10 files changed, 203 insertions(+), 113 deletions(-) diff --git a/aggregate_graphs.php b/aggregate_graphs.php index 1451368b2..70f1c6e0e 100644 --- a/aggregate_graphs.php +++ b/aggregate_graphs.php @@ -1019,13 +1019,15 @@ function graph_edit() : bool { if (isrv('reset')) { $_SESSION['aggregate_referer'] = 'aggregate_graphs.php'; - } elseif (isset($_SERVER['HTTP_REFERER']) && !substr_count($_SERVER['HTTP_REFERER'], 'aggregate_graphs.php')) { - $_SESSION['aggregate_referer'] = $_SERVER['HTTP_REFERER']; - } elseif (isset($_SERVER['HTTP_REFERER']) && !isset($_SESSION['aggregate_referer'])) { - $_SESSION['aggregate_referer'] = $_SERVER['HTTP_REFERER']; + } elseif (isset($_SERVER['HTTP_REFERER'])) { + if (!isset($_SESSION['aggregate_referer'])) { + $_SESSION['aggregate_referer'] = validate_redirect_url($_SERVER['HTTP_REFERER'], 'aggregate_graphs.php'); + } + } else { + $_SESSION['aggregate_referer'] = 'aggregate_graphs.php'; } - $referer = isset($_SESSION['aggregate_referer']) ? $_SESSION['aggregate_referer'] : 'aggregate_graphs.php'; + $referer = $_SESSION['aggregate_referer']; $use_graph_template = false; $aginfo = []; @@ -1074,14 +1076,11 @@ function graph_edit() : bool { } elseif (cacti_sizeof($graphs)) { $template = []; } else { - raise_message('missing_aggregate', __('Aggregate Graphs Accessed does not Exist'), MESSAGE_LEVEL_ERROR); + raise_message('missing_aggregate', __('Aggregate Graph Accessed does not exist'), MESSAGE_LEVEL_ERROR); - if (isset($_SERVER['HTTP_REFERER'])) { - $referer = $_SERVER['HTTP_REFERER']; - header('Location: ' . $referer); - } else { - header('Location: aggregate_graphs.php'); - } + $referer = validate_redirect_url($_SERVER['HTTP_REFERER'] ?? '', 'aggregate_graphs.php'); + + header('Location: ' . $referer); exit; } diff --git a/auth_changepassword.php b/auth_changepassword.php index ac94b1734..d7076eadd 100644 --- a/auth_changepassword.php +++ b/auth_changepassword.php @@ -40,37 +40,34 @@ switch ($action) { exit; default: - // If the user is not logged in, redirect them to the login page + /** + * If the user is not logged in, redirect them back to the page + * they came from, or to the login page. + */ if (!isset($_SESSION[SESS_USER_ID])) { - /* HTTP_REFERER redirect here is overridden by the unconditional - * index.php header below; use the safe fallback directly. */ - header('Location: index.php'); - + $referer = validate_redirect_url($_SERVER['HTTP_REFERER'] ?? '', 'index.php'); + header("Location: $referer"); exit; } } -if (isset($_SERVER['HTTP_REFERER'])) { - $return = $_SERVER['HTTP_REFERER']; +$return = validate_redirect_url($_SERVER['HTTP_REFERER'] ?? '', 'index.php'); - if (basename($return) != 'auth_changepassword.php') { - if (str_contains($return, '/plugins/')) { - $parts = explode('/plugins/', $return); - $return = CACTI_PATH_URL . 'plugins/' . $parts[1]; - } else { - $return = CACTI_PATH_URL . basename($return); - } - - $_SESSION['acp_return'] = $return; +if (basename($return) != 'auth_changepassword.php') { + if (strpos($return, '/plugins/') !== false) { + $parts = explode('/plugins/', $return); + $return = CACTI_PATH_URL . 'plugins/' . $parts[1]; } else { - if (isset($_SESSION['acp_return'])) { - $return = $_SESSION['acp_return']; - } else { - $return = CACTI_PATH_URL . 'index.php'; - } + $return = CACTI_PATH_URL . basename($return); } + + $_SESSION['acp_return'] = $return; } else { - $return = CACTI_PATH_URL . 'index.php'; + if (isset($_SESSION['acp_return'])) { + $return = $_SESSION['acp_return']; + } else { + $return = CACTI_PATH_URL . 'index.php'; + } } $user = db_fetch_row_prepared('SELECT * @@ -89,13 +86,9 @@ if (!cacti_sizeof($user) || $user['realm'] != 0) { raise_message('nodomainpassword'); } - if (isset($_SERVER['HTTP_REFERER'])) { - // global.php already sanitized HTTP_REFERER; validate host before redirect. - $_ref = $_SERVER['HTTP_REFERER']; - header('Location: ' . ((parse_url($_ref, PHP_URL_HOST) === null || parse_url($_ref, PHP_URL_HOST) === $_SERVER['HTTP_HOST']) ? $_ref : 'index.php')); - } else { - header('Location: index.php'); - } + $return = validate_redirect_url($_SERVER['HTTP_REFERER'] ?? '', 'index.php'); + + header("Location: $return"); exit; } @@ -107,13 +100,9 @@ if ($user['password_change'] != 'on') { kill_session_var(SESS_USER_ID); cacti_cookie_logout(); - if (isset($_SERVER['HTTP_REFERER'])) { - // global.php already sanitized HTTP_REFERER; validate host before redirect. - $_ref = $_SERVER['HTTP_REFERER']; - header('Location: ' . ((parse_url($_ref, PHP_URL_HOST) === null || parse_url($_ref, PHP_URL_HOST) === $_SERVER['HTTP_HOST']) ? $_ref : 'index.php')); - } else { - header('Location: index.php'); - } + $return = validate_redirect_url($_SERVER['HTTP_REFERER'] ?? '', 'index.php'); + + header("Location: $return"); exit; } diff --git a/auth_profile.php b/auth_profile.php index 7a9ce14af..409af6bb1 100644 --- a/auth_profile.php +++ b/auth_profile.php @@ -305,20 +305,16 @@ function settings() : bool { return false; } - if (isset($_SERVER['HTTP_REFERER'])) { - $referer = $_SERVER['HTTP_REFERER']; + $referer = validate_redirect_url($_SERVER['HTTP_REFERER'] ?? '', 'graph_view.php'); - if (!str_contains($referer, 'auth_profile.php')) { - $timespan_sel_pos = strpos($referer, '&predefined_timespan'); + if (strpos($referer, 'auth_profile.php') === false) { + $timespan_sel_pos = strpos($referer, '&predefined_timespan'); - if ($timespan_sel_pos !== false) { - $referer = substr($referer, 0, $timespan_sel_pos); - } - - $_SESSION['profile_referer'] = $referer; + if ($timespan_sel_pos !== false) { + $referer = substr($referer, 0, $timespan_sel_pos); } - } elseif (!isset($_SESSION['profile_referer'])) { - $_SESSION['profile_referer'] = 'graph_view.php'; + + $_SESSION['profile_referer'] = $referer; } form_start('auth_profile.php', 'chk'); diff --git a/graphs_new.php b/graphs_new.php index 59cf9f87f..06e3e3463 100644 --- a/graphs_new.php +++ b/graphs_new.php @@ -1009,24 +1009,10 @@ function graphs() : void { form_hidden_box('host_template_id', $host['host_template_id'], '0'); } - if (isset($_SERVER['HTTP_REFERER']) && $_SERVER['HTTP_REFERER'] != '') { - $referer_url = parse_url($_SERVER['HTTP_REFERER']); - - if ($_SERVER['SERVER_NAME'] != $referer_url['host']) { - // Potential security exploit 1 - srv('returnto', 'host.php'); - } elseif (!str_contains($_SERVER['HTTP_REFERER'], 'graphs_new')) { - srv('returnto', basename($_SERVER['HTTP_REFERER'])); - } else { - srv('returnto', 'host.php'); - } - } elseif (isrv('returnto') && gnrv('returnto') != '') { - $returnto_url = parse_url(gnrv('returnto')); - - if ($_SERVER['SERVER_NAME'] != $returnto_url['host']) { - // Potential security exploit 2 - srv('returnto', 'host.php'); - } + if (isset($_SERVER['HTTP_REFERER'])) { + set_request_var('returnto', validate_redirect_url($_SERVER['HTTP_REFERER'], 'host.php')); + } elseif (isset_request_var('returnto')) { + set_request_var('returnto', validate_redirect_url(get_nfilter_request_var('returnto'), 'host.php')); } load_current_session_value('returnto', 'sess_grn_returnto', ''); diff --git a/include/auth.php b/include/auth.php index 73bb114c6..6347011a1 100644 --- a/include/auth.php +++ b/include/auth.php @@ -74,8 +74,7 @@ api_plugin_hook_function('auth_alternate_realms'); */ if ($auth_method != AUTH_METHOD_BASIC) { if (isset($_SESSION[SESS_CHANGE_PASSWORD])) { - header('Location: ' . CACTI_PATH_URL . 'auth_changepassword.php?ref=' . ($_SERVER['HTTP_REFERER'] ?? 'index.php')); - + header('Location: ' . CACTI_PATH_URL . 'auth_changepassword.php?ref=' . rawurlencode(validate_redirect_url($_SERVER['HTTP_REFERER'] ?? '', 'index.php'))); exit; } @@ -322,7 +321,7 @@ if (empty($_SESSION[SESS_USER_ID])) { } if (isset($_SERVER['HTTP_REFERER'])) { - $goBack = "[" . __('Return') . " | " . __('Login Again') . ']'; + $goBack = "[" . __('Return') . " | " . __('Login Again') . ']'; } elseif ($auth_method != AUTH_METHOD_BASIC) { $goBack = "[" . __('Login Again') . ']'; } else { diff --git a/lib/functions.php b/lib/functions.php index c9b25dfb4..236888e76 100644 --- a/lib/functions.php +++ b/lib/functions.php @@ -5046,9 +5046,34 @@ function sanitize_search_string(string $string) : string { * * @return string The sanitized uri */ -function sanitize_uri($uri) { - static $drop_char_match = ['^', '$', '<', '>', '`', "'", '"', '|', '+', '[', ']', '{', '}', ';', '!', '(', ')']; - static $drop_char_replace = [ '', '', '', '', '', '', '', '', '', '', '', '', '', '', '']; +function sanitize_uri(string $uri) : string { + static $drop_char_match = [ + '^', '$', + '<', '>', + '`', "'", + '"', '|', + '+', '[', + ']', '{', + '}', ';', + '!', '(', + ')' + ]; + + static $drop_char_replace = [ + '', '', + '', '', + '', '', + '', '', + '', '', + '', '', + '', '', + '', '', + '' + ]; + + if (is_urlencoded($uri)) { + $uri = urldecode($uri); + } if (str_contains($uri, 'graph_view.php')) { if (!strpos($uri, 'action=')) { @@ -5056,7 +5081,22 @@ function sanitize_uri($uri) { } } - return str_replace($drop_char_match, $drop_char_replace, strip_tags(urldecode($uri))); + return str_replace($drop_char_match, $drop_char_replace, strip_tags($uri)); +} + +/** + * Checks to see if a string is urlencoded + * + * @param string $string the string to be validated + * + * @return boolean - true is the string is urlencoded otherwise false + */ +function is_urlencoded(string $string) : bool { + if ($string != urldecode($string)) { + return true; + } else { + return false; + } } /** @@ -5066,7 +5106,7 @@ function sanitize_uri($uri) { * * @return bool true is the string is base64 otherwise false */ -function is_base64_encoded($data) { +function is_base64_encoded(string $data) : bool { // Perform a simple check first if (!preg_match('/^[a-zA-Z0-9\/\r\n+]*={0,2}$/', $data)) { return false; diff --git a/lib/html_utility.php b/lib/html_utility.php index 24e39d268..aaaa4ec2e 100644 --- a/lib/html_utility.php +++ b/lib/html_utility.php @@ -1250,7 +1250,7 @@ function validate_store_request_vars(array $filters, string $sess_prefix = '') : * @return void */ function update_order_string(bool $inplace = false) : void { - $page = get_order_string_page(); + $page = get_order_string_page(false); $order = ''; @@ -1351,19 +1351,37 @@ function update_order_string(bool $inplace = false) : void { * @return string The generated ORDER BY clause. */ function get_order_string() : string { - $page = get_order_string_page(); + $page = get_order_string_page(true); + $sort_column = get_nfilter_request_var('sort_column'); + $sort_dir = strtoupper(get_nfilter_request_var('sort_direction')); - if (!str_contains(get_request_var('sort_column'), '(') && !str_contains(get_request_var('sort_column'), '`')) { + if ($sort_dir !== 'ASC' && $sort_dir !== 'DESC') { + $sort_dir = 'ASC'; + } + + if (strpos($sort_column, '(') === false && strpos($sort_column, '`') === false) { $del = '`'; } else { $del = ''; } + /** + * Allowlist: identifiers are word chars and dots only; anything else could escape + * backtick quoting. Validate direction to prevent SQL keyword injection. + */ + if (!preg_match('/^[a-zA-Z][a-zA-Z0-9_.()]*$/', $sort_column)) { + $sort_column = ''; + } + if (isset($_SESSION['sort_string'][$page])) { return $_SESSION['sort_string'][$page]; - } else { - return 'ORDER BY ' . $del . implode($del . '.' . $del, explode('.', get_request_var('sort_column'))) . $del . ' ' . get_request_var('sort_direction'); } + + if ($sort_column != '') { + return 'ORDER BY ' . $del . implode($del . '.' . $del, explode('.', $sort_column)) . $del . ' ' . $sort_dir; + } + + return ''; } /** @@ -1378,7 +1396,7 @@ function get_order_string() : string { * @return void */ function remove_column_from_order_string(string $column) : void { - $page = get_order_string_page(); + $page = get_order_string_page(false); if (isset($_SESSION['sort_data'][$page][$column])) { unset($_SESSION['sort_data'][$page][$column]); @@ -1394,9 +1412,11 @@ function remove_column_from_order_string(string $column) : void { * such as 'action' and 'tab'. The page count is incremented with each call to * ensure uniqueness. * + * @param bool $increment Increment the page counter if true + * * @return string A unique order string for the current page. */ -function get_order_string_page() : string { +function get_order_string_page(bool $increment = true) : string { static $page_count = 0; $page = $page_count . '_' . str_replace('.php', '', get_current_page()); @@ -1409,11 +1429,83 @@ function get_order_string_page() : string { $page .= '_' . get_nfilter_request_var('tab'); } - $page_count++; + if ($increment == true) { + $page_count++; + } return $page; } +/** + * Validate the redirect url provider by the HTTP_REFERER from PHP + * + * @param string $url The regular expression to validate. + * @param string $default The URL to travel to upon failure + * + * @return string The validated URL, or the provided $default if invalid + */ +function validate_redirect_url($url = '', $default = 'index.php') { + if ($url === '') { + return $default; + } + + $url = trim($url); + + // Decode the url to make it readable if encoded + if (is_urlencoded($url)) { + $url = urldecode($url); + } + + // reject URLs with protocol schemes (external redirects, javascript:, data:) + $bad_strings = [ + 'javascript:', + 'data:', + 'vbscript:', + 'mailto:', + 'file:' + ]; + + foreach ($bad_strings as $bstring) { + if (stripos($url, $bstring) !== false) { + return $default; + } + } + + // reject protocol-relative URLs + if (strpos($url, '//') === 0) { + return $default; + } + + // reject URLs with newlines (header injection) + if (preg_match('/[\r\n]/', $url)) { + return $default; + } + + // reject path traversal sequences + if (stripos($url, '..') !== false) { + return $default; + } + + // Prevent referring off site + $ref_host = parse_url($url, PHP_URL_HOST); + $srv_host = null; + + if (isset($_SERVER['HTTP_HOST']) && $_SERVER['HTTP_HOST'] != '') { + $srv_host = preg_replace('/:\d+$/', '', $_SERVER['HTTP_HOST']); + } + + if ($ref_host === null || ($srv_host !== null && $ref_host === $srv_host)) { + $ref_path = parse_url($url, PHP_URL_PATH) ?: ''; + $ref_query = parse_url($url, PHP_URL_QUERY); + + $safe = sanitize_uri($ref_path . ($ref_query !== null ? '?' . $ref_query : '')); + + return $safe; + } else { + return $default; + } +} + /** * Validates if the given string is a valid regular expression. * diff --git a/link.php b/link.php index 9ee1c6795..1d4f22398 100644 --- a/link.php +++ b/link.php @@ -32,24 +32,10 @@ $page = db_fetch_row_prepared('SELECT // Prevent redirect loops if (isset($_SERVER['HTTP_REFERER'])) { - if (!str_contains($_SERVER['HTTP_REFERER'], 'link.php')) { - /* Reject non-http(s) schemes (javascript:, data:, etc.) before any - * host comparison; parse_url returns null for host on those schemes, - * which would otherwise pass the === null branch unchanged. Compare - * against SERVER_NAME (web-server config) rather than HTTP_HOST - * (attacker-supplied request header) to prevent host-spoofing. - * parse_url() returns false on a completely malformed URL; false fails - * all === comparisons below, so a malformed referer falls back to - * 'index.php'. null means the component is absent (e.g. relative path), - * which is allowed through as a same-host local referer. */ - $raw = $_SERVER['HTTP_REFERER']; - $scheme = parse_url($raw, PHP_URL_SCHEME); - $host = parse_url($raw, PHP_URL_HOST); - $referer = ( - ($scheme === null || $scheme === 'http' || $scheme === 'https') && - ($host === null || $host === ($_SERVER['SERVER_NAME'] ?? '')) - ) ? sanitize_uri($raw) : 'index.php'; - $_SESSION['link_referer'] = $referer; + if (strpos($_SERVER['HTTP_REFERER'], 'link.php') === false) { + $_SESSION['link_referer'] = validate_redirect_url($_SERVER['HTTP_REFERER'], 'index.php'); + + $referer = $_SESSION['link_referer']; } elseif (isset($_SESSION['link_referer'])) { $referer = sanitize_uri($_SESSION['link_referer']); } else { diff --git a/permission_denied.php b/permission_denied.php index bcb743fbe..9d78e153f 100644 --- a/permission_denied.php +++ b/permission_denied.php @@ -27,9 +27,10 @@ require('./include/auth.php'); $version = CACTI_VERSION_TEXT; if (isset($_SERVER['HTTP_REFERER'])) { - $goBack = "[" . __('Return') . " | " . __('Login Again') . ']'; + $redirect = validate_redirect_url($_SERVER['HTTP_REFERER'], 'index.php'); + $goBack = "[" . __('Return') . " | " . __('Login Again') . ']'; } else { - $goBack = "[" . __('Return') . " | " . __('Login Again') . ']'; + $goBack = "[" . __('Return') . " | " . __('Login Again') . ']'; } // allow for plugin based permission denied page diff --git a/remote_agent.php b/remote_agent.php index 0ae4935bd..9365c05a0 100644 --- a/remote_agent.php +++ b/remote_agent.php @@ -284,6 +284,7 @@ function get_snmp_data() : void { if (!is_string($oid) || !preg_match('/^[0-9.]+$/', $oid)) { print 'U'; + return; } @@ -313,6 +314,7 @@ function get_snmp_data_walk() : void { if (!is_string($oid) || !preg_match('/^[0-9.]+$/', $oid)) { print 'U'; + return; }