Magento eCommerce PHP Remote Code Execution

Mattias Geniar, Tuesday, April 21, 2015

The fun just never ends. A remote code execution exploit was found on February 9th, 2015.

Checkpoint released a blogpost yesterday with more details on that particular vulnerability.

Check Point researchers recently discovered a critical RCE (remote code execution) vulnerability in the Magento web e-commerce platform that can lead to the complete compromise of any Magento-based store, including credit card information as well as other financial and personal data, affecting nearly two hundred thousand online shops.
Analyzing the Magento Vulnerability

The patch to the Remote Code Execution vulnerability is available on the Magento site; Magento Downloads, patch SUPEE-5344.

Yikes.

Magento's Open Source Community Policy

One very annoying part of the Open Source edition of Magento, is that the downloads available on the site do not contain the patches yet. You have to download the latest release, 1.9.1.0, and still download and apply every patch available.

It's very common for users to just download the latest release thinking that should be the up-to-date one, patches included. It boggles my mind why Magento would willingly distribute unsafe code this way, assuming users would just find out to download the patches separately.

Added to that is the fact that version numbers don't increase with the patches being applied. Seriously, it's 2015 Magento, get your act together. This is a very lame tactic to force your users to consider the commercially supported version.

The patch

If you're wondering if you should apply the patch to your Magento installation or note, let me answer this with a very clear yes:

The vulnerability is actually comprised of a chain of several vulnerabilities that ultimately allow an unauthenticated attacker to execute PHP code on the web server.

Since the patch is behind a very annoying login-wall, I've mirrored it here: PATCH_SUPEE-5344_CE_1.8.0.0_v1-2015-02-10-08-10-38.sh

The patch contains a bunch of whitespace, but the actual fix is this;

