Re: Wheezy update of roundcube
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?= <hertzog@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
Reply to: