From ad1b00ec7dc1589a05bfc7f5f8207489797ef714 Mon Sep 17 00:00:00 2001 From: Serge Latyntcev Date: Mon, 3 Feb 2020 15:56:10 +1300 Subject: [PATCH] [CVE-2019-19325] XSS through non-scalar FormField attributes Silverstripe Forms allow malicious HTML or JavaScript to be inserted through non-scalar FormField attributes, which allows performing XSS (Cross-Site Scripting) on some forms built with user input (Request data). This can lead to phishing attempts to obtain a user's credentials or other sensitive user input. There is no known attack vector for extracting user-session information or credentials automatically, it required a user to fall for the phishing attempt. XSS can also be used to modify the presentation of content in malicious ways. --- .../09_Security/04_Secure_Coding.md | 13 +++++ docs/en/04_Changelogs/4.4.5.md | 18 +++++++ docs/en/04_Changelogs/4.5.1.md | 18 +++++++ src/Core/Convert.php | 17 +++++++ src/Forms/Form.php | 6 ++- src/Forms/FormField.php | 24 +++++---- tests/php/Forms/FormFieldTest.php | 49 +++++++++++++++++++ tests/php/Forms/FormTest.php | 2 + 8 files changed, 136 insertions(+), 11 deletions(-) create mode 100644 docs/en/04_Changelogs/4.4.5.md create mode 100644 docs/en/04_Changelogs/4.5.1.md diff --git a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md b/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md index a3393a72a..8f53b6ef5 100644 --- a/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md +++ b/docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md @@ -347,6 +347,19 @@ template, you'll need to take care of casting and escaping yourself in PHP. The [Convert](api:SilverStripe\Core\Convert) class has utilities for this, mainly *Convert::raw2xml()* and *Convert::raw2att()* (which is also used by *XML* and *ATT* in template code). +
+Most of the `Convert::raw2` methods accept arrays and do not affect array keys. +If you serialize your data, make sure to do that before you pass it to `Convert::raw2` methods. + +E.g.: + +```php +json_encode(Convert::raw2sql($request->getVar('multiselect'))); // WRONG! + +Convert::raw2sql(json_encode($request->getVar('multiselect'))); // Correct! +``` +
+ PHP: ```php diff --git a/docs/en/04_Changelogs/4.4.5.md b/docs/en/04_Changelogs/4.4.5.md new file mode 100644 index 000000000..ac581ad8e --- /dev/null +++ b/docs/en/04_Changelogs/4.4.5.md @@ -0,0 +1,18 @@ +# 4.4.5 + +## Security patches + +This release contains security patches + +### CVE-2019-1935 (CVSS 7.5) + +Silverstripe Forms allow malicious HTML or JavaScript to be inserted through non-scalar FormField attributes, which allows performing XSS (Cross-Site Scripting) on some forms built with user input (Request data). This can lead to phishing attempts to obtain a user's credentials or other sensitive user input. There is no known attack vector for extracting user-session information or credentials automatically, it required a user to fall for the phishing attempt. XSS can also be used to modify the presentation of content in malicious ways. + +The vulnerability is known to apply in at least the following cases: + +The login form provided by Silverstripe. When the login form is used with Multi Factor Authentication (MFA), the attack complexity for phishing increases, and is mitigated by using security keys such as Yubikey as an unphishable token. +Forms which are configured to populate field values based on request parameters. This usually happens via setting the $value on a FormField instance during construction of the form, or by loading request data via Form->loadDataFrom($myRequest->getVars()). +Forms which have form validation applied through RequiredFields, and opt-out of using CSRF tokens via disableSecurityToken(). In this case, the vulnerability is more impactful if the form is also configured to accept GET submissions, rather than the default of POST submissions. +The vulnerability has not identified on forms created through the silverstripe/userforms module. + + diff --git a/docs/en/04_Changelogs/4.5.1.md b/docs/en/04_Changelogs/4.5.1.md new file mode 100644 index 000000000..ac581ad8e --- /dev/null +++ b/docs/en/04_Changelogs/4.5.1.md @@ -0,0 +1,18 @@ +# 4.4.5 + +## Security patches + +This release contains security patches + +### CVE-2019-1935 (CVSS 7.5) + +Silverstripe Forms allow malicious HTML or JavaScript to be inserted through non-scalar FormField attributes, which allows performing XSS (Cross-Site Scripting) on some forms built with user input (Request data). This can lead to phishing attempts to obtain a user's credentials or other sensitive user input. There is no known attack vector for extracting user-session information or credentials automatically, it required a user to fall for the phishing attempt. XSS can also be used to modify the presentation of content in malicious ways. + +The vulnerability is known to apply in at least the following cases: + +The login form provided by Silverstripe. When the login form is used with Multi Factor Authentication (MFA), the attack complexity for phishing increases, and is mitigated by using security keys such as Yubikey as an unphishable token. +Forms which are configured to populate field values based on request parameters. This usually happens via setting the $value on a FormField instance during construction of the form, or by loading request data via Form->loadDataFrom($myRequest->getVars()). +Forms which have form validation applied through RequiredFields, and opt-out of using CSRF tokens via disableSecurityToken(). In this case, the vulnerability is more impactful if the form is also configured to accept GET submissions, rather than the default of POST submissions. +The vulnerability has not identified on forms created through the silverstripe/userforms module. + + diff --git a/src/Core/Convert.php b/src/Core/Convert.php index 1eefd057f..a3b5849b6 100644 --- a/src/Core/Convert.php +++ b/src/Core/Convert.php @@ -33,6 +33,8 @@ class Convert /** * Convert a value to be suitable for an XML attribute. * + * Warning: Does not escape array keys + * * @param array|string $val String to escape, or array of strings * @return array|string */ @@ -44,6 +46,8 @@ class Convert /** * Convert a value to be suitable for an HTML attribute. * + * Warning: Does not escape array keys + * * @param string|array $val String to escape, or array of strings * @return array|string */ @@ -56,6 +60,8 @@ class Convert * Convert a value to be suitable for an HTML ID attribute. Replaces non * supported characters with a space. * + * Warning: Does not escape array keys + * * @see http://www.w3.org/TR/REC-html40/types.html#type-cdata * * @param array|string $val String to escape, or array of strings @@ -79,6 +85,8 @@ class Convert * Convert a value to be suitable for an HTML ID attribute. Replaces non * supported characters with an underscore. * + * Warning: Does not escape array keys + * * @see http://www.w3.org/TR/REC-html40/types.html#type-cdata * * @param array|string $val String to escape, or array of strings @@ -108,6 +116,8 @@ class Convert /** * Ensure that text is properly escaped for XML. * + * Warning: Does not escape array keys + * * @see http://www.w3.org/TR/REC-xml/#dt-escape * @param array|string $val String to escape, or array of strings * @return array|string @@ -127,6 +137,8 @@ class Convert /** * Ensure that text is properly escaped for Javascript. * + * Warning: Does not escape array keys + * * @param array|string $val String to escape, or array of strings * @return array|string */ @@ -182,6 +194,8 @@ class Convert * Safely encodes a value (or list of values) using the current database's * safe string encoding method * + * Warning: Does not encode array keys + * * @param mixed|array $val Input value, or list of values as an array * @param boolean $quoted Flag indicating whether the value should be safely * quoted, instead of only being escaped. By default this function will @@ -221,6 +235,9 @@ class Convert /** * Convert XML to raw text. + * + * Warning: Does not decode array keys + * * @uses html2raw() * @todo Currently &#xxx; entries are stripped; they should be converted * @param mixed $val diff --git a/src/Forms/Form.php b/src/Forms/Form.php index ca8b3a828..fdd54998f 100644 --- a/src/Forms/Form.php +++ b/src/Forms/Form.php @@ -897,7 +897,11 @@ class Form extends ViewableData implements HasRequestHandler // Create markup $parts = array(); foreach ($attrs as $name => $value) { - $parts[] = ($value === true) ? "{$name}=\"{$name}\"" : "{$name}=\"" . Convert::raw2att($value) . "\""; + if ($value === true) { + $value = $name; + } + + $parts[] = sprintf('%s="%s"', Convert::raw2att($name), Convert::raw2att($value)); } return implode(' ', $parts); diff --git a/src/Forms/FormField.php b/src/Forms/FormField.php index 94721475e..b269066ab 100644 --- a/src/Forms/FormField.php +++ b/src/Forms/FormField.php @@ -12,6 +12,7 @@ use SilverStripe\ORM\DataObject; use SilverStripe\ORM\DataObjectInterface; use SilverStripe\ORM\FieldType\DBField; use SilverStripe\ORM\FieldType\DBHTMLText; +use SilverStripe\ORM\ValidationResult; use SilverStripe\View\SSViewer; /** @@ -740,14 +741,16 @@ class FormField extends RequestHandler foreach ($attributes as $name => $value) { if ($value === true) { - $parts[] = sprintf('%s="%s"', $name, $name); + $value = $name; } else { - $strValue = Convert::raw2att($value); - if (!is_string($strValue)) { - $strValue = json_encode($strValue); + if (is_scalar($value)) { + $value = (string) $value; + } else { + $value = json_encode($value); } - $parts[] = sprintf('%s="%s"', $name, $strValue); } + + $parts[] = sprintf('%s="%s"', Convert::raw2att($name), Convert::raw2att($value)); } return implode(' ', $parts); @@ -1343,13 +1346,14 @@ class FormField extends RequestHandler public function debug() { $strValue = is_string($this->value) ? $this->value : print_r($this->value, true); + return sprintf( '%s (%s: %s : %s) = %s', - static::class, - $this->name, - $this->title, - $this->message, - $strValue + Convert::raw2att(static::class), + Convert::raw2att($this->name), + Convert::raw2att($this->title), + $this->getMessageCast() == ValidationResult::CAST_HTML ? Convert::raw2xml($this->message) : $this->message, + Convert::raw2att($strValue) ); } diff --git a/tests/php/Forms/FormFieldTest.php b/tests/php/Forms/FormFieldTest.php index 208b06e3f..e6333e43f 100644 --- a/tests/php/Forms/FormFieldTest.php +++ b/tests/php/Forms/FormFieldTest.php @@ -5,6 +5,7 @@ namespace SilverStripe\Forms\Tests; use ReflectionClass; use SilverStripe\Core\ClassInfo; use SilverStripe\Core\Config\Config; +use SilverStripe\Core\Convert; use SilverStripe\Dev\SapphireTest; use SilverStripe\Forms\CompositeField; use SilverStripe\Forms\FieldList; @@ -14,6 +15,7 @@ use SilverStripe\Forms\NullableField; use SilverStripe\Forms\RequiredFields; use SilverStripe\Forms\Tests\FormFieldTest\TestExtension; use SilverStripe\Forms\TextField; +use SilverStripe\ORM\ValidationResult; class FormFieldTest extends SapphireTest { @@ -187,6 +189,53 @@ class FormFieldTest extends SapphireTest $this->assertContains('three="3"', $field->getAttributesHTML('one', 'two')); } + /** + * Covering all potential inputs for Convert::raw2xml + */ + public function escapeHtmlDataProvider() + { + return [ + [''], + [['']], + [['' => '']] + ]; + } + + /** + * @dataProvider escapeHtmlDataProvider + **/ + public function testGetAttributesEscapeHtml($value) + { + $key = bin2hex(random_bytes(4)); + + if (is_scalar($value)) { + $field = new FormField('', '', ''); + $field->setAttribute($value, $key); + $html = $field->getAttributesHTML(); + $this->assertFalse(strpos($html, '')); + } + + $field = new FormField('', '', ''); + $field->setAttribute($key, $value); + $html = $field->getAttributesHTML(); + + $this->assertFalse(strpos($html, '')); + } + + /** + * @dataProvider escapeHtmlDataProvider + */ + public function testDebugEscapeHtml($value) + { + $field = new FormField('', '', ''); + $field->setAttribute('', $value); + $field->setMessage('', null, ValidationResult::CAST_HTML); + + $html = $field->debug(); + + $this->assertFalse(strpos($html, '')); + } + public function testReadonly() { $field = new FormField('MyField'); diff --git a/tests/php/Forms/FormTest.php b/tests/php/Forms/FormTest.php index 87336ddc8..7f4b83dd6 100644 --- a/tests/php/Forms/FormTest.php +++ b/tests/php/Forms/FormTest.php @@ -904,9 +904,11 @@ class FormTest extends FunctionalTest $form->setAttribute('one', 1); $form->setAttribute('two', 2); $form->setAttribute('three', 3); + $form->setAttribute('', ''); $this->assertNotContains('one="1"', $form->getAttributesHTML('one', 'two')); $this->assertNotContains('two="2"', $form->getAttributesHTML('one', 'two')); $this->assertContains('three="3"', $form->getAttributesHTML('one', 'two')); + $this->assertNotContains('', $form->getAttributesHTML()); } function testMessageEscapeHtml()