Note: This was responsibly disclosed to SilverStripe immediately when it was discovered. It has now beenpublicly 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 workedfor the last 7 years, we have primarily built sites in PHP, specificallySilverStripe. 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 aDataObject class that basically everything that writes to the database uses. That is from yourHomePage class to yourImageSlide class to yourMember class.

A typicalDataObject might look something like this (namespacing removed for brevity):

classMyCustomObjectextendsDataObject{private$db=['MyProperty'=>'Varchar'];private$has_many=['Slides'=>ImageSlide::class];}

When you have an instance of one of these objects, you can read/writeMyProperty 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 calledSwipeStripe however it wasn't a bug with the module, just in this case initially exposed through the module.

There is anOrderForm class with a specificupdate method which is where the problems start:

publicfunctionupdate(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. Theupdate 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 likeSomeProperty=1 and more likeSomeProperty[MySneakySQLStatement]=1, things start going weird, skipping how the prepared statements work and ending up in the raw query.

This issue presents itself here becausepostVars returnsSomeProperty[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 theupdate function I mentioned earlier isn't the problem, it is the code that it depended on which backs basically all writing to the database.

InSilverStripe'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 theupdate 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 seehere.

It really amount to double checking that field assignments are not arrays.

For example, in part of theDataObject class:

// Make sure none of our field assignment are arraysforeach($manipulationas$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 releaseshere or view the security release processeshere.