Skip to content

Commit

Permalink
security: Refresh URL
Browse files Browse the repository at this point in the history
This initially targeted a particular vulnerability with the Advanced
Search Refresh URLs but subsequently evolved into unifying all Refresh
URLs. We decided to create methods that properly encode URLs to prevent
code execution and to ensure they are all uniformly created. This adds
such capabilities and updates current Refresh URL generation to the new
methods. This also adds a method `Format::http_query_string()` that will
filter the query string and remove any unwanted params. Lastly, this
removes the old and tired `Misc::currentURL()` in favor of the new
"hip" (is that what the kids say these days?) method `Http::url()`.
  • Loading branch information
protich authored and JediKev committed Nov 8, 2022
1 parent 75e111c commit 3702a4f
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 48 deletions.
2 changes: 1 addition & 1 deletion bootstrap.php
Original file line number Diff line number Diff line change
Expand Up @@ -375,7 +375,7 @@ static function croak($message) {
Bootstrap::init();

#CURRENT EXECUTING SCRIPT.
define('THISPAGE', Misc::currentURL());
define('THISPAGE', Http::url());

define('DEFAULT_MAX_FILE_UPLOADS', ini_get('max_file_uploads') ?: 5);
define('DEFAULT_PRIORITY_ID', 1);
Expand Down
3 changes: 1 addition & 2 deletions file.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,7 @@

// Try and determine if an agent is viewing the page / file
if (strpos($_SERVER['HTTP_REFERRER'], ROOT_PATH . 'scp/') !== false) {
$_SESSION['_staff']['auth']['dest'] =
'/' . ltrim($_SERVER['REQUEST_URI'], '/');
$_SESSION['_staff']['auth']['dest'] = Http::refresh_url();
Http::redirect(ROOT_PATH.'scp/login.php');
} else {
require 'secure.inc.php';
Expand Down
8 changes: 8 additions & 0 deletions include/class.format.php
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,14 @@ static function htmldecode($var) {
return htmlspecialchars_decode($var, $flags);
}

static function http_query_string(string $query, array $filter = null) {
$args = [];
parse_str($query, $args);
if ($filter && is_array($filter))
$args = array_diff_key($args, array_flip($filter));
return http_build_query($args);
}

static function input($var) {
return Format::htmlchars($var);
}
Expand Down
43 changes: 43 additions & 0 deletions include/class.http.php
Original file line number Diff line number Diff line change
Expand Up @@ -151,5 +151,48 @@ static function build_query($vars, $encode=true, $separator='&') {

return http_build_query($vars, '', $separator);
}

// Get current url
static function url() {
// Scheme + Host.
$https = osTicket::is_https();
$url = sprintf('http%s://%s',
$https ? 's' : '',
$_SERVER['HTTP_HOST']);
// Add Port if not 80 and not https
$port = osTicket::get_client_port();
if ($port && $port != 80 && !$https && !str_contains($url, $port))
$url .= ":{$port}";
// Path + Query string
if (isset($_SERVER['REQUEST_URI']))
$url .= $_SERVER['REQUEST_URI'];
else
$url .= sprintf('%s?$s',
$_SERVER['PHP_SELF'], $_SERVER['QUERY_STRING']);

return $url;
}

// Parse path from given url or the current url
static function url_path(string $url = null, bool $htmlentities = true) {
$path = parse_url($url ?: self::url(), PHP_URL_PATH);
return ($htmlentities && $path)
? htmlentities($path) : $path;
}

// Parse query string from current url
static function query_string(string $url = null) {
return parse_url($url ?: self::url(), PHP_URL_QUERY);
}

// Refresh url
static function refresh_url(array $queryFilter = []) {
$url = self::url();
$refresh_url = self::url_path($url);
if (($qs=self::query_string($url)))
$refresh_url .= sprintf('?%s',
Format::http_query_string($qs, $queryFilter));
return $refresh_url;
}
}
?>
23 changes: 0 additions & 23 deletions include/class.misc.php
Original file line number Diff line number Diff line change
Expand Up @@ -226,29 +226,6 @@ static function date_range($period, $time=false, $tz=null) {
return (object) array('start' => $start, 'end' => $end);
}

//Current page
static function currentURL() {

$str = 'http';
if (isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] == 'on') {
$str .='s';
}
$str .= '://';
if (!isset($_SERVER['REQUEST_URI'])) { //IIS???
$_SERVER['REQUEST_URI'] = substr($_SERVER['PHP_SELF'],1 );
if (isset($_SERVER['QUERY_STRING'])) {
$_SERVER['REQUEST_URI'].='?'.$_SERVER['QUERY_STRING'];
}
}
if ($_SERVER['SERVER_PORT']!=80) {
$str .= $_SERVER['SERVER_NAME'].':'.$_SERVER['SERVER_PORT'].$_SERVER['REQUEST_URI'];
} else {
$str .= $_SERVER['SERVER_NAME'].$_SERVER['REQUEST_URI'];
}

return $str;
}

