Skip to content

Commit 5ec4968

Browse files
committed
Fix cross-site scripting (XSS) vulnerability in setting Content-Type/Content-Disposition for attachment preview/download
Thanks to rehme.infosec for reporting the issues.
1 parent 7ad7680 commit 5ec4968

File tree

6 files changed

+98
-25
lines changed

6 files changed

+98
-25
lines changed

Diff for: CHANGELOG.md

+4
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
## Unreleased
44

5+
- Fix cross-site scripting (XSS) vulnerability in setting Content-Type/Content-Disposition for attachment preview/download
6+
7+
## Release 1.5.5
8+
59
- Fix cross-site scripting (XSS) vulnerability in handling of SVG in HTML messages (#9168)
610

711
## Release 1.5.4

Diff for: program/actions/mail/viewsource.php

+11-7
Original file line numberDiff line numberDiff line change
@@ -45,23 +45,27 @@ public function run($args = [])
4545
$headers = $rcmail->storage->get_message_headers($uid);
4646
}
4747

48-
$charset = $headers->charset ?: $rcmail->config->get('default_charset');
48+
$charset = $headers->charset ?: $rcmail->config->get('default_charset');
49+
$filename = '';
50+
$params = [
51+
'type' => 'text/plain',
52+
'type_charset' => $charset,
53+
];
4954

5055
if (!empty($_GET['_save'])) {
5156
$subject = rcube_mime::decode_header($headers->subject, $headers->charset);
5257
$filename = self::filename_from_subject(mb_substr($subject, 0, 128));
5358
$filename = ($filename ?: $uid) . '.eml';
5459

55-
$rcmail->output->download_headers($filename, [
56-
'length' => $headers->size,
57-
'type' => 'text/plain',
58-
'type_charset' => $charset,
59-
]);
60+
$params['length'] = $headers->size;
61+
$params['disposition'] = 'attachment';
6062
}
6163
else {
62-
header("Content-Type: text/plain; charset={$charset}");
64+
$params['disposition'] = 'inline';
6365
}
6466

67+
$rcmail->output->download_headers($filename, $params);
68+
6569
if (isset($part_id) && isset($message)) {
6670
$message->get_part_body($part_id, empty($_GET['_save']), 0, -1);
6771
}

Diff for: program/lib/Roundcube/rcube_charset.php

+12
Original file line numberDiff line numberDiff line change
@@ -177,6 +177,18 @@ class rcube_charset
177177
65001 => 'UTF-8',
178178
];
179179

