Merge pull request #94 from silverstripe-security/fix/cve-2019-19325

CVE-2019-1935

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).
 
+<div class="warning" markdown='1'>
+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!
+```
+</div>
+
 PHP:
 
 ```php

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.
+
+

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.
+
+

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

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);

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_scalar($value)) {
-                if (!is_string($strValue)) {
+                    $value = (string) $value;
-                    $strValue = json_encode($strValue);
+                } 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 : <span style="color:red;">%s</span>) = %s',
-            static::class,
+            Convert::raw2att(static::class),
-            $this->name,
+            Convert::raw2att($this->name),
-            $this->title,
+            Convert::raw2att($this->title),
-            $this->message,
+            $this->getMessageCast() == ValidationResult::CAST_HTML ? Convert::raw2xml($this->message) : $this->message,
-            $strValue
+            Convert::raw2att($strValue)
         );
     }
 

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 [
+            ['<html>'],
+            [['<html>']],
+            [['<html>' => '<html>']]
+        ];
+    }
+
+    /**
+     * @dataProvider escapeHtmlDataProvider
+     **/
+    public function testGetAttributesEscapeHtml($value)
+    {
+        $key = bin2hex(random_bytes(4));
+
+        if (is_scalar($value)) {
+            $field = new FormField('<html>', '<html>', '<html>');
+            $field->setAttribute($value, $key);
+            $html = $field->getAttributesHTML();
+            $this->assertFalse(strpos($html, '<html>'));
+        }
+
+        $field = new FormField('<html>', '<html>', '<html>');
+        $field->setAttribute($key, $value);
+        $html = $field->getAttributesHTML();
+
+        $this->assertFalse(strpos($html, '<html>'));
+    }
+
+    /**
+     * @dataProvider escapeHtmlDataProvider
+     */
+    public function testDebugEscapeHtml($value)
+    {
+        $field = new FormField('<html>', '<html>', '<html>');
+        $field->setAttribute('<html>', $value);
+        $field->setMessage('<html>', null, ValidationResult::CAST_HTML);
+
+        $html = $field->debug();
+
+        $this->assertFalse(strpos($html, '<html>'));
+    }
+
     public function testReadonly()
     {
         $field = new FormField('MyField');

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('<html>', '<html>');
         $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('<html>', $form->getAttributesHTML());
     }
 
     function testMessageEscapeHtml()