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 4, 2023 · 81ac3c3 · 81ac3c3
1 parent 57a599a
commit 81ac3c3
Show file tree

Hide file tree

Showing 6 changed files with 95 additions and 25 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,7 @@
 - Fix bug where images attached to application/smil messages weren't displayed (#8870)
 - Fix PHP string replacement error in utils/error.php (#9185)
 - Fix regression where `smtp_user` did not allow pre/post strings before/after `%u` placeholder (#9162)
+- Fix cross-site scripting (XSS) vulnerability in setting Content-Type/Content-Disposition for attachment preview/download
 
 ## Release 1.6.4
 

diff --git a/program/actions/mail/viewsource.php b/program/actions/mail/viewsource.php
@@ -45,26 +45,30 @@ public function run($args = [])
                 $headers = $rcmail->storage->get_message_headers($uid);
             }
 
-            $charset = $headers->charset ?: $rcmail->config->get('default_charset', RCUBE_CHARSET);
+            $charset  = $headers->charset ?: $rcmail->config->get('default_charset', RCUBE_CHARSET);
+            $filename = '';
+            $params   = [
+                'type'         => 'text/plain',
+                'type_charset' => $charset,
+            ];
 
             if (!empty($_GET['_save'])) {
                 $subject  = rcube_mime::decode_header($headers->subject, $headers->charset);
                 $filename = self::filename_from_subject(mb_substr($subject, 0, 128));
                 $filename = ($filename ?: $uid)  . '.eml';
 
-                $rcmail->output->download_headers($filename, [
-                        'length'       => $headers->size,
-                        'type'         => 'text/plain',
-                        'type_charset' => $charset,
-                ]);
+                $params['length'] = $headers->size;
+                $params['disposition'] = 'attachment';
             }
             else {
                 // Make sure it works in an iframe (#9084)
                 $rcmail->output->page_headers();
 
-                header("Content-Type: text/plain; charset={$charset}");
+                $params['disposition'] = 'inline';
             }
 
+            $rcmail->output->download_headers($filename, $params);
+
             if (isset($part_id) && isset($message)) {
                 $message->get_part_body($part_id, empty($_GET['_save']), 0, -1);
             }

diff --git a/program/lib/Roundcube/rcube_charset.php b/program/lib/Roundcube/rcube_charset.php
@@ -178,6 +178,18 @@ class rcube_charset
         65001 => 'UTF-8',
     ];
 
+    /**
+     * 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
@@ -2163,6 +2163,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]) && !is_array($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,34 +225,54 @@ public function common_headers($privacy = true)
      */
     public function download_headers($filename, $params = [])
     {
+        // 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);
+        $ctype       = 'application/octet-stream';
+        $disposition = $params['disposition'];
+
+        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'];
         }
 
-        header("Content-Type: " . (!empty($params['type']) ? $params['type'] : "application/octet-stream"));
+        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'];
+            }
 
-        if ($params['disposition'] == 'attachment' && $this->browser->ie) {
-            header("Content-Type: application/force-download");
+            $ctype .= "; charset={$charset}";
         }
 
-        $disposition = "Content-Disposition: " . $params['disposition'];
+        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'];
+                }
 
-        // For non-ascii characters we'll use RFC2231 syntax
-        if (!preg_match('/[^a-zA-Z0-9_.:,?;@+ -]/', $filename)) {
-            $disposition .= sprintf("; filename=\"%s\"", $filename);
-        }
-        else {
-            $disposition .= sprintf("; filename*=%s''%s",
-                !empty($params['charset']) ? $params['charset'] : $this->charset,
-                rawurlencode($filename)
-            );
+                $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/tests/Framework/Charset.php b/tests/Framework/Charset.php
@@ -8,7 +8,6 @@
  */
 class Framework_Charset extends PHPUnit\Framework\TestCase
 {
-
     /**
      * Data for test_clean()
      */
@@ -33,6 +32,35 @@ function test_clean($input, $output)
         $this->assertSame($output, rcube_charset::clean($input));
     }
 
+    /**
+     * 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()
      */