tonyair
/
silverstripe-framework
mirror of https://github.com/silverstripe/silverstripe-framework
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

Merge branch '4.10' into 4.11

This commit is contained in:
Steve Boyd 2022-06-28 17:41:49 +12:00
parent dec85819bd 794640247b
commit 98b985fb91
6 changed files with 150 additions and 107 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

59
docs/en/02_Developer_Guides/09_Security/04_Secure_Coding.md
View File

@ -214,39 +214,34 @@ or [sanitize](http://htmlpurifier.org/) it correctly.
See [http://shiflett.org/articles/foiling-cross-site-attacks](http://shiflett.org/articles/foiling-cross-site-attacks)
for in-depth information about "Cross-Site-Scripting".
### What if I can't trust my editors?
The default configuration of Silverstripe CMS assumes some level of trust is given to your editors who have access
to the CMS. Though the HTML WYSIWYG editor is configured to provide some control over the HTML an editor provides,
this is not enforced server side, and so can be bypassed by a malicious editor. A editor that does so can use an
XSS attack against an admin to perform any administrative action.
If you can't trust your editors, Silverstripe CMS must be configured to filter the content so that any javascript is
stripped out
To enable filtering, set the HtmlEditorField::$sanitise_server_side [configuration](/developer_guides/configuration/configuration) property to
true, e.g.
```
HtmlEditorField::config()->sanitise_server_side = true
```
The built in sanitiser enforces the TinyMCE whitelist rules on the server side, and is sufficient to eliminate the
most common XSS vectors.
However some subtle XSS attacks that exploit HTML parsing bugs need heavier filtering. For greater protection
you can install the [htmlpurifier](https://github.com/silverstripe-labs/silverstripe-htmlpurifier) module which
will replace the built in sanitiser with one that uses the [HTML Purifier](http://htmlpurifier.org/) library.
In both cases, you must ensure that you have not configured TinyMCE to explicitly allow script elements or other
javascript-specific attributes.
### Additional options
For `HTMLText` database fields which aren't edited through `HtmlEditorField`, you also
have the option to explicitly whitelist allowed tags in the field definition, e.g. `"MyField" => "HTMLText('meta','link')"`.
The `SiteTree.ExtraMeta` property uses this to limit allowed input.
##### But I also need my editors to provide javascript
### What if I need to allow script or style tags?
It is not currently possible to allow editors to provide javascript content and yet still protect other users
The default configuration of Silverstripe CMS uses a santiser to enforce TinyMCE whitelist rules on the server side,
and is sufficient to eliminate the most common XSS vectors. Notably, this will remove script and style tags.
If your site requires script or style tags to be added via TinyMCE, Silverstripe CMS can be configured to disable the
server side santisation. You will also need to update the TinyMCE whitelist [settings](/developer_guides/forms/field_types/htmleditorfield/#setting-options) to remove the frontend sanitisation.
However, it's strongly discouraged as it opens up the possibility of malicious code being added to your site through the CMS.
To disable filtering, set the `HtmlEditorField::$sanitise_server_side` [configuration](/developer_guides/configuration/configuration) property to `false`, i.e.
```yml
---
Name: project-htmleditor
After: htmleditor
---
SilverStripe\Forms\HTMLEditor\HTMLEditorField:
sanitise_server_side: false
```
Note it is not currently possible to allow editors to provide javascript content and yet still protect other users
from any malicious code within that javascript.
We recommend configuring [shortcodes](/developer_guides/extending/shortcodes) that can be used by editors in place of using javascript directly.
@ -435,16 +430,6 @@ Some rules of thumb:
* Don't concatenate URLs in a template. It only works in extremely simple cases that usually contain bugs.
* Use *Controller::join_links()* to concatenate URLs. It deals with query strings and other such edge cases.
### Filtering incoming HTML from TinyMCE
In some cases you may be particularly concerned about which HTML elements are addable to Content via the CMS.
By default, although TinyMCE is configured to restrict some dangerous tags (such as `script` tags), this restriction
is not enforced server-side. A malicious user with write access to the CMS might create a specific request to avoid
these restrictions.
To enable server side filtering using the same whitelisting controls as TinyMCE, set the
HtmlEditorField::$sanitise_server_side config property to true.
## Cross-Site Request Forgery (CSRF)
Silverstripe CMS has built-in countermeasures against [CSRF](http://shiflett.org/articles/cross-site-request-forgeries) identity theft for all form submissions. A form object

46
src/Core/Convert.php
View File

@ -2,12 +2,11 @@
namespace SilverStripe\Core;
use InvalidArgumentException;
use SimpleXMLElement;
use SilverStripe\Dev\Deprecation;
use SilverStripe\ORM\DB;
use SilverStripe\View\Parsers\URLSegmentFilter;
use InvalidArgumentException;
use SimpleXMLElement;
use Exception;
/**
* Library of conversion functions, implemented as static methods.
@ -29,7 +28,6 @@ use Exception;
*/
class Convert
{
/**
* Convert a value to be suitable for an XML attribute.
*
@ -296,40 +294,34 @@ class Convert
* @param string $val
* @param boolean $disableDoctypes Disables the use of DOCTYPE, and will trigger an error if encountered.
* false by default.
* @param boolean $disableExternals Disables the loading of external entities. false by default. No-op in PHP 8.
* @param boolean $disableExternals Does nothing because xml entities are removed
* @deprecated 4.11.0:5.0.0
* @return array
* @throws Exception
*/
public static function xml2array($val, $disableDoctypes = false, $disableExternals = false)
{
// PHP 8 deprecates libxml_disable_entity_loader() as it is no longer needed
if (\PHP_VERSION_ID >= 80000) {
$disableExternals = false;
}
Deprecation::notice('4.10', 'Use a dedicated XML library instead');
// Check doctype
if ($disableDoctypes && preg_match('/\<\!DOCTYPE.+]\>/', $val ?? '')) {
if ($disableDoctypes && strpos($val ?? '', '<!DOCTYPE') !== false) {
throw new InvalidArgumentException('XML Doctype parsing disabled');
}
// Disable external entity loading
$oldVal = null;
if ($disableExternals) {
$oldVal = libxml_disable_entity_loader($disableExternals ?? false);
// CVE-2021-41559 Ensure entities are removed due to their inherent security risk via
// XXE attacks and quadratic blowup attacks, and also lack of consistent support
$val = preg_replace('/(?s)<!ENTITY.*?>/', '', $val ?? '');
// If there's still an <!ENTITY> present, then it would be the result of a maliciously
// crafted XML document e.g. <!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
if (strpos($val ?? '', '<!ENTITY') !== false) {
throw new InvalidArgumentException('Malicious XML entity detected');
}
try {
$xml = new SimpleXMLElement($val);
$result = self::recursiveXMLToArray($xml);
} catch (Exception $ex) {
if ($disableExternals) {
libxml_disable_entity_loader($oldVal);
}
throw $ex;
}
if ($disableExternals) {
libxml_disable_entity_loader($oldVal ?? false);
}
return $result;
// This will throw an exception if the XML contains references to any internal entities
// that were defined in an <!ENTITY /> before it was removed
$xml = new SimpleXMLElement($val ?? '');
return self::recursiveXMLToArray($xml);
}
/**

2
src/Forms/HTMLEditor/HTMLEditorField.php
View File

@ -40,7 +40,7 @@ class HTMLEditorField extends TextareaField
* @config
* @var bool
*/
private static $sanitise_server_side = false;
private static $sanitise_server_side = true;
/**
* Number of rows

11
src/Forms/HTMLEditor/HTMLEditorSanitiser.php
View File

@ -345,6 +345,17 @@ class HTMLEditorSanitiser
foreach ($elementRule->attributesForced as $attr => $forced) {
$el->setAttribute($attr, $forced);
}
// Matches "javascript:" with any arbitrary linebreaks inbetween the characters.
$regex = '/^\s*' . implode('\v*', str_split('javascript:')) . '/';
// Strip out javascript execution in href or src attributes.
foreach (['src', 'href'] as $dangerAttribute) {
if ($el->hasAttribute($dangerAttribute)) {
if (preg_match($regex, $el->getAttribute($dangerAttribute))) {
$el->removeAttribute($dangerAttribute);
}
}
}
}
if ($el->tagName === 'a' && $linkRelValue !== null) {

115
tests/php/Core/ConvertTest.php
View File

@ -403,55 +403,86 @@ PHP
*/
public function testXML2Array()
{
// Ensure an XML file at risk of entity expansion can be avoided safely
$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING">]>
<!DOCTYPE results [
<!ENTITY long "SOME_SUPER_LONG_STRING">
]>
<results>
<result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
<result>&long;&long;&long;</result>
<result>My para</result>
<result>Ampersand &amp; is retained and not double encoded</result>
</results>
XML
;
try {
Convert::xml2array($inputXML, true);
} catch (Exception $ex) {
}
$this->assertTrue(
isset($ex)
&& $ex instanceof InvalidArgumentException
&& $ex->getMessage() === 'XML Doctype parsing disabled'
);
// Test without doctype validation
;
$expected = [
'result' => [
'Now include SOME_SUPER_LONG_STRING lots of times to expand the in-memory size of this XML structure',
[
'long' => [
[
'long' => 'SOME_SUPER_LONG_STRING'
],
[
'long' => 'SOME_SUPER_LONG_STRING'
],
[
'long' => 'SOME_SUPER_LONG_STRING'
]
]
]
]
'result' => [
'My para',
'Ampersand & is retained and not double encoded'
]
];
$result = Convert::xml2array($inputXML, false, true);
$this->assertEquals(
$expected,
$result
);
$result = Convert::xml2array($inputXML, false, false);
$this->assertEquals(
$expected,
$result
);
$actual = Convert::xml2array($inputXML, false);
$this->assertEquals($expected, $actual);
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('XML Doctype parsing disabled');
Convert::xml2array($inputXML, true);
}
/**
* Tests {@link Convert::xml2array()} if an exception the contains a reference to a removed <!ENTITY />
*/
public function testXML2ArrayEntityException()
{
$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [
<!ENTITY long "SOME_SUPER_LONG_STRING">
]>
<results>
<result>Now include &long; lots of times to expand the in-memory size of this XML structure</result>
<result>&long;&long;&long;</result>
</results>
XML;
$this->expectException(Exception::class);
$this->expectExceptionMessage('String could not be parsed as XML');
Convert::xml2array($inputXML);
}
/**
* Tests {@link Convert::xml2array()} if an exception the contains a reference to a multiple removed <!ENTITY />
*/
public function testXML2ArrayMultipleEntitiesException()
{
$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [<!ENTITY long "SOME_SUPER_LONG_STRING"><!ENTITY short "SHORT_STRING">]>
<results>
<result>Now include &long; and &short; lots of times</result>
<result>&long;&long;&long;&short;&short;&short;</result>
</results>
XML;
$this->expectException(Exception::class);
$this->expectExceptionMessage('String could not be parsed as XML');
Convert::xml2array($inputXML);
}
/**
* Tests {@link Convert::xml2array()} if there is a malicious <!ENTITY /> present
*/
public function testXML2ArrayMaliciousEntityException()
{
$inputXML = <<<XML
<?xml version="1.0"?>
<!DOCTYPE results [
<!ENTITY><!<!ENTITY>ENTITY ext SYSTEM "http://evil.com">
]>
<results>
<result>Evil document</result>
</results>
XML;
$this->expectException(InvalidArgumentException::class);
$this->expectExceptionMessage('Malicious XML entity detected');
Convert::xml2array($inputXML);
}
/**

24
tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php
View File

@ -74,6 +74,30 @@ class HTMLEditorSanitiserTest extends FunctionalTest
'<a href="/test" target="_blank">Test</a>',
'noopener rel attribute is unchanged when link_rel_value is null'
],
[
'a[href|target|rel]',
'<a href="javascript:alert(0);">Test</a>',
'<a>Test</a>',
'Javascript in the href attribute of a link is completely removed'
],
[
'a[href|target|rel]',
'<a href="' . implode("\n", str_split(' javascript:')) . '">Test</a>',
'<a>Test</a>',
'Javascript in the href attribute of a link is completely removed even for multiline markup'
],
[
'map[name],area[href|shape|coords]',
'<map name="test"><area shape="rect" coords="34,44,270,350" href="javascript:alert(0);"></map>',
'<map name="test"><area shape="rect" coords="34,44,270,350"></map>',
'Javascript in the href attribute of a map\'s clickable area is completely removed'
],
[
'iframe[src]',
'<iframe src="javascript:alert(0);"></iframe>',
'<iframe></iframe>',
'Javascript in the src attribute of an iframe is completely removed'
],
];
$config = HTMLEditorConfig::get('htmleditorsanitisertest');