File Disclosure Vulnerability

[david67810]

There is a zero-day file disclosure vulnerability in the latest version of roundcube webmail which currently is being exploited by hackers to read roundcube's configuration files and steal its database credentials. It requires a valid username/password to login to a roundcube installation.
I don't know how exactly should I report the details of this bug. Is there a proper way to privately send the details to roundcube's developers? Can I send the details to hello(at)roundcube(dot)net?

[alecpl]

You can send it to alec at alec.pl, can be encrypted with pgp key: 19359DC1. The address you mentioned would work too, but it might be faster to deal with it if you send the details to me. Thanks.

[david67810]

I sent the details to alec at alec.pl. Thanks.

[alecpl]

Thank you. I confirm the exploit works on every version since 1.1-beta.

[alecpl]

We're testing now a proper patch for this issue. The fix will be pushed tomorrow and the new releases should be available soon after that.

[alecpl]

Fixed in all release branches up to 1.0.

[thomascube]

CVE-2017-16651 - details will be published after release.

[rcsanchez97]

@alecpl @thomascube I am in the process of backporting the fix to version 0.7.2 (in Debian Wheezy). Since the code is somewhat different between the 1.x patches that are listed here in this issue, I would like to know the best to proceed. Would I be able to obtain the details of the exploit in order to reproduce the issue and validate my patch? Or would it be better for me to post my patch here or on the -dev mailing list for your comment?

[alecpl]

But why would you do this for 5-years old code?

The most important part of the patch is in rcmail.php, it fixes the discovered vulnerability. Other changes are to fix yet unknown attack vectors.

[rcsanchez97]

I am working on backporting this because I support the Debian LTS project: https://wiki.debian.org/LTS Debian Wheezy (which shipped roundcube 0.7.2) is supported until 31st May 2018. If the patch were too intrusive, it is possible we would have not attempted to fix this by backporting the patch. However, I believe that I have correctly adapted the patch and I would like to either try to replicate the issue or have an upstream developer review it to confirm that I have properly adapted it. Even if I can reproduce the exploit and then confirm for myself that I have adapted the patch properly, I would feel better about having it reviewed by an upstream developer. Should I post the patch here for review?

[alecpl]

Actually there's no way to reproduce the issue on 0.7, because program/steps/settings/upload.inc file does not exist there. So, 0.7 is not vulnerable. You should still probably apply the patch to prevent from yet unknown vulnerabilities, but there's no known way to test the vulnerability on 0.7.

[rcsanchez97]

That is good to know. Would you be willing to review this patch to see if the way I adapted makes sense?

[rcsanchez97]

Attaching it did not work. Perhaps this is better:

From 9be2224c779d7abc7b29eea2b83a8a3671c543e0 Mon Sep 17 00:00:00 2001
From: Aleksander Machniak <[email protected]>
Date: Wed, 8 Nov 2017 11:01:16 +0100
Subject: [PATCH] Fix file disclosure vulnerability caused by insuficient input
 validation in relation with attachment plugins (#6026)

---
 .../database_attachments/database_attachments.php  |  7 +++-
 .../filesystem_attachments.php                     | 47 ++++++++++++++++++++--
 program/include/rcmail.php                         |  5 ++-
 3 files changed, 53 insertions(+), 6 deletions(-)

--- roundcube.git.orig/plugins/database_attachments/database_attachments.php
+++ roundcube.git/plugins/database_attachments/database_attachments.php
@@ -74,8 +74,11 @@
         if ($args['path']) {
             $args['data'] = file_get_contents($args['path']);
 
-            if ($args['data'] === false)
+            if ($args['data'] === false) {
                 return $args;
+            }
+
+            $args['path'] = null;
         }
 
         $data = base64_encode($args['data']);
@@ -144,10 +147,14 @@
             $_SESSION['user_id'],
             $args['id']);
 
-        if ($sql_arr = $rcmail->db->fetch_assoc($sql_result)) {
+        $sql_arr = $rcmail->db->fetch_assoc($sql_result);
+        if ($sql_arr['data'] !== null && $sql_arr['data'] !== false) {
             $args['data'] = base64_decode($sql_arr['data']);
             $args['status'] = true;
         }
+        else {
+            $args['status'] = false;
+        }
 
         return $args;
     }
--- roundcube.git.orig/plugins/filesystem_attachments/filesystem_attachments.php
+++ roundcube.git/plugins/filesystem_attachments/filesystem_attachments.php
@@ -7,12 +7,19 @@
  * attachments of messages currently being composed, writing attachments
  * to disk when drafts with attachments are re-opened and writing
  * attachments to disk for inline display in current html compositions.
+ * It also handles uploaded files for other uses, so not only attachments.
  *
  * Developers may wish to extend this class when creating attachment
  * handler plugins:
  *   require_once('plugins/filesystem_attachments/filesystem_attachments.php');
  *   class myCustom_attachments extends filesystem_attachments
  *
+ * Note for developers: It is plugin's responsibility to care about security.
+ * So, e.g. if the plugin is asked about some file path it should check
+ * if it's really the storage path of the plugin and not e.g. /etc/passwd.
+ * It is done by setting 'status' flag on every plugin hook it uses.
+ * Roundcube core will trust the returned path if status=true.
+ *
  * @author Ziba Scott <[email protected]>
  * @author Thomas Bruederli <[email protected]>
  * 
