New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File Disclosure Vulnerability #6026

Closed
david67810 opened this Issue Nov 6, 2017 · 18 comments

Comments

Projects
None yet
5 participants
@david67810

david67810 commented Nov 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@alecpl

alecpl Nov 6, 2017

Member

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.

Member

alecpl commented Nov 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@david67810

david67810 Nov 6, 2017

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

david67810 commented Nov 6, 2017

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

@alecpl

This comment has been minimized.

Show comment
Hide comment
@alecpl

alecpl Nov 6, 2017

Member

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

Member

alecpl commented Nov 6, 2017

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

@alecpl alecpl added this to the 1.3.3 milestone Nov 6, 2017

@alecpl

This comment has been minimized.

Show comment
Hide comment
@alecpl

alecpl Nov 7, 2017

Member

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.

Member

alecpl commented Nov 7, 2017

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 added a commit that referenced this issue Nov 8, 2017

alecpl added a commit that referenced this issue Nov 8, 2017

alecpl added a commit that referenced this issue Nov 8, 2017

alecpl added a commit that referenced this issue Nov 8, 2017

alecpl added a commit that referenced this issue Nov 8, 2017

@alecpl

This comment has been minimized.

Show comment
Hide comment
@alecpl

alecpl Nov 8, 2017

Member

Fixed in all release branches up to 1.0.

Member

alecpl commented Nov 8, 2017

Fixed in all release branches up to 1.0.

@alecpl alecpl closed this Nov 8, 2017

@thomascube

This comment has been minimized.

Show comment
Hide comment
@thomascube

thomascube Nov 8, 2017

Member

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

Member

thomascube commented Nov 8, 2017

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

@rcsanchez97

This comment has been minimized.

Show comment
Hide comment
@rcsanchez97

rcsanchez97 Nov 18, 2017

@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?

rcsanchez97 commented Nov 18, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@alecpl

alecpl Nov 19, 2017

Member

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.

Member

alecpl commented Nov 19, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rcsanchez97

rcsanchez97 Nov 19, 2017

rcsanchez97 commented Nov 19, 2017

@alecpl

This comment has been minimized.

Show comment
Hide comment
@alecpl

alecpl Nov 20, 2017

Member

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.

Member

alecpl commented Nov 20, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rcsanchez97

rcsanchez97 Nov 20, 2017

rcsanchez97 commented Nov 20, 2017

@rcsanchez97

This comment has been minimized.

Show comment
Hide comment
@rcsanchez97

rcsanchez97 Nov 20, 2017

Attaching it did not work. Perhaps this is better:

From 9be2224c779d7abc7b29eea2b83a8a3671c543e0 Mon Sep 17 00:00:00 2001
From: Aleksander Machniak <alec@alec.pl>
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 <ziba@umich.edu>
  * @author Thomas Bruederli <roundcube@gmail.com>
  * 
@@ -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 commented Nov 20, 2017

Attaching it did not work. Perhaps this is better:

From 9be2224c779d7abc7b29eea2b83a8a3671c543e0 Mon Sep 17 00:00:00 2001
From: Aleksander Machniak <alec@alec.pl>
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 <ziba@umich.edu>
  * @author Thomas Bruederli <roundcube@gmail.com>
  * 
@@ -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

This comment has been minimized.

Show comment
Hide comment
@rcsanchez97

rcsanchez97 Nov 23, 2017

rcsanchez97 commented Nov 23, 2017

@alecpl

This comment has been minimized.

Show comment
Hide comment
@alecpl

alecpl Nov 23, 2017

Member

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.

Member

alecpl commented Nov 23, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rcsanchez97

rcsanchez97 Nov 23, 2017

rcsanchez97 commented Nov 23, 2017

@fijosh

This comment has been minimized.

Show comment
Hide comment
@fijosh

fijosh Sep 10, 2018

@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.

fijosh commented Sep 10, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@rcsanchez97

rcsanchez97 Sep 10, 2018

rcsanchez97 commented Sep 10, 2018

@fijosh

This comment has been minimized.

Show comment
Hide comment
@fijosh

fijosh Sep 10, 2018

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 :) )

fijosh commented Sep 10, 2018

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 :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment