Note: This was responsibly disclosed to SilverStripe immediately when it was discovered. It has now been publicly disclosed by SilverStripe (CVE-2019-5715 / SS-2018-021) as of the 18th of February with patches for all supported versions released.
Where I have worked for the last 7 years, we have primarily built sites in PHP, specifically SilverStripe. As a final little surprise for my colleagues before I left, I discovered an SQL injection vulnerability.
Before going into detail of what the security vulnerability is, I need to explain a little about SilverStripe.
SilverStripe
SilverStripe is an open source PHP content management system. At its core, you have a DataObject class that basically everything that writes to the database uses. That is from your HomePage class to your ImageSlide class to your Member class.
A typical DataObject might look something like this (namespacing removed for brevity):
class MyCustomObject extends DataObject
{
private $db = [
'MyProperty' => 'Varchar'
];
private $has_many = [
'Slides' => ImageSlide::class
];
}
When you have an instance of one of these objects, you can read/write MyProperty through magic methods:
$myPropertyVal = $myCustomObject->MyProperty;
$slides = $myCustomObject->Slides();
$myCustomObject->MyProperty = 'Hello world!';
$myCustomObject->write();
This is the main way you will deal with getting and setting values. There are other helper methods which can bulk set these values from arrays etc.
That's all you need to know to get the gist of the issue that is to come.
Discovery
While I did discover the issue, there are two important things to mention in the discovery process:
- A site of ours was having a security audit through one of those automatic vulnerability testing systems. An error from a specific attempt pointed me in the right direction.
- When I disclosed the issue to SilverStripe, they discovered further issues which expanded on this vulnerability.
The error triggered by the automated security audit was with a SilverStripe module called SwipeStripe however it wasn't a bug with the module, just in this case initially exposed through the module.
There is an OrderForm class with a specific update method which is where the problems start:
public function update(SS_HTTPRequest $request) {
if ($request->isPOST()) {
$member = Customer::currentUser() ? Customer::currentUser() : singleton('Customer');
$order = Cart::get_current_order();
//Update the Order
$order->update($request->postVars());
$order->updateModifications($request->postVars())
->write();
$form = OrderForm::create(
$this->controller,
'OrderForm'
)->disableSecurityToken();
// $form->validate();
return $form->renderWith('OrderFormCart');
}
}
It is $order->update($request->postVars()); that is a problem. The update function call on the $order object is a built-in SilverStripe function where you pass an array of fields and it will set them for you. The passing of POST variables straight into it is a Very Bad Idea™ (you should be checking your fields properly) but I digress.
SilverStripe would normally safely prepare queries so SQL injection can't happen but there was an edge case where it didn't work.
It turns out that if your POST request looks less like SomeProperty=1 and more like SomeProperty[MySneakySQLStatement]=1, things start going weird, skipping how the prepared statements work and ending up in the raw query.
This issue presents itself here because postVars returns SomeProperty[MySneakySQLStatement]=1 to something a little like this:
{
"SomeProperty": {
"MySneakySQLStatement": 1
}
}
If internally it looked more like the following, it likely would have not been an issue:
{
"SomeProperty[MySneakySQLStatement]": 1
}
Now to actually have a working exploit, it is a little more complex than just sticking an SQL query in a field name but you get the idea.
I gave everything I knew to SilverStripe and they took it from there, finding that it is a bit bigger of an issue than it seemed.
The actual issue
In terms of a method to exploit this issue, I wasn't wrong but it wasn't the only method. It turns out that the update function I mentioned earlier isn't the problem, it is the code that it depended on which backs basically all writing to the database.
In SilverStripe's blog post about the issue, they state:
Both direct assignment on DataObject (update(), setters via method calls, setters via magic methods) and indirect assignment (e.g. Form->saveInto()) are affected.
So not just the update method that I originally noticed but basically every method of writing was affected! This bug just turned from a "bad but unlikely to affect many" to a "oh this might affect everyone".
Their blog post goes onto say:
The vulnerability is related to the DBField classes underpinning the DataObject logic. Most DBField types in SilverStripe 3 are affected. Only the DBYear field type is confirmed to be affected in SilverStripe 4, limiting the impact in our current release line to a relatively uncommon use case.
While still a critical vulnerability, it is great that through other fixes and improvements that the latest major version of SilverStripe (Version 4) is less impacted.
The fix
The fix is actually fairly small, split across 5 files (for SilverStripe 4) which you can see here.
It really amount to double checking that field assignments are not arrays.
For example, in part of the DataObject class:
// Make sure none of our field assignment are arrays
foreach ($manipulation as $tableManipulation) {
if (!isset($tableManipulation['fields'])) {
continue;
}
foreach ($tableManipulation['fields'] as $fieldValue) {
if (is_array($fieldValue)) {
user_error(
'DataObject::writeManipulation: parameterised field assignments are disallowed',
E_USER_ERROR
);
}
}
}
Conclusion
Always, always, ALWAYS responsibly disclose security issues. Thanks to Matt, Maxime, Ingo and everyone else at SilverStripe who worked on testing and resolving this vulnerability! 🙂
For SilverStripe users, you can view all security releases here or view the security release processes here.
Top comments (0)