From 2b5420ee7d6ea29c1096e8aad952dda83862ef08 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Tue, 23 Aug 2022 15:36:48 +1200 Subject: [PATCH 1/7] [CVE-2022-37430] Sanitise mixed case javascript --- src/Forms/HTMLEditor/HTMLEditorSanitiser.php | 2 +- tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php index 09ff3e8a4..b35fcf19a 100644 --- a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php +++ b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php @@ -347,7 +347,7 @@ class HTMLEditorSanitiser } // Matches "javascript:" with any arbitrary linebreaks inbetween the characters. - $regex = '/^\s*' . implode('\v*', str_split('javascript:')) . '/'; + $regex = '/^\s*' . implode('\v*', str_split('javascript:')) . '/i'; // Strip out javascript execution in href or src attributes. foreach (['src', 'href'] as $dangerAttribute) { if ($el->hasAttribute($dangerAttribute)) { diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php index 4b3695274..14f6771a5 100644 --- a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php +++ b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php @@ -98,6 +98,12 @@ class HTMLEditorSanitiserTest extends FunctionalTest '', 'Javascript in the src attribute of an iframe is completely removed' ], + [ + 'iframe[src]', + '', + '', + 'Mixed case javascript in the src attribute of an iframe is completely removed' + ], ]; $config = HTMLEditorConfig::get('htmleditorsanitisertest'); From d3c28579b797074f0ef99c0d042d77812b9d33a9 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 6 Sep 2022 15:06:35 +1200 Subject: [PATCH 2/7] [CVE-2022-38462] Don't allow CRLF in header values --- src/Control/HTTPResponse.php | 10 +++++++++- tests/php/Control/HTTPResponseTest.php | 20 ++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/Control/HTTPResponse.php b/src/Control/HTTPResponse.php index 15eb80520..5f7dc5b4b 100644 --- a/src/Control/HTTPResponse.php +++ b/src/Control/HTTPResponse.php @@ -267,7 +267,7 @@ class HTTPResponse public function addHeader($header, $value) { $header = strtolower($header ?? ''); - $this->headers[$header] = $value; + $this->headers[$header] = $this->sanitiseHeader($value); return $this; } @@ -310,6 +310,14 @@ class HTTPResponse return $this; } + /** + * Sanitise header values to avoid possible XSS vectors + */ + private function sanitiseHeader(string $value): string + { + return preg_replace('/\v/', '', $value); + } + /** * @param string $dest * @param int $code diff --git a/tests/php/Control/HTTPResponseTest.php b/tests/php/Control/HTTPResponseTest.php index 18469ec62..88c1aaeeb 100644 --- a/tests/php/Control/HTTPResponseTest.php +++ b/tests/php/Control/HTTPResponseTest.php @@ -45,6 +45,26 @@ class HTTPResponseTest extends SapphireTest $this->assertEmpty($response->getHeader('X-Animal')); } + public function providerSanitiseHeaders() + { + return [ + 'plain text is retained' => ['some arbitrary value1', 'some arbitrary value1'], + 'special chars are retained' => ['`~!@#$%^&*()_+-=,./<>?;\':"[]{}\\|', '`~!@#$%^&*()_+-=,./<>?;\':"[]{}\\|'], + 'line breaks are removed' => ['no line breaks', "n\ro line \nbreaks\r\n"], + ]; + } + + /** + * @dataProvider providerSanitiseHeaders + */ + public function testSanitiseHeaders(string $expected, string $value) + { + $response = new HTTPResponse(); + + $response->addHeader('X-Sanitised', $value); + $this->assertSame($expected, $response->getHeader('X-Sanitised')); + } + public function providerTestValidStatusCodes() { return [ From 168ca00555ce020fa9fb754e27e0a23a58097ed1 Mon Sep 17 00:00:00 2001 From: Guy Sartorelli Date: Tue, 25 Oct 2022 11:35:05 +1300 Subject: [PATCH 3/7] [CVE-2022-38724] Restrict embed shortcode attributes --- .../Shortcodes/EmbedShortcodeProvider.php | 34 +++++++++- .../Shortcodes/EmbedShortcodeProviderTest.php | 64 +++++++++++++++++++ 2 files changed, 97 insertions(+), 1 deletion(-) diff --git a/src/View/Shortcodes/EmbedShortcodeProvider.php b/src/View/Shortcodes/EmbedShortcodeProvider.php index 42f8e947a..7d6f2bbc7 100644 --- a/src/View/Shortcodes/EmbedShortcodeProvider.php +++ b/src/View/Shortcodes/EmbedShortcodeProvider.php @@ -16,6 +16,7 @@ use SilverStripe\View\HTML; use SilverStripe\View\Parsers\ShortcodeHandler; use SilverStripe\View\Parsers\ShortcodeParser; use SilverStripe\Control\Director; +use SilverStripe\Core\Config\Configurable; use SilverStripe\Dev\Deprecation; use SilverStripe\View\Embed\EmbedContainer; @@ -26,6 +27,23 @@ use SilverStripe\View\Embed\EmbedContainer; */ class EmbedShortcodeProvider implements ShortcodeHandler { + use Configurable; + + /** + * A whitelist of shortcode attributes which are allowed in the resultant markup. + * Note that the tinymce plugin restricts attributes on the client-side separately. + * + * @config + * @deprecated 4.12.0 Removed without equivalent functionality to replace it + */ + private static array $attribute_whitelist = [ + 'url', + 'thumbnail', + 'class', + 'width', + 'height', + 'caption', + ]; /** * Gets the list of shortcodes provided by this handler @@ -207,9 +225,17 @@ class EmbedShortcodeProvider implements ShortcodeHandler } } + $attributes = static::buildAttributeListFromArguments($arguments, ['width', 'height', 'url', 'caption']); + if (array_key_exists('style', $arguments)) { + $attributes->push(ArrayData::create([ + 'Name' => 'style', + 'Value' => Convert::raw2att($arguments['style']), + ])); + } + $data = [ 'Arguments' => $arguments, - 'Attributes' => static::buildAttributeListFromArguments($arguments, ['width', 'height', 'url', 'caption']), + 'Attributes' => $attributes, 'Content' => DBField::create_field('HTMLFragment', $content) ]; @@ -264,6 +290,12 @@ class EmbedShortcodeProvider implements ShortcodeHandler */ private static function buildAttributeListFromArguments(array $arguments, array $exclude = []): ArrayList { + // Clean out any empty arguments and anything not whitelisted + $whitelist = static::config()->get('attribute_whitelist'); + $arguments = array_filter($arguments, function ($value, $key) use ($whitelist) { + return in_array($key, $whitelist) && strlen(trim($value ?? '')); + }, ARRAY_FILTER_USE_BOTH); + $attributes = ArrayList::create(); foreach ($arguments as $key => $value) { if (in_array($key, $exclude ?? [])) { diff --git a/tests/php/View/Shortcodes/EmbedShortcodeProviderTest.php b/tests/php/View/Shortcodes/EmbedShortcodeProviderTest.php index 0fbab6386..a485793a6 100644 --- a/tests/php/View/Shortcodes/EmbedShortcodeProviderTest.php +++ b/tests/php/View/Shortcodes/EmbedShortcodeProviderTest.php @@ -3,6 +3,7 @@ namespace SilverStripe\View\Tests\Shortcodes; use Psr\SimpleCache\CacheInterface; +use SilverStripe\Core\Config\Config; use SilverStripe\View\Parsers\ShortcodeParser; use SilverStripe\View\Shortcodes\EmbedShortcodeProvider; use SilverStripe\Dev\SapphireTest; @@ -186,4 +187,67 @@ class EmbedShortcodeProviderTest extends EmbedUnitTest EmbedShortcodeProvider::flushCachedShortcodes($parser, $content); $this->assertFalse($cache->has($key)); } + + public function testOnlyWhitelistedAttributesAllowed() + { + $url = 'https://www.youtube.com/watch?v=dM15HfUYwF0'; + $html = $this->getShortcodeHtml( + $url, + $url, + << + EOT, + << $url, + 'caption' => 'A nice video', + 'width' => 778, + 'height' => 437, + 'data-some-value' => 'my-data', + 'onmouseover' => 'alert(2)', + 'style' => 'background-color:red;', + ], + ); + $this->assertEqualIgnoringWhitespace( + <<

