CVE-2020-24881: Server-Side Request Forgery (SSRF) (CWE-918)
value: SSRF exists in osTicket before 1.14.3, where an attacker can add malicious file to server or perform port scanning.
CWE description: The web server receives a URL or similar request from an upstream component and retrieves the contents of this URL, but it does not sufficiently ensure that the request is being sent to the expected destination.
CVSS v3 base score: 9.8
Published: 2020-11-02T21:15Z
Repository: https://github.com/osTicket/osTicket
Language: PHP
Files changed: include/class.config.php, include/class.format.php, include/class.thread.php, include/i18n/en_US/config.yaml, include/i18n/en_US/help/tips/settings.ticket.yaml, include/staff/settings-tickets.inc.php
Commit date: 2020-08-19 09:22:56 -0500
Commit message: 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`).

Diff: diff --git a/include/class.config.php b/include/class.config.php
index d49c0de70..9448f5cf1 100644
--- a/include/class.config.php
+++ b/include/class.config.php
@@ -228,10 +228,11 @@ class OsticketConfig extends Config {
         'agent_avatar' => 'gravatar.mm',
         'ticket_lock' => 2, // Lock on activity
         'max_open_tickets' => 0,
         'files_req_auth' => 1,
         'force_https' => '',
+        'allow_external_images' => 1,
     );
 
     function __construct($section=null) {
         parent::__construct($section);
 
@@ -1033,10 +1034,14 @@ class OsticketConfig extends Config {
 
     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);
     }
 
     function hideStaffName() {
@@ -1415,10 +1420,11 @@ class OsticketConfig extends Config {
             'require_topic_to_close'=>isset($vars['require_topic_to_close'])?1:0,
             'show_related_tickets'=>isset($vars['show_related_tickets'])?1:0,
             '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,
         ));
     }
 
     function updateTasksSettings($vars, &$errors) {
         $f=array();
diff --git a/include/class.format.php b/include/class.format.php
index ba56f2bd9..66677381e 100644
--- a/include/class.format.php
+++ b/include/class.format.php
@@ -409,17 +409,33 @@ class Format {
         return Format::htmlchars($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);
 
         if ($balance)
             $text = self::html_balance($text, false);
@@ -431,10 +447,33 @@ class Format {
             return self::viewableImages($text);
 
         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))
             return array_map(array('Format','striptags'), $var, array_fill(0, count($var), $decode));
 
diff --git a/include/class.thread.php b/include/class.thread.php
index a8d5438d4..35d9aed58 100644
--- a/include/class.thread.php
+++ b/include/class.thread.php
@@ -1645,10 +1645,13 @@ implements TemplateVariable {
         }
 
         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;
 
         $entry = new static(array(
@@ -2888,11 +2891,11 @@ class HtmlThreadEntryBody extends ThreadEntryBody {
 
         switch ($output) {
         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']);
         }
     }
 }
diff --git a/include/i18n/en_US/config.yaml b/include/i18n/en_US/config.yaml
index c11e35dbe..7fb476505 100644
--- a/include/i18n/en_US/config.yaml
+++ b/include/i18n/en_US/config.yaml
@@ -78,10 +78,11 @@ core:
     overlimit_notice_active: 0
     email_attachments: 1
     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
     log_graceperiod: 12
     client_registration: 'public'
diff --git a/include/i18n/en_US/help/tips/settings.ticket.yaml b/include/i18n/en_US/help/tips/settings.ticket.yaml
index 09a8abeaa..dd07860d0 100644
--- a/include/i18n/en_US/help/tips/settings.ticket.yaml
+++ b/include/i18n/en_US/help/tips/settings.ticket.yaml
@@ -122,10 +122,19 @@ collaborator_ticket_visibility:
 require_topic_to_close:
     title: Require Help 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: >
         Enable this feature to exclude assigned tickets from the <span class="doc-desc-title">Open
         Tickets Queue</span>.
diff --git a/include/staff/settings-tickets.inc.php b/include/staff/settings-tickets.inc.php
index a73c4ada4..7d043a25d 100644
--- a/include/staff/settings-tickets.inc.php
+++ b/include/staff/settings-tickets.inc.php
@@ -232,10 +232,17 @@ if(!($maxfileuploads=ini_get('max_file_uploads')))
             <td>
                 <input type="checkbox" name="require_topic_to_close" <?php echo $config['require_topic_to_close']?'checked="checked"':''; ?>>
                 <?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>
             </th>
         </tr>

Vulnerable code:             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]);
            return Format::clickableurls($this->body);
Fixed code:         'allow_external_images' => 1,
    function allowExternalImages() {
        return ($this->get('allow_external_images'));
    }

            'allow_external_images'=>isset($vars['allow_external_images'])?1:0,
        global $cfg;

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

            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 '';
    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;
    }

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

            return Format::clickableurls(Format::stripExternalImages($this->body, true));
    allow_external_images: 1
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.

        <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>