Fix cross-site scripting (XSS) vulnerability in setting Content-Type/…

…Content-Disposition for attachment preview/download Thanks to rehme.infosec for reporting the issues.
roundcube · Nov 6, 2023 · bf599fe · bf599fe
1 parent 24df766
commit bf599fe
Show file tree

Hide file tree

Showing 6 changed files with 97 additions and 23 deletions.
diff --git a/CHANGELOG b/CHANGELOG
@@ -1,5 +1,7 @@
 # Changelog Roundcube Webmail
 
+- Fix cross-site scripting (XSS) vulnerability in setting Content-Type/Content-Disposition for attachment preview/download
+
 RELEASE 1.4.15
 --------------
 - Fix cross-site scripting (XSS) vulnerability in handling of SVG in HTML messages (#9168)

diff --git a/program/lib/Roundcube/rcube_charset.php b/program/lib/Roundcube/rcube_charset.php
@@ -182,6 +182,18 @@ public static function error_handler($errno, $errstr)
         throw new ErrorException($errstr, 0, $errno);
     }
 
+    /**
+     * Validate character set identifier.
+     *
+     * @param string $input Character set identifier
+     *
+     * @return bool True if valid, False if not valid
+     */
+    public static function is_valid($input)
+    {
+        return is_string($input) && preg_match('|^[a-zA-Z0-9_./:#-]{2,32}$|', $input) > 0;
+    }
+
     /**
      * Parse and validate charset name string.
      * Sometimes charset string is malformed, there are also charset aliases,

diff --git a/program/lib/Roundcube/rcube_imap.php b/program/lib/Roundcube/rcube_imap.php
@@ -2129,6 +2129,11 @@ protected function structure_part($part, $count = 0, $parent = '', $mime_headers
             $struct->charset = $mime_headers->charset;
         }
 
+        // Sanitize charset for security
+        if ($struct->charset && !rcube_charset::is_valid($struct->charset)) {
+            $struct->charset = '';
+        }
+
         // read content encoding
         if (!empty($part[5])) {
             $struct->encoding = strtolower($part[5]);

diff --git a/program/lib/Roundcube/rcube_output.php b/program/lib/Roundcube/rcube_output.php
@@ -212,7 +212,7 @@ public function common_headers($privacy = true)
     }
 
     /**
-     * Send headers related to file downloads
+     * Send headers related to file downloads.
      *
      * @param string $filename File name
      * @param array  $params   Optional parameters:
@@ -225,31 +225,54 @@ public function common_headers($privacy = true)
      */
     public function download_headers($filename, $params = array())
     {
+        // For security reasons we validate type, filename and charset params.
+        // Some HTTP servers might drop a header that is malformed or very long, this then
+        // can lead to web browsers unintentionally executing javascript code in the body.
+
         if (empty($params['disposition'])) {
             $params['disposition'] = 'attachment';
         }
 
-        if ($params['disposition'] == 'inline' && stripos($params['type'], 'text') === 0) {
-            $params['type'] .= '; charset=' . ($params['type_charset'] ?: $this->charset);
-        }
-
-        header("Content-Type: " . ($params['type'] ?: "application/octet-stream"));
+        $ctype       = 'application/octet-stream';
+        $disposition = $params['disposition'];
 
-        if ($params['disposition'] == 'attachment' && $this->browser->ie) {
-            header("Content-Type: application/force-download");
+        if (!empty($params['type']) && is_string($params['type']) && strlen($params['type']) < 256
+            && preg_match('/^[a-z0-9!#$&.+^_-]+\/[a-z0-9!#$&.+^_-]+$/i', $params['type'])
+        ) {
+            $ctype = $params['type'];
         }
 
-        $disposition = "Content-Disposition: " . $params['disposition'];
+        if ($disposition == 'inline' && stripos($ctype, 'text') === 0) {
+            $charset = $this->charset;
+            if (!empty($params['type_charset']) && rcube_charset::is_valid($params['type_charset'])) {
+                $charset = $params['type_charset'];
+            }
 
-        // For non-ascii characters we'll use RFC2231 syntax
-        if (!preg_match('/[^a-zA-Z0-9_.:,?;@+ -]/', $filename)) {
-            $disposition .= sprintf("; filename=\"%s\"", $filename);
+            $ctype .= "; charset={$charset}";
         }
-        else {
-            $disposition .= sprintf("; filename*=%s''%s", $params['charset'] ?: $this->charset, rawurlencode($filename));
+
+        if (is_string($filename) && strlen($filename) > 0 && strlen($filename) <= 1024) {
+            // For non-ascii characters we'll use RFC2231 syntax
+            if (!preg_match('/[^a-zA-Z0-9_.:,?;@+ -]/', $filename)) {
+                $disposition .= "; filename=\"{$filename}\"";
+            }
+            else {
+                $filename = rawurlencode($filename);
+                $charset  = $this->charset;
+                if (!empty($params['charset']) && rcube_charset::is_valid($params['charset'])) {
+                    $charset = $params['charset'];
+                }
+
+                $disposition .= "; filename*={$charset}''{$filename}";
+            }
         }
 
-        header($disposition);
+        header("Content-Disposition: {$disposition}");
+        header("Content-Type: {$ctype}");
+
+        if ($params['disposition'] == 'attachment' && $this->browser->ie) {
+            header("Content-Type: application/force-download");
+        }
 
         if (isset($params['length'])) {
             header("Content-Length: " . $params['length']);

diff --git a/program/steps/mail/viewsource.inc b/program/steps/mail/viewsource.inc
@@ -34,23 +34,27 @@ if ($uid = rcube_utils::get_input_value('_uid', rcube_utils::INPUT_GET)) {
         $headers = $RCMAIL->storage->get_message_headers($uid);
     }
 
-    $charset = $headers->charset ?: $RCMAIL->config->get('default_charset');
+    $charset  = $headers->charset ?: $RCMAIL->config->get('default_charset');
+    $filename = '';
+    $params   = array(
+        'type'         => 'text/plain',
+        'type_charset' => $charset,
+    );
 
     if (!empty($_GET['_save'])) {
         $subject  = rcube_mime::decode_header($headers->subject, $headers->charset);
         $filename = rcmail_filename_from_subject(mb_substr($subject, 0, 128));
         $filename = ($filename ?: $uid)  . '.eml';
 
-        $RCMAIL->output->download_headers($filename, array(
-                'length'       => $headers->size,
-                'type'         => 'text/plain',
-                'type_charset' => $charset,
-        ));
+        $params['length'] = $headers->size;
+        $params['disposition'] = 'attachment';
     }
     else {
-        header("Content-Type: text/plain; charset={$charset}");
+        $params['disposition'] = 'inline';
     }
 
+    $RCMAIL->output->download_headers($filename, $params);
+
     if (isset($message)) {
         $message->get_part_body($part_id, empty($_GET['_save']), 0, -1);
     }

diff --git a/tests/Framework/Charset.php b/tests/Framework/Charset.php
@@ -9,7 +9,6 @@
  */
 class Framework_Charset extends PHPUnit_Framework_TestCase
 {
-
     /**
      * Data for test_clean()
      */
@@ -40,6 +39,35 @@ function test_clean_2()
         $this->assertNotRegExp('/\xD0\xD0/', rcube_charset::clean($bogus));
     }
 
+    /**
+     * Data for test_is_valid()
+     */
+    function data_is_valid()
+    {
+        $list = [];
+        foreach (mb_list_encodings() as $charset) {
+            $list[] = [$charset, true];
+        }
+
+        return array_merge($list, [
+            ['', false],
+            ['a', false],
+            ['aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', false],
+            [null, false],
+
+            ['TCVN5712-1:1993', true],
+            ['JUS_I.B1.002', true],
+        ]);
+    }
+
+    /**
+     * @dataProvider data_is_valid
+     */
+    function test_is_valid($input, $result)
+    {
+        $this->assertSame($result, rcube_charset::is_valid($input));
+    }
+
     /**
      * Data for test_parse_charset()
      */