A nice video

+ EOT, + $html + ); + } + + public function testWhitelistIsConfigurable() + { + // Allow new whitelisted attribute + Config::modify()->merge(EmbedShortcodeProvider::class, 'attribute_whitelist', ['data-some-value']); + + $url = 'https://www.youtube.com/watch?v=dM15HfUYwF0'; + $html = $this->getShortcodeHtml( + $url, + $url, + << + EOT, + << $url, + 'caption' => 'A nice video', + 'width' => 779, + 'height' => 437, + 'data-some-value' => 'my-data', + 'onmouseover' => 'alert(2)', + 'style' => 'background-color:red;', + ], + ); + $this->assertEqualIgnoringWhitespace( + <<

A nice video

+ EOT, + $html + ); + } } From 49e637d244cf59afaace2cd3e1e8178fac4c411c Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Thu, 10 Nov 2022 11:36:56 +1300 Subject: [PATCH 4/7] MNT Explicitly test with blowfish --- tests/php/Security/SecurityDefaultAdminTest.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/php/Security/SecurityDefaultAdminTest.php b/tests/php/Security/SecurityDefaultAdminTest.php index ec691936b..082e7895b 100644 --- a/tests/php/Security/SecurityDefaultAdminTest.php +++ b/tests/php/Security/SecurityDefaultAdminTest.php @@ -7,6 +7,7 @@ use SilverStripe\Security\Member; use SilverStripe\Security\PasswordEncryptor; use SilverStripe\Security\Permission; use SilverStripe\Security\DefaultAdminService; +use SilverStripe\Security\Security; class SecurityDefaultAdminTest extends SapphireTest { @@ -35,6 +36,7 @@ class SecurityDefaultAdminTest extends SapphireTest $this->defaultUsername = null; $this->defaultPassword = null; } + Security::config()->set('password_encryption_algorithm', 'blowfish'); DefaultAdminService::setDefaultAdmin('admin', 'password'); Permission::reset(); } From 78b661dcf6fa931c8324b65672e00fdcc585b9c0 Mon Sep 17 00:00:00 2001 From: Lee Bradley Date: Wed, 9 Nov 2022 15:36:10 +0000 Subject: [PATCH 5/7] Prevent infinite loop when getting table name for ComponentID If the field isn't in the first 2 classes then would just continue to loop Fix means it will continue going to parent classes Can be seen in the UsedOnTable in `admin` module if you have injected a new `Image` class that extends the built in one --- src/ORM/RelatedData/StandardRelatedDataService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ORM/RelatedData/StandardRelatedDataService.php b/src/ORM/RelatedData/StandardRelatedDataService.php index 2c3eab624..04501fc23 100644 --- a/src/ORM/RelatedData/StandardRelatedDataService.php +++ b/src/ORM/RelatedData/StandardRelatedDataService.php @@ -162,7 +162,7 @@ class StandardRelatedDataService implements RelatedDataService $tableName = $this->dataObjectSchema->tableName($candidateClass); break; } - $candidateClass = get_parent_class($class ?? ''); + $candidateClass = get_parent_class($candidateClass ?? ''); } return $tableName; } From 4308a93cc81a75227bcbfc1abd4aaf5e21ef21ee Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Mon, 21 Nov 2022 13:01:32 +1300 Subject: [PATCH 6/7] [CVE-2022-38148] Validate SortColumn exists --- .../GridField/GridFieldSortableHeader.php | 11 ++++++ .../GridField/GridFieldSortableHeaderTest.php | 36 +++++++++++++++---- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/Forms/GridField/GridFieldSortableHeader.php b/src/Forms/GridField/GridFieldSortableHeader.php index c9e9e03e5..61cf81ca9 100644 --- a/src/Forms/GridField/GridFieldSortableHeader.php +++ b/src/Forms/GridField/GridFieldSortableHeader.php @@ -11,6 +11,7 @@ use SilverStripe\ORM\DataObject; use SilverStripe\View\ArrayData; use SilverStripe\View\SSViewer; use LogicException; +use SilverStripe\Core\Injector\Injector; /** * GridFieldSortableHeader adds column headers to a {@link GridField} that can @@ -271,6 +272,16 @@ class GridFieldSortableHeader implements GridField_HTMLProvider, GridField_DataM return $dataList; } + // Prevent SQL Injection by validating that SortColumn exists + /** @var GridFieldDataColumns $columns */ + $columns = $gridField->getConfig()->getComponentByType(GridFieldDataColumns::class); + $fields = $columns->getDisplayFields($gridField); + if (!array_key_exists($state->SortColumn, $fields) && + !in_array($state->SortColumn, $this->getFieldSorting()) + ) { + throw new LogicException('Invalid SortColumn: ' . $state->SortColumn); + } + return $dataList->sort($state->SortColumn, $state->SortDirection('asc')); } diff --git a/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php b/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php index 5973f1589..5a48de8ec 100644 --- a/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php +++ b/tests/php/Forms/GridField/GridFieldSortableHeaderTest.php @@ -71,17 +71,18 @@ class GridFieldSortableHeaderTest extends SapphireTest $list = Team::get()->filter([ 'ClassName' => Team::class ]); $config = new GridFieldConfig_RecordEditor(); $gridField = new GridField('testfield', 'testfield', $list, $config); + $component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class); // Test normal sorting + $component->setFieldSorting(['Name' => 'City']); $state = $gridField->State->GridFieldSortableHeader; $state->SortColumn = 'City'; $state->SortDirection = 'asc'; - $compontent = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class); - $listA = $compontent->getManipulatedData($gridField, $list); + $listA = $component->getManipulatedData($gridField, $list); $state->SortDirection = 'desc'; - $listB = $compontent->getManipulatedData($gridField, $list); + $listB = $component->getManipulatedData($gridField, $list); $this->assertEquals( ['Auckland', 'Cologne', 'Melbourne', 'Wellington'], @@ -93,12 +94,13 @@ class GridFieldSortableHeaderTest extends SapphireTest ); // Test one relation 'deep' + $component->setFieldSorting(['Name' => 'Cheerleader.Name']); $state->SortColumn = 'Cheerleader.Name'; $state->SortDirection = 'asc'; - $relationListA = $compontent->getManipulatedData($gridField, $list); + $relationListA = $component->getManipulatedData($gridField, $list); $state->SortDirection = 'desc'; - $relationListB = $compontent->getManipulatedData($gridField, $list); + $relationListB = $component->getManipulatedData($gridField, $list); $this->assertEquals( ['Wellington', 'Melbourne', 'Cologne', 'Auckland'], @@ -110,12 +112,13 @@ class GridFieldSortableHeaderTest extends SapphireTest ); // Test two relations 'deep' + $component->setFieldSorting(['Name' => 'Cheerleader.Hat.Colour']); $state->SortColumn = 'Cheerleader.Hat.Colour'; $state->SortDirection = 'asc'; - $relationListC = $compontent->getManipulatedData($gridField, $list); + $relationListC = $component->getManipulatedData($gridField, $list); $state->SortDirection = 'desc'; - $relationListD = $compontent->getManipulatedData($gridField, $list); + $relationListD = $component->getManipulatedData($gridField, $list); $this->assertEquals( ['Cologne', 'Auckland', 'Wellington', 'Melbourne'], @@ -139,6 +142,7 @@ class GridFieldSortableHeaderTest extends SapphireTest $component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class); // Test that inherited dataobjects will work correctly + $component->setFieldSorting(['Name' => 'Cheerleader.Hat.Colour']); $state->SortColumn = 'Cheerleader.Hat.Colour'; $state->SortDirection = 'asc'; $relationListA = $component->getManipulatedData($gridField, $list); @@ -179,6 +183,7 @@ class GridFieldSortableHeaderTest extends SapphireTest ); // Test subclasses of tables + $component->setFieldSorting(['Name' => 'CheerleadersMom.Hat.Colour']); $state->SortColumn = 'CheerleadersMom.Hat.Colour'; $state->SortDirection = 'asc'; $relationListB = $component->getManipulatedData($gridField, $list); @@ -229,4 +234,21 @@ class GridFieldSortableHeaderTest extends SapphireTest $relationListBdesc->column('City') ); } + + public function testSortColumnValidation() + { + $this->expectException(\LogicException::class); + $this->expectExceptionMessage('Invalid SortColumn: INVALID'); + + $list = Team::get()->filter([ 'ClassName' => Team::class ]); + $config = new GridFieldConfig_RecordEditor(); + $gridField = new GridField('testfield', 'testfield', $list, $config); + $component = $gridField->getConfig()->getComponentByType(GridFieldSortableHeader::class); + + $state = $gridField->State->GridFieldSortableHeader; + $state->SortColumn = 'INVALID'; + $state->SortDirection = 'asc'; + + $component->getManipulatedData($gridField, $list); + } } From fe13856769492a9bf584c824843153864cf8c725 Mon Sep 17 00:00:00 2001 From: Steve Boyd Date: Mon, 21 Nov 2022 13:06:40 +1300 Subject: [PATCH 7/7] [CVE-2022-37429] Sanitise XSS --- src/Forms/HTMLEditor/HTMLEditorSanitiser.php | 4 ++-- .../HTMLEditor/HTMLEditorSanitiserTest.php | 24 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php index b35fcf19a..a075d98fa 100644 --- a/src/Forms/HTMLEditor/HTMLEditorSanitiser.php +++ b/src/Forms/HTMLEditor/HTMLEditorSanitiser.php @@ -347,9 +347,9 @@ class HTMLEditorSanitiser } // Matches "javascript:" with any arbitrary linebreaks inbetween the characters. - $regex = '/^\s*' . implode('\v*', str_split('javascript:')) . '/i'; + $regex = '/^\s*' . implode('\s*', str_split('javascript:')) . '/i'; // Strip out javascript execution in href or src attributes. - foreach (['src', 'href'] as $dangerAttribute) { + foreach (['src', 'href', 'data'] as $dangerAttribute) { if ($el->hasAttribute($dangerAttribute)) { if (preg_match($regex, $el->getAttribute($dangerAttribute))) { $el->removeAttribute($dangerAttribute); diff --git a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php index 14f6771a5..3d5c3d5c6 100644 --- a/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php +++ b/tests/php/Forms/HTMLEditor/HTMLEditorSanitiserTest.php @@ -104,6 +104,30 @@ class HTMLEditorSanitiserTest extends FunctionalTest '', 'Mixed case javascript in the src attribute of an iframe is completely removed' ], + [ + 'iframe[src]', + "", + '', + 'Javascript with tab elements the src attribute of an iframe is completely removed' + ], + [ + 'object[data]', + '', + '', + 'Object with OK content in the data attribute is retained' + ], + [ + 'object[data]', + '', + '', + 'Object with dangerous content in data attribute is completely removed' + ], + [ + 'img[src]', + '', + '', + 'XSS vulnerable attributes starting with on or style are removed via configuration' + ], ]; $config = HTMLEditorConfig::get('htmleditorsanitisertest');