--- app/code/core/Mage/Admin/Model/Observer.php
+++ app/code/core/Mage/Admin/Model/Observer.php
@@ -43,6 +43,10 @@ class Mage_Admin_Model_Observer
     {
         $session = Mage::getSingleton('admin/session');
         /** @var $session Mage_Admin_Model_Session */
+
+        /**
+         * @var $request Mage_Core_Controller_Request_Http
+         */
         $request = Mage::app()->getRequest();
         $user = $session->getUser();
 
@@ -56,7 +60,7 @@ class Mage_Admin_Model_Observer
         if (in_array($requestedActionName, $openActions)) {
             $request->setDispatched(true);
         } else {
-            if($user) {
+            if ($user) {
                 $user->reload();
             }
             if (!$user || !$user->getId()) {
@@ -67,13 +71,14 @@ class Mage_Admin_Model_Observer
                     $user = $session->login($username, $password, $request);
                     $request->setPost('login', null);
                 }
-                if (!$request->getParam('forwarded')) {
+                if (!$request->getInternallyForwarded()) {
+                    $request->setInternallyForwarded();
                     if ($request->getParam('isIframe')) {
                         $request->setParam('forwarded', true)
                             ->setControllerName('index')
                             ->setActionName('deniedIframe')
                             ->setDispatched(false);
-                    } elseif($request->getParam('isAjax')) {
+                    } elseif ($request->getParam('isAjax')) {
                         $request->setParam('forwarded', true)
                             ->setControllerName('index')
                             ->setActionName('deniedJson')
diff --git app/code/core/Mage/Core/Controller/Request/Http.php app/code/core/Mage/Core/Controller/Request/Http.php
index 368f392..123e89e 100644
--- app/code/core/Mage/Core/Controller/Request/Http.php
+++ app/code/core/Mage/Core/Controller/Request/Http.php
@@ -76,6 +76,13 @@ class Mage_Core_Controller_Request_Http extends Zend_Controller_Request_Http
     protected $_beforeForwardInfo = array();
 
     /**
+     * Flag for recognizing if request internally forwarded
+     *
+     * @var bool
+     */
+    protected $_internallyForwarded = false;
+
+    /**
      * Returns ORIGINAL_PATH_INFO.
      * This value is calculated instead of reading PATH_INFO
      * directly from $_SERVER due to cross-platform differences.
@@ -530,4 +537,27 @@ class Mage_Core_Controller_Request_Http extends Zend_Controller_Request_Http
         }
         return false;
     }
+
+    /**
+     * Define that request was forwarded internally
+     *
+     * @param boolean $flag
+     * @return Mage_Core_Controller_Request_Http
+     */
+    public function setInternallyForwarded($flag = true)
+    {
+        $this->_internallyForwarded = (bool)$flag;
+        return $this;
+    }
+
+    /**
+     * Checks if request was forwarded internally
+     *
+     * @return bool
+     */
+    public function getInternallyForwarded()
+    {
+        return $this->_internallyForwarded;
+    }
+
 }
diff --git lib/Varien/Db/Adapter/Pdo/Mysql.php lib/Varien/Db/Adapter/Pdo/Mysql.php
index 7b903df..a688695 100644
--- lib/Varien/Db/Adapter/Pdo/Mysql.php
+++ lib/Varien/Db/Adapter/Pdo/Mysql.php
@@ -2651,10 +2651,6 @@ class Varien_Db_Adapter_Pdo_Mysql extends Zend_Db_Adapter_Pdo_Mysql implements V
 
         $query = '';
         if (is_array($condition)) {
-            if (isset($condition['field_expr'])) {
-                $fieldName = str_replace('#?', $this->quoteIdentifier($fieldName), $condition['field_expr']);
-                unset($condition['field_expr']);
-            }
             $key = key(array_intersect_key($condition, $conditionKeyMap));
 
             if (isset($condition['from']) || isset($condition['to'])) {

Please patch!



Hi! My name is Mattias Geniar. I'm a Support Manager at Nucleus Hosting in Belgium, a general web geek & public speaker. Currently working on DNS Spy & Oh Dear!. Follow me on Twitter as @mattiasgeniar.

Share this post

Did you like this post? Will you help me share it on social media? Thanks!

Comments

Willem Tuesday, April 21, 2015 at 15:51 - Reply

It is indeed mind-boggling. I asked Magento and they said:

Due to our lack of automated tests, we have no possibility for a quick release cycles.

Btw, you can test whether you have applied the patch correctly here: https://shoplift.byte.nl


    Mattias Geniar Tuesday, April 21, 2015 at 16:04 - Reply

    Amazing response …

    A “quick release” apparently means the same as “2 months of the patch being out, we haven’t actually bothered to update our zip files on the site“.

    I’m hoping this blogpost gets enough attention to trigger a response from Magento and a change of course for their Open Source community edition.


      Melvyn Sopacua Tuesday, April 21, 2015 at 20:43 - Reply

      This must not have been someone that has ever been on a release engineering team. Automated tests are not part of the equation. If you can release the patch you can run your variant of newvers.sh and make the new version available. If not then you better call it a preliminary patch.

      Also not someone trained in communication. Apparently we are provided with a patch that hasn’t been tested properly.


Pieter Tuesday, April 21, 2015 at 17:40 - Reply

Stop using magento? Seems a good enough reason for me. There’s plenty of other, much better designed frameworks out there.


      Xavier Wednesday, April 22, 2015 at 16:51 - Reply

      PrestaShop

      Full disclosure: I work at PrestaShop and I don’t mean to advertise our product with this reply. Just hoping for some feedback, whether your reaction is “yes, love it”, “meh” or “absolutely not”.

      :)


        Toni Wednesday, April 22, 2015 at 20:19 - Reply

        I’m sorry to trash your employer, but I’ve seen my 2-year old organize things better than your codebase.
        It’s a horrible pain in the butt to work with.

        Also, I don’t trust anyone using die() in their production code. Ever.

        Or anyone not having a clear structure in the framework for separating rendering / outputting to the client and the logic itself.
        This just means hacks all around.

        Ajax being a good example of this:
        controllers/front/OrderController.php:332

        if ($this->errors)
        {
        if (Tools::getValue('ajax'))
        $this->ajaxDie('{"hasError" : true, "errors" : ["'.implode('\',\'', $this->errors).'"]}');
        $this->step = 1;
        }
        if ($this->ajax)
        $this->ajaxDie(true);

        I mean what the…

        And who in their right mind would say this is an acceptable way of doing things:

        Tools::redirect('index.php?controller=order&step=1');

        You don’t have to be a node.js or Ruby programmer to realise that’s just moronic in so many levels.

        While Magento is also a horrible mess of a behemoth, it does have at least x2 the features than any other, including PrestaShop.


    Mike Tuesday, April 21, 2015 at 21:47 - Reply

    “There’s plenty of other, much better designed frameworks out there.”

    Such as?


Tom Harrison Wednesday, April 22, 2015 at 05:53 - Reply

In 2010, I worked at a company that had unwittingly chosen Magento. It quickly became evident to me that this “Open Source” software was nothing more than a come-on for their consulting service. A working version had been installed a year before, and the hook was set. Its excellent UI and slick marketing belied its seamy underbelly. I read the code, as I try to do with all libraries providing critical services, and began to realize that there was not simply bad programming, but something more nefarious: what I saw as a clear attempt to hook users into something free, then quickly create a morass which even skilled developers could not escape. We did, at great expense and time just by switching to another CMS product, but most companies who choose broad solutions like Magento are not that lucky. Perhaps they have changed in the last years. But this sounds exactly like the issues we had to wrench ourselves out of… This time with immediate and widespread security implications.


Joe Wednesday, April 22, 2015 at 22:15 - Reply

I’ve been forced to used magento ce a few times over the years by clients thinking it is the “latest and greatest” but have never had more then headaches with it. Several years ago I finally made the switch over to the SaaS platform 3dcart and have not looked back since. Check them out http://www.3dcart.com

For a very low cost they handle all of this for me and I can focus working ON my business and not FOR my business.


Leave a Reply

Your email address will not be published. Required fields are marked *

Inbound links