Hi Markus, On Wed, 20 Jul 2016, Markus Koschany wrote: > Feel free to work on everything you like. Fixing CVE-2014-9587 together > with CVE-2016-4069 isn't strictly required but you could probably reuse > some of your work if you try to tackle these issue. In any case the > whole CSRF complex requires much more work IMO and unless you are > already familiar with Roundcube and PHP it might not be the right > package to start with. It's up to you.
It was indeed a non-trivial amount of work... but the attached patch fixes CVE-2016-4069 according to my tests (i.e. downloads requests without _token do fail). On thursday I will see if I can deal with CVE-2014-9587 as well. Then there's https://security-tracker.debian.org/tracker/CVE-2016-4068 you left it open but it's mitigated since one cannot view SVG files. There is a patch available now (https://github.com/roundcube/roundcubemail/commit/a1fdb205f824dee7fd42dda739f207abc85ce158) but I'm not sure it's worth the effort of the backport. Because backporting this patch would also require backporting the real fix for https://security-tracker.debian.org/tracker/CVE-2015-8864 which is also rather involved. Thus I'm tempted to just mark the CVE-2016-4068 as fixed with your DLA-537-1. What do you think? I just spent 5 hours just for the attached patch... Cheers, -- Raphaël Hertzog ◈ Debian Developer Support Debian LTS: http://www.freexian.com/services/debian-lts.html Learn to master Debian: http://debian-handbook.info/get/
>From 3ed28d5dc43923fe8a495a77faa436178ec17919 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rapha=C3=ABl=20Hertzog?= <hert...@debian.org> Date: Tue, 6 Sep 2016 15:15:34 +0200 Subject: [PATCH] Fix CVE-2016-4069: CRSF forgery on download of attachments This is a backport of https://github.com/roundcube/roundcubemail/commit/699af1e5206ed9114322adaa3c25c1c969640a53 For the _token validation, it's a two step process. In index.php, we check that the token has the expected value in the GET request. The lack of the token does not generate an error at this level. The check for its presence is done only in the code of the action that actually require such a token. --- index.php | 4 ++++ plugins/managesieve/managesieve.js | 2 +- plugins/managesieve/managesieve.php | 5 +++++ program/include/main.inc | 4 ++-- program/include/rcmail.php | 4 +++- program/include/rcube_message.php | 2 +- program/include/rcube_shared.inc | 6 +++++- program/include/rcube_template.php | 7 ++++--- program/js/app.js.src | 20 ++++++++++++++------ program/steps/addressbook/export.inc | 5 +++++ program/steps/mail/func.inc | 4 ++-- program/steps/mail/get.inc | 6 ++++++ program/steps/mail/viewsource.inc | 6 +++++- 13 files changed, 57 insertions(+), 18 deletions(-) diff --git a/index.php b/index.php index 43a47f0..2e613a3 100644 --- a/index.php +++ b/index.php @@ -204,6 +204,10 @@ else { $OUTPUT->show_message('invalidrequest', 'error'); $OUTPUT->send($RCMAIL->task); } + else if (array_key_exists('_token', $_GET) && !$RCMAIL->check_request(RCUBE_INPUT_GET)) { + $OUTPUT->show_message('invalidrequest', 'error'); + $OUTPUT->send($RCMAIL->task); + } // check referer if configured if (!$request_check_whitelist[$RCMAIL->action] && $RCMAIL->config->get('referer_check') && !rcube_check_referer()) { diff --git a/plugins/managesieve/managesieve.js b/plugins/managesieve/managesieve.js index 1c6f2de..39866fa 100644 --- a/plugins/managesieve/managesieve.js +++ b/plugins/managesieve/managesieve.js @@ -183,7 +183,7 @@ rcube_webmail.prototype.managesieve_setget = function() var id = this.filtersets_list.get_single_selection(), script = this.env.filtersets[id]; - location.href = this.env.comm_path+'&_action=plugin.managesieve&_act=setget&_set='+urlencode(script); + this.goto_url('plugin.managesieve', {_act: 'setget', _set: script}, false, true); }; // Set activate/deactivate request diff --git a/plugins/managesieve/managesieve.php b/plugins/managesieve/managesieve.php index 9e2e8e1..f75e9fd 100644 --- a/plugins/managesieve/managesieve.php +++ b/plugins/managesieve/managesieve.php @@ -426,6 +426,11 @@ class managesieve extends rcube_plugin } } else if ($action == 'setget') { + if (! get_input_value('_token', RCUBE_INPUT_GET)) { + $this->rc->output->show_message('invalidrequest', 'error'); + $this->rc->output->send(); + exit; + } $script_name = get_input_value('_set', RCUBE_INPUT_GPC, true); $script = $this->sieve->get_script($script_name); diff --git a/program/include/main.inc b/program/include/main.inc index e2d6514..3acf9b6 100644 --- a/program/include/main.inc +++ b/program/include/main.inc @@ -124,10 +124,10 @@ function rcmail_overwrite_action($action) * @param string Request task (omit if the same) * @return The application URL */ -function rcmail_url($action, $p=array(), $task=null) +function rcmail_url($action, $p=array(), $task=null, $secure=false) { $app = rcmail::get_instance(); - return $app->url((array)$p + array('_action' => $action, 'task' => $task)); + return $app->url((array)$p + array('_action' => $action, 'task' => $task), $secure); } diff --git a/program/include/rcmail.php b/program/include/rcmail.php index 2a4cd78..6c9bb85 100644 --- a/program/include/rcmail.php +++ b/program/include/rcmail.php @@ -1442,7 +1442,7 @@ class rcmail * @param mixed Either a string with the action or url parameters as key-value pairs * @return string Valid application URL */ - public function url($p) + public function url($p, $secure = false) { if (!is_array($p)) $p = array('_action' => @func_get_arg(0)); @@ -1460,6 +1460,8 @@ class rcmail $delm = '&'; } } + if ($secure && ($token = $this->get_request_token())) + $url .= $delm . '_token=' . urlencode($token); return $url; } diff --git a/program/include/rcube_message.php b/program/include/rcube_message.php index ce46122..461a43d 100644 --- a/program/include/rcube_message.php +++ b/program/include/rcube_message.php @@ -90,7 +90,7 @@ class rcube_message 'safe' => $this->is_safe, 'prefer_html' => $this->app->config->get('prefer_html'), 'get_url' => rcmail_url('get', array( - '_mbox' => $this->imap->get_mailbox_name(), '_uid' => $uid)) + '_mbox' => $this->imap->get_mailbox_name(), '_uid' => $uid), true) ); if (!empty($this->headers->structure)) { diff --git a/program/include/rcube_shared.inc b/program/include/rcube_shared.inc index 089aab4..3ab7a67 100644 --- a/program/include/rcube_shared.inc +++ b/program/include/rcube_shared.inc @@ -32,7 +32,7 @@ */ function send_nocacheing_headers() { - global $OUTPUT; + global $OUTPUT, $RCMAIL; if (headers_sent()) return; @@ -41,6 +41,10 @@ function send_nocacheing_headers() header("Last-Modified: ".gmdate("D, d M Y H:i:s")." GMT"); // Request browser to disable DNS prefetching (CVE-2010-0464) header("X-DNS-Prefetch-Control: off"); + // send CSRF and clickjacking protection headers + if ($xframe = $RCMAIL->config->get('x_frame_options', 'sameorigin')) { + header('X-Frame-Options: ' . $xframe); + } // We need to set the following headers to make downloads work using IE in HTTPS mode. if ($OUTPUT->browser->ie && rcube_https_check()) { diff --git a/program/include/rcube_template.php b/program/include/rcube_template.php index 7fd3338..06d4030 100755 --- a/program/include/rcube_template.php +++ b/program/include/rcube_template.php @@ -369,10 +369,11 @@ class rcube_template extends rcube_html_page $js .= $this->get_js_commands() . ($this->framed ? ' }' : ''); $this->add_script($js, 'head_top'); - // send clickjacking protection headers + // allow (legal) iframe content to be loaded $iframe = $this->framed || !empty($_REQUEST['_framed']); - if (!headers_sent() && ($xframe = $this->app->config->get('x_frame_options', 'sameorigin'))) - header('X-Frame-Options: ' . ($iframe && $xframe == 'deny' ? 'sameorigin' : $xframe)); + if (!headers_sent() && $iframe && $this->app->config->get('x_frame_options', 'sameorigin') == 'deny') { + header('X-Frame-Options: sameorigin', true); + } // call super method parent::write($template, $this->config['skin_path']); diff --git a/program/js/app.js.src b/program/js/app.js.src index 5affa5e..1d50174 100644 --- a/program/js/app.js.src +++ b/program/js/app.js.src @@ -769,7 +769,7 @@ function rcube_webmail() } } - this.goto_url('get', qstring+'&_download=1', false); + this.goto_url('get', qstring+'&_download=1', false, true); break; case 'select-all': @@ -981,7 +981,7 @@ function rcube_webmail() case 'download': if (uid = this.get_single_uid()) - this.goto_url('viewsource', '&_uid='+uid+'&_mbox='+urlencode(this.env.mailbox)+'&_save=1'); + this.goto_url('viewsource', '&_uid='+uid+'&_mbox='+urlencode(this.env.mailbox)+'&_save=1', false, true); break; // quicksearch @@ -1034,7 +1034,7 @@ function rcube_webmail() case 'export': if (this.contact_list.rowcount > 0) { - this.goto_url('export', { _source:this.env.source, _gid:this.env.group, _search:this.env.search_request }); + this.goto_url('export', { _source:this.env.source, _gid:this.env.group, _search:this.env.search_request }, false, true); } break; @@ -1198,6 +1198,12 @@ function rcube_webmail() return url + '?' + name + '=' + value; }; + // append CSRF protection token to the given url + this.secure_url = function(url) + { + return this.add_url(url, '_token', this.env.request_token); + }, + this.is_framed = function() { return (this.env.framed && parent.rcmail && parent.rcmail != this && parent.rcmail.command); @@ -4742,7 +4748,7 @@ function rcube_webmail() id = this.env.iid ? this.env.iid : selection[0]; // append token to request - this.goto_url('delete-identity', '_iid='+id+'&_token='+this.env.request_token, true); + this.goto_url('delete-identity', '_iid='+id, true, true); return true; }; @@ -5796,9 +5802,11 @@ function rcube_webmail() this.location_href(url, window); }; - this.goto_url = function(action, query, lock) + this.goto_url = function(action, query, lock, secure) { - this.redirect(this.url(action, query)); + var url = this.url(action, query); + if (secure) url = this.secure_url(url); + this.redirect(url, lock); }; this.location_href = function(url, target, frame) diff --git a/program/steps/addressbook/export.inc b/program/steps/addressbook/export.inc index 988dabf..e801cae 100644 --- a/program/steps/addressbook/export.inc +++ b/program/steps/addressbook/export.inc @@ -20,6 +20,11 @@ $Id$ */ +if (!get_input_value('_token', RCUBE_INPUT_GET)) { + $RCMAIL->output->show_message('invalidrequest', 'error'); + $RCMAIL->output->send(); + exit; +} // Use search result if (!empty($_REQUEST['_search']) && isset($_SESSION['search'][$_REQUEST['_search']])) diff --git a/program/steps/mail/func.inc b/program/steps/mail/func.inc index 00c4d08..fc94d8d 100644 --- a/program/steps/mail/func.inc +++ b/program/steps/mail/func.inc @@ -1382,7 +1382,7 @@ function rcmail_draftinfo_decode($str) function rcmail_message_part_controls() { - global $MESSAGE; + global $MESSAGE, $RCMAIL; $part = asciiwords(get_input_value('_part', RCUBE_INPUT_GPC)); if (!is_object($MESSAGE) || !is_array($MESSAGE->parts) || !($_GET['_uid'] && $_GET['_part']) || !$MESSAGE->mime_parts[$part]) @@ -1394,7 +1394,7 @@ function rcmail_message_part_controls() if (!empty($part->filename)) { $table->add('title', Q(rcube_label('filename'))); $table->add(null, Q($part->filename)); - $table->add(null, '[' . html::a('?'.str_replace('_frame=', '_download=', $_SERVER['QUERY_STRING']), Q(rcube_label('download'))) . ']'); + $table->add(null, '[' . html::a('?'.str_replace('_frame=', '_download=', $_SERVER['QUERY_STRING']).'&_token='.$RCMAIL->get_request_token(), Q(rcube_label('download'))) . ']'); } if (!empty($part->size)) { diff --git a/program/steps/mail/get.inc b/program/steps/mail/get.inc index 2789d20..9f27bf5 100644 --- a/program/steps/mail/get.inc +++ b/program/steps/mail/get.inc @@ -84,6 +84,12 @@ else if ($pid = get_input_value('_part', RCUBE_INPUT_GET)) { if ($plugin['abort']) exit; + if ($plugin['download'] && !get_input_value('_token', RCUBE_INPUT_GET)) { + $RCMAIL->output->show_message('invalidrequest', 'error'); + $RCMAIL->output->send(); + exit; + } + // overwrite modified vars from plugin $mimetype = $plugin['mimetype']; list($ctype_primary, $ctype_secondary) = explode('/', $mimetype); diff --git a/program/steps/mail/viewsource.inc b/program/steps/mail/viewsource.inc index 62992cb..378de45 100644 --- a/program/steps/mail/viewsource.inc +++ b/program/steps/mail/viewsource.inc @@ -18,7 +18,11 @@ $Id: viewsource.inc 4410 2011-01-12 18:25:02Z thomasb $ */ - +if (!empty($_GET['_save']) && !get_input_value('_token', RCUBE_INPUT_GET)) { + $RCMAIL->output->show_message('invalidrequest', 'error'); + $RCMAIL->output->send(); + exit; +} ob_end_clean(); // similar code as in program/steps/mail/get.inc -- 2.9.3