@@ -104,7 +111,7 @@
      */
     function remove($args)
     {
-        $args['status'] = @unlink($args['path']);
+        $args['status'] = $this->verify_path($args['path']) && @unlink($args['path']);
         return $args;
     }
 
@@ -115,7 +122,7 @@
      */
     function display($args)
     {
-        $args['status'] = file_exists($args['path']);
+        $args['status'] = $this->verify_path($args['path']) && file_exists($args['path']);
         return $args;
     }
 
@@ -126,6 +133,10 @@
      */
     function get($args)
     {
+        if (!$this->verify_path($args['path'])) {
+            $args['path'] = null;
+        }
+
         return $args;
     }
 
@@ -141,8 +152,8 @@
             foreach ($_SESSION['plugins']['filesystem_attachments'] as $group => $files) {
                 if ($args['group'] && $args['group'] != $group)
                     continue;
-                foreach ((array)$files as $filename){
-                    if(file_exists($filename)){
+                foreach ((array)$files as $filename) {
+                    if (file_exists($filename)) {
                         unlink($filename);
                     }
                 }
@@ -158,4 +169,34 @@
 	    list($usec, $sec) = explode(' ', microtime()); 
         return preg_replace('/[^0-9]/', '', $userid . $sec . $usec);
     }
+
+    /**
+     * For security we'll always verify the file path stored in session,
+     * as session entries can be faked in various ways e.g. #6026.
+     * We allow only files in Roundcube temp dir
+     */
+    protected function verify_path($path)
+    {
+        if (empty($path)) {
+            return false;
+        }
+
+        $rcmail    = rcube::get_instance();
+        $temp_dir  = $rcmail->config->get('temp_dir');
+        $file_path = pathinfo($path, PATHINFO_DIRNAME);
+
+        if ($temp_dir !== $file_path) {
+            rcube::raise_error(array(
+                    'code'    => 403,
+                    'file'    => __FILE__,
+                    'line'    => __LINE__,
+                    'message' => sprintf("%s can't read %s (not in temp_dir)",
+                        $rcmail->get_user_name(), substr($path, 0, 512))
+                ), true, false);
+
+            return false;
+        }
+
+        return true;
+    }
 }
--- roundcube.git.orig/program/include/rcmail.php
+++ roundcube.git/program/include/rcmail.php
@@ -894,10 +894,14 @@
       $_SESSION['password']  = $this->encrypt($pass);
       $_SESSION['login_time'] = mktime();
 
-      if (isset($_REQUEST['_timezone']) && $_REQUEST['_timezone'] != '_default_')
-        $_SESSION['timezone'] = floatval($_REQUEST['_timezone']);
-      if (isset($_REQUEST['_dstactive']) && $_REQUEST['_dstactive'] != '_default_')
-        $_SESSION['dst_active'] = intval($_REQUEST['_dstactive']);
+      $timezone = floatval($_REQUEST['_timezone']);
+      if ($timezone && is_string($timezone) && $timezone != '_default_') {
+        $_SESSION['timezone'] = $timezone;
+      }
+      $dst_active = intval($_REQUEST['_dstactive']);
+      if ($dst_active && is_string($dst_active) && $dst_active != '_default_') {
+        $_SESSION['dst_active'] = $dst_active;
+      }
 
       // force reloading complete list of subscribed mailboxes
       $this->imap->clear_cache('mailboxes', true);

[rcsanchez97]

@alecpl Have you been able to review my patch? I would like to make an upload and close this issue in Debian Wheezy, unless it would be better to skip the patch altogether and simply mark it as not affecting roundcube 0.7.2. Please advise.

[alecpl]

Looks good, but in the last part you should not remove floatval() use. Also you should test if sending mail with attachments works with the patch, to make sure it did not break anything.

[rcsanchez97]

Thanks! I appreciate the feedback. I will make the recommended adjustments to the patch, test the changes, and prepare an updated package for Debian.

[fijosh]

@rcsanchez97 just FYI, backporting this fix into 0.7.2 broke working with attachments completely.
It is not possible to add/remove attachments or even reply to emails with attachments.

Specifically in my case, it was caused by missing temp_dir - I had to manually create it in the install dir of Roundcube and assign privileges for apache user to write into it.

I am running Roundcube 0.7.2 and the temp_dir simply did not exist at all (maybe it was upgraded over the years from some older version where the temp_dir was not used, I don't really remember)

It took me some time to figure out that this is the problem, hope that someone will find this comment in case they face the same issue.

[rcsanchez97]

Thanks for the info. Debian wheezy LTS ended a few months ago and this version of roundcube is no longer in a supported Debian release. I am glad that you discovered a workaround and documented it here, as there will not be an update to correct this.

[fijosh]

Yeah, I know the support has ended, I just wanted to mention it for someone else who may face the problem.

For the record, the folder that I had to create was /usr/share/roundcube/temp (where /usr/share/roundcube is the installation directory of Roundcube, I believe this is the default one)

I just remembered that long time ago I have upgraded this Debian box from squeeze to wheezy, where I was also using roundcube. So maybe this problem is related to upgrade from older release(s) of Debian.
Who knows, not much we can do about it now (apart from working around it until I get myself to migrate to a more recent version :) )