Request 3244 (superseded)

This request is superseded by request 3245 (Show diff)

You're not reviewing the full diff of request 3244 , but the diff to the superseded request 3243 (Show full diff)

Overview

Request 3244 (superseded)

1.4.15 with XSS patch

Created by mollekopf about 1 year ago
In state superseded
Supersedes 3243
Superseded by 3245
Open review for Admin
Open review for vanmeeuwen
Open review for mollekopf
Open review for kolab-developers

Submit roundcubemail

Submit package Kolab:16:Testing / roundcubemail to package Kolab:16 / roundcubemail

roundcubemail.spec Changed

0001-Fix-cross-site-scripting-XSS-vulnerability-in-settin.patch Added

@@ -0,0 +1,239 @@
+From bf599fe1cfbb9a6a13681524fd27e85aeb1f549a Mon Sep 17 00:00:00 2001
+From: Aleksander Machniak <alec@alec.pl>
+Date: Sat, 4 Nov 2023 17:52:00 +0100
+Subject: PATCH Fix cross-site scripting (XSS) vulnerability in setting
+ Content-Type/Content-Disposition for attachment preview/download
+
+Thanks to rehme.infosec for reporting the issues.
+---
+ CHANGELOG                               |  2 +
+ program/lib/Roundcube/rcube_charset.php | 12 ++++++
+ program/lib/Roundcube/rcube_imap.php    |  5 +++
+ program/lib/Roundcube/rcube_output.php  | 53 ++++++++++++++++++-------
+ program/steps/mail/viewsource.inc       | 18 +++++----
+ tests/Framework/Charset.php             | 30 +++++++++++++-
+ 6 files changed, 97 insertions(+), 23 deletions(-)
+
+diff --git a/CHANGELOG b/CHANGELOG
+index cab1c743d..ead3d106c 100644
+--- 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
+index 88eb6001f..1f69e118d 100644
+--- a/program/lib/Roundcube/rcube_charset.php
++++ b/program/lib/Roundcube/rcube_charset.php
+@@ -182,6 +182,18 @@ class rcube_charset
+         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
+index 714ba33f3..c1eebe1ba 100644
+--- a/program/lib/Roundcube/rcube_imap.php
++++ b/program/lib/Roundcube/rcube_imap.php
+@@ -2129,6 +2129,11 @@ class rcube_imap extends rcube_storage
+             $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($part5)) {
+             $struct->encoding = strtolower($part5);
+diff --git a/program/lib/Roundcube/rcube_output.php b/program/lib/Roundcube/rcube_output.php
+index 27acc7075..f24c47fda 100644
+--- a/program/lib/Roundcube/rcube_output.php
++++ b/program/lib/Roundcube/rcube_output.php
+@@ -212,7 +212,7 @@ abstract class rcube_output
+     }
+ 
+     /**
+-     * 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 @@ abstract class rcube_output
+      */
+     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
+index 532ea073b..91b1703e0 100644
+--- 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
+index f36b7b969..af6c4c783 100644
+--- 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 @@ class Framework_Charset extends PHPUnit_Framework_TestCase
+         $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()
+      */
+-- 
+2.41.0
+

debian.changelog Changed

debian.series Changed

roundcubemail.dsc Changed

Build Results
RPM Lint

Refresh

Comments for request 3244 0

Request History

mollekopf created request about 1 year ago

1.4.15 with XSS patch

mollekopf superseded request about 1 year ago

superseded by 3245