From bda02002de8bbffbcc6200302fb5edcede916ddd Mon Sep 17 00:00:00 2001
From: Aleksander Machniak <alec@alec.pl>
Date: Sat, 30 May 2020 08:34:11 +0200
Subject: [PATCH] Security: Better fix for CVE-2020-12641

---
 CHANGELOG                             |  9 ++---
 program/lib/Roundcube/rcube_image.php | 50 ++++++++++++++++++++++-----
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index d9116cdd0..b5dcd6c76 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -36,6 +36,7 @@ CHANGELOG Roundcube Webmail
 - Fix bug where PDF attachments marked as inline could have not been attached on mail forward (#7382)
 - Security: Fix a couple of XSS issues in Installer (#7406)
 - Security: Fix XSS issue in template object 'username' (#7406)
+- Security: Better fix for CVE-2020-12641
 
 RELEASE 1.4.4
 -------------
@@ -63,10 +64,10 @@ RELEASE 1.4.4
 - Make install-jsdeps.sh script working without the 'file' program installed (#7325)
 - Fix performance issue of parsing big HTML messages by disabling HTML5 parser for these (#7331)
 - Fix so Print button for PDF attachments works on Firefox >= 75 (#5125)
-- Security: Fix XSS issue in handling of CDATA in HTML messages
-- Security: Fix remote code execution via crafted 'im_convert_path' or 'im_identify_path' settings
-- Security: Fix local file inclusion (and code execution) via crafted 'plugins' option
-- Security: Fix CSRF bypass that could be used to log out an authenticated user (#7302)
+- Security: Fix XSS issue in handling of CDATA in HTML messages [CVE-2020-12625]
+- Security: Fix remote code execution via crafted 'im_convert_path' or 'im_identify_path' settings [CVE-2020-12641]
+- Security: Fix local file inclusion (and code execution) via crafted 'plugins' option [CVE-2020-12640]
+- Security: Fix CSRF bypass that could be used to log out an authenticated user [CVE-2020-12626] (#7302)
 
 RELEASE 1.4.3
 -------------
diff --git a/program/lib/Roundcube/rcube_image.php b/program/lib/Roundcube/rcube_image.php
index df36a34db..0f00ce8ff 100644
--- a/program/lib/Roundcube/rcube_image.php
+++ b/program/lib/Roundcube/rcube_image.php
@@ -99,7 +99,7 @@ class rcube_image
     {
         $result  = false;
         $rcube   = rcube::get_instance();
-        $convert = $rcube->config->get('im_convert_path', false);
+        $convert = self::getCommand('im_convert_path');
         $props   = $this->props();
 
         if (empty($props)) {
@@ -158,7 +158,7 @@ class rcube_image
                             'size'    => $width . 'x' . $height,
                         );
 
-                        $result = rcube::exec(escapeshellcmd($convert)
+                        $result = rcube::exec($convert
                             . ' 2>&1 -flatten -auto-orient -colorspace sRGB -strip'
                             . ' -quality {quality} -resize {size} {intype}:{in} {type}:{out}', $p);
                     }
@@ -307,7 +307,7 @@ class rcube_image
     public function convert($type, $filename = null)
     {
         $rcube   = rcube::get_instance();
-        $convert = $rcube->config->get('im_convert_path', false);
+        $convert = self::getCommand('im_convert_path');
 
         if (!$filename) {
             $filename = $this->image_file;
@@ -324,8 +324,7 @@ class rcube_image
             $p['out']  = $filename;
             $p['type'] = self::$extensions[$type];
 
-            $result = rcube::exec(escapeshellcmd($convert)
-                . ' 2>&1 -colorspace sRGB -strip -flatten -quality 75 {in} {type}:{out}', $p);
+            $result = rcube::exec($convert . ' 2>&1 -colorspace sRGB -strip -flatten -quality 75 {in} {type}:{out}', $p);
 
             if ($result === '') {
                 chmod($filename, 0600);
@@ -408,7 +407,7 @@ class rcube_image
         $rcube = rcube::get_instance();
 
         // @TODO: check if specified mimetype is really supported
-        return class_exists('Imagick', false) || $rcube->config->get('im_convert_path');
+        return class_exists('Imagick', false) || self::getCommand('im_convert_path');
     }
 
     /**
@@ -419,9 +418,9 @@ class rcube_image
         $rcube = rcube::get_instance();
 
         // use ImageMagick in command line
-        if ($cmd = $rcube->config->get('im_identify_path')) {
+        if ($cmd = self::getCommand('im_identify_path')) {
             $args = array('in' => $this->image_file, 'format' => "%m %[fx:w] %[fx:h]");
-            $id   = rcube::exec(escapeshellcmd($cmd) . ' 2>/dev/null -format {format} {in}', $args);
+            $id   = rcube::exec($cmd . ' 2>/dev/null -format {format} {in}', $args);
 
             if ($id) {
                 return explode(' ', strtolower($id));
@@ -464,4 +463,39 @@ class rcube_image
         $size = $props['width'] * $props['height'] * $multip;
         return rcube_utils::mem_check($size);
     }
+
+    /**
+     * Get the configured command and make sure it is safe to use.
+     * We cannot trust configuration, and escapeshellcmd() is useless.
+     *
+     * @param string $opt_name Configuration option name
+     *
+     * @return bool|string The command or False if not set or invalid
+     */
+    private static function getCommand($opt_name)
+    {
+        static $error = [];
+
+        $cmd = rcube::get_instance()->config->get($opt_name);
+
+        if (empty($cmd)) {
+            return false;
+        }
+
+        if (preg_match('/^(convert|identify)(\.exe)?$/i', $cmd)) {
+            return $cmd;
+        }
+
+        // Executable must exist, also disallow network shares on Windows
+        if ($cmd[0] != "\\" && file_exists($cmd)) {
+            return $cmd;
+        }
+
+        if (empty($error[$opt_name])) {
+            rcube::raise_error("Invalid $opt_name: $cmd", true, false);
+            $error[$opt_name] = true;
+        }
+
+        return false;
+    }
 }