180+
/**
181+
* Validate character set identifier.
182+
*
183+
* @param string $input Character set identifier
184+
*
185+
* @return bool True if valid, False if not valid
186+
*/
187+
public static function is_valid($input)
188+
{
189+
return is_string($input) && preg_match('|^[a-zA-Z0-9_./:#-]{2,32}$|', $input) > 0;
190+
}
191+
180192
/**
181193
* Parse and validate charset name string.
182194
* Sometimes charset string is malformed, there are also charset aliases,

Diff for: program/lib/Roundcube/rcube_imap.php

+5
Original file line numberDiff line numberDiff line change
@@ -2156,6 +2156,11 @@ protected function structure_part($part, $count = 0, $parent = '', $mime_headers
21562156
$struct->charset = $mime_headers->charset;
21572157
}
21582158

2159+
// Sanitize charset for security
2160+
if ($struct->charset && !rcube_charset::is_valid($struct->charset)) {
2161+
$struct->charset = '';
2162+
}
2163+
21592164
// read content encoding
21602165
if (!empty($part[5])) {
21612166
$struct->encoding = strtolower($part[5]);

Diff for: program/lib/Roundcube/rcube_output.php

+37-17
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ public function common_headers($privacy = true)
212212
}
213213

214214
/**
215-
* Send headers related to file downloads
215+
* Send headers related to file downloads.
216216
*
217217
* @param string $filename File name
218218
* @param array $params Optional parameters:
@@ -225,34 +225,54 @@ public function common_headers($privacy = true)
225225
*/
226226
public function download_headers($filename, $params = [])
227227
{
228+
// For security reasons we validate type, filename and charset params.
229+
// Some HTTP servers might drop a header that is malformed or very long, this then
230+
// can lead to web browsers unintentionally executing javascript code in the body.
231+
228232
if (empty($params['disposition'])) {
229233
$params['disposition'] = 'attachment';
230234
}
231235

232-
if ($params['disposition'] == 'inline' && stripos($params['type'], 'text') === 0) {
233-
$params['type'] .= '; charset=' . ($params['type_charset'] ?: $this->charset);
236+
$ctype = 'application/octet-stream';
237+
$disposition = $params['disposition'];
238+
239+
if (!empty($params['type']) && is_string($params['type']) && strlen($params['type']) < 256
240+
&& preg_match('/^[a-z0-9!#$&.+^_-]+\/[a-z0-9!#$&.+^_-]+$/i', $params['type'])
241+
) {
242+
$ctype = $params['type'];
234243
}
235244

236-
header("Content-Type: " . (!empty($params['type']) ? $params['type'] : "application/octet-stream"));
245+
if ($disposition == 'inline' && stripos($ctype, 'text') === 0) {
246+
$charset = $this->charset;
247+
if (!empty($params['type_charset']) && rcube_charset::is_valid($params['type_charset'])) {
248+
$charset = $params['type_charset'];
249+
}
237250

238-
if ($params['disposition'] == 'attachment' && $this->browser->ie) {
239-
header("Content-Type: application/force-download");
251+
$ctype .= "; charset={$charset}";
240252
}
241253

242-
$disposition = "Content-Disposition: " . $params['disposition'];
254+
if (is_string($filename) && strlen($filename) > 0 && strlen($filename) <= 1024) {
255+
// For non-ascii characters we'll use RFC2231 syntax
256+
if (!preg_match('/[^a-zA-Z0-9_.:,?;@+ -]/', $filename)) {
257+
$disposition .= "; filename=\"{$filename}\"";
258+
}
259+
else {
260+
$filename = rawurlencode($filename);
261+
$charset = $this->charset;
262+
if (!empty($params['charset']) && rcube_charset::is_valid($params['charset'])) {
263+
$charset = $params['charset'];
264+
}
243265

244-
// For non-ascii characters we'll use RFC2231 syntax
245-
if (!preg_match('/[^a-zA-Z0-9_.:,?;@+ -]/', $filename)) {
246-
$disposition .= sprintf("; filename=\"%s\"", $filename);
247-
}
248-
else {
249-
$disposition .= sprintf("; filename*=%s''%s",
250-
!empty($params['charset']) ? $params['charset'] : $this->charset,
251-
rawurlencode($filename)
252-
);
266+
$disposition .= "; filename*={$charset}''{$filename}";
267+
}
253268
}
254269

255-
header($disposition);
270+
header("Content-Disposition: {$disposition}");
271+
header("Content-Type: {$ctype}");
272+
273+
if ($params['disposition'] == 'attachment' && $this->browser->ie) {
274+
header("Content-Type: application/force-download");
275+
}
256276

257277
if (isset($params['length'])) {
258278
header("Content-Length: " . $params['length']);

Diff for: tests/Framework/Charset.php

+29-1
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
*/
99
class Framework_Charset extends PHPUnit\Framework\TestCase
1010
{
11-
1211
/**
1312
* Data for test_clean()
1413
*/
@@ -33,6 +32,35 @@ function test_clean($input, $output)
3332
$this->assertSame($output, rcube_charset::clean($input));
3433
}
3534

35+
/**
36+
* Data for test_is_valid()
37+
*/
38+
function data_is_valid()
39+
{
40+
$list = [];
41+
foreach (mb_list_encodings() as $charset) {
42+
$list[] = [$charset, true];
43+
}
44+
45+
return array_merge($list, [
46+
['', false],
47+
['a', false],
48+
['aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', false],
49+
[null, false],
50+
51+
['TCVN5712-1:1993', true],
52+
['JUS_I.B1.002', true],
53+
]);
54+
}
55+
56+
/**
57+
* @dataProvider data_is_valid
58+
*/
59+
function test_is_valid($input, $result)
60+
{
61+
$this->assertSame($result, rcube_charset::is_valid($input));
62+
}
63+
3664
/**
3765
* Data for test_parse_charset()
3866
*/

0 commit comments

Comments
 (0)