Skip to content
Permalink
Browse files Browse the repository at this point in the history
ssrf: External Inline Images
This mitigates an SSRF security vulnerability reported by [Talat
Mehmood](https://twitter.com/Blackbatsecuri1) where we do not check if the
src URL for external inline images contain a valid image extension when
displaying. This means if someone puts something like `<img
src="mymalicioussite.com/badscript.js">` in the body of their message, when
someone clicks Show Images or when the ticket is printed the server will
call the src URL to get the "image" data for display. This is a problem as
this could load malicious code/scripts in the browser. This adds a new
setting called `Allow External Images` and if Disabled we will not allow any
external images in Threads. If this setting is Enabled (default), we will
only allow `<img>` src URLs with valid image extensions
(`png`,`jpg`,`jpeg`,`gif`).
  • Loading branch information
JediKev committed Aug 19, 2020
1 parent d2491c1 commit d98c2d0
Show file tree
Hide file tree
Showing 6 changed files with 71 additions and 6 deletions.
6 changes: 6 additions & 0 deletions include/class.config.php
Expand Up @@ -230,6 +230,7 @@ class OsticketConfig extends Config {
'max_open_tickets' => 0,
'files_req_auth' => 1,
'force_https' => '',
'allow_external_images' => 1,
);

function __construct($section=null) {
Expand Down Expand Up @@ -1035,6 +1036,10 @@ function requireTopicToClose() {
return $this->get('require_topic_to_close');
}

function allowExternalImages() {
return ($this->get('allow_external_images'));
}

function getDefaultTicketQueueId() {
return $this->get('default_ticket_queue', 1);
}
Expand Down Expand Up @@ -1417,6 +1422,7 @@ function updateTicketsSettings($vars, &$errors) {
'allow_client_updates'=>isset($vars['allow_client_updates'])?1:0,
'ticket_lock' => $vars['ticket_lock'],
'default_ticket_queue'=>$vars['default_ticket_queue'],
'allow_external_images'=>isset($vars['allow_external_images'])?1:0,
));
}

Expand Down
49 changes: 44 additions & 5 deletions include/class.format.php
Expand Up @@ -411,13 +411,29 @@ function input($var) {

//Format text for display..
function display($text, $inline_images=true, $balance=true) {
global $cfg;

// Exclude external images?
$exclude = !$cfg->allowExternalImages();
// Allowed image extensions
$allowed = array('gif', 'png', 'jpg', 'jpeg');

// Make showing offsite images optional
$text = preg_replace_callback('/<img ([^>]*)(src="http[^"]+")([^>]*)\/>/',
function($match) {
// Drop embedded classes -- they don't refer to ours
$match = preg_replace('/class="[^"]*"/', '', $match);
return sprintf('<span %s class="non-local-image" data-%s %s></span>',
$match[1], $match[2], $match[3]);
function($match) use ($exclude, $allowed) {
$m = array();
// Split the src URL and get the extension
preg_match('/src="([^"]+)"/', $match[2], $m);
$url = explode('.', explode('?', $m[1])[0]);
$ext = preg_split('/[^A-Za-z]/', end($url))[0];

if (!$exclude && in_array($ext, $allowed)) {
// Drop embedded classes -- they don't refer to ours
$match = preg_replace('/class="[^"]*"/', '', $match);
return sprintf('<span %s class="non-local-image" data-%s %s></span>',
$match[1], $match[2], $match[3]);
} else
return '';
},
$text);

Expand All @@ -433,6 +449,29 @@ function($match) {
return $text;
}

function stripExternalImages($input, $display=false) {
global $cfg;

// Allowed Inline External Image Extensions
$allowed = array('gif', 'png', 'jpg', 'jpeg');
$exclude = !$cfg->allowExternalImages();

$input = preg_replace_callback('/<img ([^>]*)(src="([^"]+)")([^>]*)\/?>/',
function($match) use ($allowed, $exclude, $display) {
// Split the src URL and get the extension
$url = explode('.', explode('?', $match[3])[0]);
$ext = preg_split('/[^A-Za-z]/', end($url))[0];

if (($exclude && $display) || !in_array($ext, $allowed))
return '';
else
return $match[0];
},
$input);

return $input;
}

function striptags($var, $decode=true) {

if(is_array($var))
Expand Down
5 changes: 4 additions & 1 deletion include/class.thread.php
Expand Up @@ -1647,6 +1647,9 @@ static function create($vars=false) {
if (!($body = Format::strip_emoticons($vars['body']->getClean())))
$body = '-'; //Special tag used to signify empty message as stored.

// Ensure valid external images
$body = Format::stripExternalImages($body);

$poster = $vars['poster'];
if ($poster && is_object($poster))
$poster = (string) $poster;
Expand Down Expand Up @@ -2890,7 +2893,7 @@ function display($output=false) {
case 'email':
return $this->body;
case 'pdf':
return Format::clickableurls($this->body);
return Format::clickableurls(Format::stripExternalImages($this->body, true));
default:
return Format::display($this->body, true, !$this->options['balanced']);
}
Expand Down
1 change: 1 addition & 0 deletions include/i18n/en_US/config.yaml
Expand Up @@ -80,6 +80,7 @@ core:
ticket_number_format: '######'
ticket_sequence_id: 0
queue_bucket_counts: 0
allow_external_images: 1
task_number_format: '#'
task_sequence_id: 2
log_level: 2
Expand Down
9 changes: 9 additions & 0 deletions include/i18n/en_US/help/tips/settings.ticket.yaml
Expand Up @@ -124,6 +124,15 @@ require_topic_to_close:
content: >
If Enabled, a Ticket must have a Help Topic in order to be Closed by an Agent
allow_external_images:
title: Allow External Images
content: >
If Enabled, the system will allow external inline images that have a valid image
extension (.png, .jpg, .jpeg, .gif). If Disabled, the system will exclude
any external inline images. One caveat to note, is if the setting is Disabled we
will still store external inline images that have a valid image extension in case
the setting is re-enabled in the future.
assigned_tickets:
title: Assigned Tickets
content: >
Expand Down
7 changes: 7 additions & 0 deletions include/staff/settings-tickets.inc.php
Expand Up @@ -234,6 +234,13 @@
<?php echo __('Enable'); ?>&nbsp;<i class="help-tip icon-question-sign" href="#require_topic_to_close"></i>
</td>
</tr>
<tr>
<td><?php echo __('Allow External Images'); ?>:</td>
<td>
<input type="checkbox" name="allow_external_images" <?php echo $config['allow_external_images']?'checked="checked"':''; ?>>
<?php echo __('Enable'); ?>&nbsp;<i class="help-tip icon-question-sign" href="#allow_external_images"></i>
</td>
</tr>
<tr>
<th colspan="2">
<em><b><?php echo __('Attachments');?></b>: <?php echo __('Size and maximum uploads setting mainly apply to web tickets.');?></em>
Expand Down

1 comment on commit d98c2d0

@tr4l
Copy link

@tr4l tr4l commented on d98c2d0 Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

The issue with a SSRF is not that an attacker can load malicious script. The issue is that a request is made by the server. The attacker can still add an appropropiate extention and get internal IP of the server, or call an internal ES server, aws endpoint protected by firewall, sometime even a file from the filesystem of the server.

The filter in itself doesn't seems really robust neither (but didn't dig into it).
What happens with url like https://www.example.com/evil-script.js#legit.jpg ?

Please sign in to comment.