From 81ac3c342a4f288deb275590895b52ec3785cf8a Mon Sep 17 00:00:00 2001 From: Aleksander Machniak 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.md | 1 + program/actions/mail/viewsource.php | 18 +++++---- program/lib/Roundcube/rcube_charset.php | 12 ++++++ program/lib/Roundcube/rcube_imap.php | 5 +++ program/lib/Roundcube/rcube_output.php | 54 +++++++++++++++++-------- tests/Framework/Charset.php | 30 +++++++++++++- 6 files changed, 95 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1d7f00da0a..eaeb6be831d 100644 --- 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 index 46c3d5f87eb..029128e3dec 100644 --- 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 index ce5f8141887..77a7c0ad209 100644 --- 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 index de815995efe..6e5d9a5204a 100644 --- 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 index b268bcad799..8aa2bdaf194 100644 --- 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 index 38e217c2e1a..2efc48f160c 100644 --- 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() */