static function realpath($path) {
$rp = realpath($path);
return $rp ? $rp : $path;
Expand Down
32 changes: 32 additions & 0 deletions include/class.osticket.php
Original file line number Diff line number Diff line change
Expand Up @@ -520,6 +520,38 @@ static function get_root_path($dir) {
return null;
}

/*
* get_base_url
*
* Get base url osTicket is installed on
* It Should match help desk url.
*
*/
static function get_base_url() {
return sprintf('http%s://%s',
osTicket::is_https() ? 's' : '',
$_SERVER['HTTP_HOST'] . ROOT_PATH);
}

/*
* get_client_port
*
* Get client PORT from "Http_X-Forwarded-PORT" if we have trusted
* proxies set.
* FIXME: Follow trusted proxies chain
*
*/
static function get_client_port($header='HTTP_X_FORWARDED_PORT') {
$port = $_SERVER['SERVER_PORT'];
// We're just making sure we have Trusted Proxies
// FIXME: Validate
$proxies = self::getTrustedProxies();
if (isset($_SERVER[$header]) && $proxies)
$port = $_SERVER[$header];

return $port;
}

/*
* get_client_ip
*
Expand Down
2 changes: 1 addition & 1 deletion include/client/tickets.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@


<h1 style="margin:10px 0">
<a href="<?php echo Format::htmlchars($_SERVER['REQUEST_URI']); ?>"
<a href="<?php echo Html::refresh_url(); ?>"
><i class="refresh icon-refresh"></i>
<?php echo __('Tickets'); ?>
</a>
Expand Down
14 changes: 3 additions & 11 deletions include/staff/tasks.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,9 @@
// Make sure the cdata materialized view is available
TaskForm::ensureDynamicDataView();

// Figure out REFRESH url — which might not be accurate after posting a
// response
list($path,) = explode('?', $_SERVER['REQUEST_URI'], 2);
$args = array();
parse_str($_SERVER['QUERY_STRING'], $args);

// Remove commands from query
unset($args['id']);
unset($args['a']);

$refresh_url = htmlspecialchars($path) . '?' . http_build_query($args);
// Remove some variables from query string.
$qsFilter = ['id', 'a'];
$refresh_url = Http::refresh_url($qsFilter);

$sort_options = array(
'updated' => __('Most Recently Updated'),
Expand Down
14 changes: 5 additions & 9 deletions include/staff/templates/queue-tickets.tmpl.php
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,11 @@

// Figure out REFRESH url — which might not be accurate after posting a
// response
list($path,) = explode('?', $_SERVER['REQUEST_URI'], 2);
$args = array();
parse_str($_SERVER['QUERY_STRING'], $args);

// Remove commands from query
unset($args['id']);
if (isset($args['a']) && ($args['a'] !== 'search')) unset($args['a']);

$refresh_url = $path . '?' . http_build_query($args);
// Remove some variables from query string.
$qsFilter = ['id'];
if (isset($_REQUEST['a']) && ($_REQUEST['a'] !== 'search'))
$qsFilter[] = 'a';
$refresh_url = Http::refresh_url($qsFilter);

// Establish the selected or default sorting mechanism
if (isset($_GET['sort']) && is_numeric($_GET['sort'])) {
Expand Down
3 changes: 2 additions & 1 deletion main.inc.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,9 @@
Bootstrap::loadCode();
Bootstrap::connect();

#Global override
# Global Override when behind a proxy
$_SERVER['REMOTE_ADDR'] = osTicket::get_client_ip();
$_SERVER['SERVER_PORT'] = osTicket::get_client_port();

if(!($ost=osTicket::start()) || !($cfg = $ost->getConfig()))
Bootstrap::croak(__('Unable to load config info from DB.').' '.__('Get technical help!'));
Expand Down

0 comments on commit 3702a4f

Please sign in to comment.