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 @@ public static function raw2att($val)
     /**
      * 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 @@ public static function raw2htmlatt($val)
      * 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 @@ public static function raw2htmlname($val)
      * 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 @@ public static function raw2htmlid($val)
     /**
      * 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 @@ public static function raw2xml($val)
     /**
      * 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 @@ public static function array2json($val, $options = 0)
      * 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 @@ public static function symbol2sql($identifier, $separator = '.')
 
     /**
      * 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 @@ public function getAttributesHTML($attrs = null)
         // 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\DataObjectInterface;
 use SilverStripe\ORM\FieldType\DBField;
 use SilverStripe\ORM\FieldType\DBHTMLText;
+use SilverStripe\ORM\ValidationResult;
 use SilverStripe\View\SSViewer;
 
 /**
@@ -740,14 +741,16 @@ public function getAttributesHTML($attributes = null)
 
         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 @@ public function getDescription()
     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,
-            $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)
         );
     }
 

tests/php/Forms/FormFieldTest.php

@@ -5,6 +5,7 @@
 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\RequiredFields;
 use SilverStripe\Forms\Tests\FormFieldTest\TestExtension;
 use SilverStripe\Forms\TextField;
+use SilverStripe\ORM\ValidationResult;
 
 class FormFieldTest extends SapphireTest
 {
@@ -187,6 +189,53 @@ public function testAttributesHTML()
         $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 @@ public function testAttributesHTML()
         $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()