The anatomy of a critical vulnerability
Tags: security, php, silverstripe
Published: 2019-03-06 11:12:12

---
title: The anatomy of a critical vulnerability
published: true
description: From discovery to disclosure
tags: security, php, silverstripe
canonical_url: https://turnerj.com/blog/the-anatomy-of-a-critical-vulnerability
---

**Note: This was responsibly disclosed to SilverStripe immediately when it was discovered. It has now been [publicly disclosed by SilverStripe](https://www.silverstripe.org/download/security-releases/ss-2018-021) (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](https://dev.to/turnerj/i-left-my-job-today-f1a), we have primarily built sites in PHP, specifically [SilverStripe](https://www.silverstripe.org/). 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):

```php
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:

```php
$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](https://github.com/swipestripe/silverstripe-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:

```php
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:

```json
{
    "SomeProperty": {
        "MySneakySQLStatement": 1
    }
}
```

If internally it looked more like the following, it likely would have not been an issue:

```json
{
    "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](https://www.silverstripe.org/download/security-releases/ss-2018-021) 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](https://github.com/silverstripe/silverstripe-framework/commit/6bf9542d664ac7935691c8055505b7ad8ea26e9a).

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

For example, in part of the `DataObject` class:

```php
// 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](https://www.silverstripe.org/download/security-releases/) or view the security release processes [here](http://docs.silverstripe.org/en/contributing/release_process/).