Commit 878777a8 authored by webchick's avatar webchick

Issue #2099741 by Wim Leers, wwalc, mr.baileys, eaton, dstol, nod_,...

Issue #2099741 by Wim Leers, wwalc, mr.baileys, eaton, dstol, nod_, effulgentsia: Protect WYSIWYG Editors from XSS Without Destroying User Data.
parent 3ad400ec
......@@ -12,6 +12,18 @@
*/
class Xss {
/**
* Indicates that XSS filtering must be applied in whitelist mode: only
* specified HTML tags are allowed.
*/
const FILTER_MODE_WHITELIST = TRUE;
/**
* Indicates that XSS filtering must be applied in blacklist mode: only
* specified HTML tags are disallowed.
*/
const FILTER_MODE_BLACKLIST = FALSE;
/**
* The list of html tags allowed by filterAdmin().
*
......@@ -35,10 +47,14 @@ class Xss {
* javascript:).
*
* @param $string
* The string with raw HTML in it. It will be stripped of everything that can
* cause an XSS attack.
* @param array $allowed_tags
* An array of allowed tags.
* The string with raw HTML in it. It will be stripped of everything that
* can cause an XSS attack.
* @param array $html_tags
* An array of HTML tags.
* @param bool $mode
* (optional) Defaults to FILTER_MODE_WHITELIST ($html_tags is used as a
* whitelist of allowed tags), but can also be set to FILTER_MODE_BLACKLIST
* ($html_tags is used as a blacklist of disallowed tags).
*
* @return string
* An XSS safe version of $string, or an empty string if $string is not
......@@ -48,14 +64,14 @@ class Xss {
*
* @ingroup sanitization
*/
public static function filter($string, $allowed_tags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd')) {
public static function filter($string, $html_tags = array('a', 'em', 'strong', 'cite', 'blockquote', 'code', 'ul', 'ol', 'li', 'dl', 'dt', 'dd'), $mode = Xss::FILTER_MODE_WHITELIST) {
// Only operate on valid UTF-8 strings. This is necessary to prevent cross
// site scripting issues on Internet Explorer 6.
if (!Unicode::validateUtf8($string)) {
return '';
}
// Store the text format.
static::split($allowed_tags, TRUE);
static::split($html_tags, TRUE, $mode);
// Remove NULL characters (ignored by some browsers).
$string = str_replace(chr(0), '', $string);
// Remove Netscape 4 JS entities.
......@@ -80,7 +96,7 @@ public static function filter($string, $allowed_tags = array('a', 'em', 'strong'
<[^>]*(>|$) # a string that starts with a <, up until the > or the end of the string
| # or
> # just a >
)%x', '\Drupal\Component\Utility\Xss::split', $string);
)%x', 'static::split', $string);
}
/**
......@@ -112,17 +128,22 @@ public static function filterAdmin($string) {
* If $store is TRUE then the array contains the allowed tags.
* If $store is FALSE then the array has one element, the HTML tag to process.
* @param bool $store
* Whether to store $m.
* Whether to store $matches.
* @param bool $mode
* (optional) Ignored when $store is FALSE, otherwise used to determine
* whether $matches is a list of allowed (if FILTER_MODE_WHITELIST) or
* disallowed (if FILTER_MODE_BLACKLIST) HTML tags.
*
* @return string
* If the element isn't allowed, an empty string. Otherwise, the cleaned up
* version of the HTML element.
*/
protected static function split($matches, $store = FALSE) {
static $allowed_html;
protected static function split($matches, $store = FALSE, $mode = Xss::FILTER_MODE_WHITELIST) {
static $html_tags, $split_mode;
if ($store) {
$allowed_html = array_flip($matches);
$html_tags = array_flip($matches);
$split_mode = $mode;
return;
}
......@@ -151,8 +172,12 @@ protected static function split($matches, $store = FALSE) {
$elem = '!--';
}
if (!isset($allowed_html[strtolower($elem)])) {
// Disallowed HTML element.
// When in whitelist mode, an element is disallowed when not listed.
if ($split_mode === static::FILTER_MODE_WHITELIST && !isset($html_tags[strtolower($elem)])) {
return '';
}
// When in blacklist mode, an element is disallowed when listed.
elseif ($split_mode === static::FILTER_MODE_BLACKLIST && isset($html_tags[strtolower($elem)])) {
return '';
}
......
......@@ -10,6 +10,7 @@
use Drupal\ckeditor\CKEditorPluginBase;
use Drupal\Component\Utility\NestedArray;
use Drupal\editor\Entity\Editor;
use Drupal\filter\Plugin\FilterInterface;
/**
* Defines the "internal" plugin (i.e. core plugins part of our CKEditor build).
......@@ -287,7 +288,7 @@ protected function generateAllowedContentSetting(Editor $editor) {
// When nothing is disallowed, set allowedContent to true.
$format = entity_load('filter_format', $editor->format);
$filter_types = $format->getFilterTypes();
if (!in_array(FILTER_TYPE_HTML_RESTRICTOR, $filter_types)) {
if (!in_array(FilterInterface::TYPE_HTML_RESTRICTOR, $filter_types)) {
return TRUE;
}
// Generate setting that accurately reflects allowed tags and attributes.
......
......@@ -24,7 +24,8 @@
* id = "ckeditor",
* label = @Translation("CKEditor"),
* supports_content_filtering = TRUE,
* supports_inline_editing = TRUE
* supports_inline_editing = TRUE,
* is_xss_safe = FALSE
* )
*/
class CKEditor extends EditorBase implements ContainerFactoryPluginInterface {
......
......@@ -104,6 +104,7 @@ function testLoading() {
'editor' => 'ckeditor',
'editorSettings' => $ckeditor_plugin->getJSSettings($editor),
'editorSupportsContentFiltering' => TRUE,
'isXssSafe' => FALSE,
)));
$this->assertTrue($editor_settings_present, "Text Editor module's JavaScript settings are on the page.");
$this->assertIdentical($expected, $settings['editor'], "Text Editor module's JavaScript settings on the page are correct.");
......@@ -131,6 +132,7 @@ function testLoading() {
'editor' => 'ckeditor',
'editorSettings' => $ckeditor_plugin->getJSSettings($editor),
'editorSupportsContentFiltering' => TRUE,
'isXssSafe' => FALSE,
)));
$this->assertTrue($editor_settings_present, "Text Editor module's JavaScript settings are on the page.");
$this->assertIdentical($expected, $settings['editor'], "Text Editor module's JavaScript settings on the page are correct.");
......
......@@ -5,6 +5,8 @@
* Documentation for Text Editor API.
*/
use Drupal\filter\FilterFormatInterface;
/**
* @addtogroup hooks
* @{
......@@ -103,6 +105,31 @@ function hook_editor_js_settings_alter(array &$settings, array $formats) {
}
}
/**
* Modifies the text editor XSS filter that will used for the given text format.
*
* Is only called when an EditorXssFilter will effectively be used; this hook
* does not allow one to alter that decision.
*
* @param string &$editor_xss_filter_class
* The text editor XSS filter class that will be used.
* @param \Drupal\filter\FilterFormatInterface $format
* The text format configuration entity. Provides context based upon which
* one may want to adjust the filtering.
* @param \Drupal\filter\FilterFormatInterface $original_format|null
* (optional) The original text format configuration entity (when switching
* text formats/editors). Also provides context based upon which one may want
* to adjust the filtering.
*
* @see \Drupal\editor\EditorXssFilterInterface
*/
function hook_editor_xss_filter_alter(&$editor_xss_filter_class, FilterFormatInterface $format, FilterFormatInterface $original_format = NULL) {
$filters = $format->filters()->getAll();
if (isset($filters['filter_wysiwyg']) && $filters['filter_wysiwyg']->status) {
$editor_xss_filter_class = '\Drupal\filter_wysiwyg\EditorXssFilter\WysiwygFilter';
}
}
/**
* @} End of "addtogroup hooks".
*/
......@@ -10,6 +10,8 @@
use Drupal\Component\Utility\NestedArray;
use Drupal\Core\Entity\EntityInterface;
use Drupal\field\Field;
use Drupal\filter\FilterFormatInterface;
use Drupal\filter\Plugin\FilterInterface;
/**
* Implements hook_help().
......@@ -377,9 +379,101 @@ function editor_pre_render_format($element) {
$manager = \Drupal::service('plugin.manager.editor');
$element['#attached'] = NestedArray::mergeDeep($element['#attached'], $manager->getAttachments($format_ids));
// Apply XSS filters when editing content if necessary. Some types of text
// editors cannot guarantee that the end user won't become a victim of XSS.
if (!empty($element['value']['#value'])) {
$original = $element['value']['#value'];
$format = entity_load('filter_format', $element['format']['format']['#value']);
// Ensure XSS-safety for the current text format/editor.
$filtered = editor_filter_xss($original, $format);
if ($filtered !== FALSE) {
$element['value']['#value'] = $filtered;
}
// Only when the user has access to multiple text formats, we must add data-
// attributes for the original value and change tracking, because they are
// only necessary when the end user can switch between text formats/editors.
if ($element['format']['format']['#access']) {
$element['value']['#attributes']['data-editor-value-is-changed'] = 'false';
$element['value']['#attributes']['data-editor-value-original'] = $original;
}
}
return $element;
}
/**
* Applies text editor XSS filtering.
*
* @param string $html
* The HTML string that will be passed to the text editor.
* @param \Drupal\filter\FilterFormatInterface $format
* The text format whose text editor will be used.
* @param \Drupal\filter\FilterFormatInterface $original_format|null
* (optional) The original text format (i.e. when switching text formats,
* $format is the text format that is going to be used, $original_format is
* the one that was being used initially, the one that is stored in the
* database when editing).
*
* @return string|false
* FALSE when no XSS filtering needs to be applied (either because no text
* editor is associated with the text format, or because the text editor is
* safe from XSS attacks, or because the text format does not use any XSS
* protection filters), otherwise the XSS filtered string.
*
* @see https://drupal.org/node/2099741
*/
function editor_filter_xss($html, FilterFormatInterface $format, FilterFormatInterface $original_format = NULL) {
$editor = editor_load($format->id());
// If no text editor is associated with this text format, then we don't need
// text editor XSS filtering either.
if (!isset($editor)) {
return FALSE;
}
// If the text editor associated with this text format guarantees security,
// then we also don't need text editor XSS filtering.
$definition = \Drupal::service('plugin.manager.editor')->getDefinition($editor->editor);
if ($definition['is_xss_safe'] === TRUE) {
return FALSE;
}
// If there is no filter preventing XSS attacks in the text format being used,
// then no text editor XSS filtering is needed either. (Because then the
// editing user can already be attacked by merely viewing the content.)
// e.g.: an admin user creates content in Full HTML and then edits it, no text
// format switching happens; in this case, no text editor XSS filtering is
// desirable, because it would strip style attributes, amongst others.
$current_filter_types = $format->getFilterTypes();
if (!in_array(FilterInterface::TYPE_HTML_RESTRICTOR, $current_filter_types, TRUE)) {
if ($original_format === NULL) {
return FALSE;
}
// Unless we are switching from another text format, in which case we must
// first check whether a filter preventing XSS attacks is used in that text
// format, and if so, we must still apply XSS filtering.
// e.g.: an anonymous user creates content in Restricted HTML, an admin user
// edits it (then no XSS filtering is applied because no text editor is
// used), and switches to Full HTML (for which a text editor is used). Then
// we must apply XSS filtering to protect the admin user.
else {
$original_filter_types = $original_format->getFilterTypes();
if (!in_array(FilterInterface::TYPE_HTML_RESTRICTOR, $original_filter_types, TRUE)) {
return FALSE;
}
}
}
// Otherwise, apply the text editor XSS filter. We use the default one unless
// a module tells us to use a different one.
$editor_xss_filter_class = '\Drupal\editor\EditorXssFilter\Standard';
\Drupal::moduleHandler()->alter('editor_xss_filter', $editor_xss_filter_class, $format, $original_format);
return call_user_func($editor_xss_filter_class . '::filterXss', $html, $format, $original_format);
}
/**
* Implements hook_entity_insert().
*/
......
editor.filter_xss:
path: '/editor/filter_xss/{filter_format}'
defaults:
_controller: '\Drupal\editor\EditorController::filterXss'
requirements:
_entity_access: 'filter_format.view'
editor.field_untransformed_text:
path: '/editor/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
defaults:
......
......@@ -34,14 +34,22 @@
* attached.
*/
function changeTextEditor($formatSelector, activeFormatID, newFormatID) {
var originalFormatID = activeFormatID;
var field = findFieldForFormatSelector($formatSelector);
// Detach the current editor (if any) and attach a new editor.
if (drupalSettings.editor.formats[activeFormatID]) {
Drupal.editorDetach(field, drupalSettings.editor.formats[activeFormatID]);
}
// When no text editor is currently active, stop tracking changes.
else if (!drupalSettings.editor.formats[activeFormatID]) {
$(field).off('.editor');
}
activeFormatID = newFormatID;
// Attach the new text editor (if any).
if (drupalSettings.editor.formats[activeFormatID]) {
Drupal.editorAttach(field, drupalSettings.editor.formats[activeFormatID]);
var format = drupalSettings.editor.formats[activeFormatID];
filterXssWhenSwitching(field, format, originalFormatID, Drupal.editorAttach);
}
$formatSelector.attr('data-editor-active-text-format', newFormatID);
}
......@@ -135,10 +143,22 @@
$this.attr('data-editor-active-text-format', activeFormatID);
var field = findFieldForFormatSelector($this);
// Directly attach this editor, if the text format is enabled.
// Directly attach this text editor, if the text format is enabled.
if (settings.editor.formats[activeFormatID]) {
// XSS protection for the current text format/editor is performed on the
// server side, so we don't need to do anything special here.
Drupal.editorAttach(field, settings.editor.formats[activeFormatID]);
}
// When there is no text editor for this text format, still track changes,
// because the user has the ability to switch to some text editor, other-
// wise this code would not be executed.
else {
$(field).on('change.editor keypress.editor', function () {
field.setAttribute('data-editor-value-is-changed', 'true');
// Just knowing that the value was changed is enough, stop tracking.
$(field).off('.editor');
});
}
// Attach onChange handler to text format selector element.
if ($this.is('select')) {
......@@ -200,6 +220,10 @@
// happen within the text editor.
Drupal.editors[format.editor].onChange(field, function () {
$(field).trigger('formUpdated');
// Keep track of changes, so we know what to do when switching text
// formats and guaranteeing XSS protection.
field.setAttribute('data-editor-value-is-changed', 'true');
});
}
};
......@@ -214,7 +238,51 @@
}
Drupal.editors[format.editor].detach(field, format, trigger);
// Restore the original value if the user didn't make any changes yet.
if (field.getAttribute('data-editor-value-is-changed') === 'false') {
field.value = field.getAttribute('data-editor-value-original');
}
}
};
/**
* Filter away XSS attack vectors when switching text formats.
*
* @param DOM field
* The textarea DOM element.
* @param Object format
* The text format that's being activated, from drupalSettings.editor.formats.
* @param String originalFormatID
* The text format ID of the original text format.
* @param Function callback
* A callback to be called (with no parameters) after the field's value has
* been XSS filtered.
*/
function filterXssWhenSwitching (field, format, originalFormatID, callback) {
// A text editor that already is XSS-safe needs no additional measures.
if (format.editor.isXssSafe) {
callback(field, format);
}
// Otherwise, ensure XSS safety: let the server XSS filter this value.
else {
$.ajax({
url: Drupal.url('editor/filter_xss/' + format.format),
type: 'POST',
data: {
'value': field.value,
'original_format_id': originalFormatID
},
dataType: 'json',
success: function (xssFilteredValue) {
// If the server returns false, then no XSS filtering is needed.
if (xssFilteredValue !== false) {
field.value = xssFilteredValue;
}
callback(field, format);
}
});
}
}
})(jQuery, Drupal, drupalSettings);
......@@ -42,8 +42,15 @@ class Editor extends Plugin {
/**
* Whether the editor supports the inline editing provided by the Edit module.
*
* @var boolean
* @var bool
*/
public $supports_inline_editing;
/**
* Whether this text editor is not vulnerable to XSS attacks.
*
* @var bool
*/
public $is_xss_safe;
}
......@@ -10,17 +10,21 @@
use Drupal\Core\Ajax\AjaxResponse;
use Drupal\Core\Ajax\OpenModalDialogCommand;
use Drupal\Core\Ajax\CloseModalDialogCommand;
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\Entity\EntityInterface;
use Drupal\editor\Ajax\GetUntransformedTextCommand;
use Drupal\editor\Form\EditorImageDialog;
use Drupal\editor\Form\EditorLinkDialog;
use Drupal\filter\Entity\FilterFormat;
use Symfony\Component\DependencyInjection\ContainerAware;
use Drupal\filter\Plugin\FilterInterface;
use Drupal\filter\FilterFormatInterface;
use Symfony\Component\HttpFoundation\JsonResponse;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
/**
* Returns responses for Editor module routes.
*/
class EditorController extends ContainerAware {
class EditorController extends ControllerBase {
/**
* Returns an Ajax response to render a text field without transformation filters.
......@@ -43,10 +47,41 @@ public function getUntransformedText(EntityInterface $entity, $field_name, $lang
// Direct text editing is only supported for single-valued fields.
$field = $entity->getTranslation($langcode)->$field_name;
$editable_text = check_markup($field->value, $field->format, $langcode, FALSE, array(FILTER_TYPE_TRANSFORM_REVERSIBLE, FILTER_TYPE_TRANSFORM_IRREVERSIBLE));
$editable_text = check_markup($field->value, $field->format, $langcode, FALSE, array(FilterInterface::TYPE_TRANSFORM_REVERSIBLE, FilterInterface::TYPE_TRANSFORM_IRREVERSIBLE));
$response->addCommand(new GetUntransformedTextCommand($editable_text));
return $response;
}
/**
* Apply the necessary XSS filtering for using a certain text format's editor.
*
* @param \Symfony\Component\HttpFoundation\Request $request
* The current request object.
* @param \Drupal\filter\FilterFormatInterface $filter_format
* The text format whose text editor (if any) will be used.
*
* @return \Symfony\Component\HttpFoundation\JsonResponse
* A JSON response containing the XSS-filtered value.
*
* @see editor_filter_xss()
*/
public function filterXss(Request $request, FilterFormatInterface $filter_format) {
$value = $request->request->get('value');
if (!isset($value)) {
throw new NotFoundHttpException();
}
// The original_format parameter will only exist when switching text format.
$original_format_id = $request->request->get('original_format_id');
$original_format = NULL;
if (isset($original_format_id)) {
$original_format = $this->entityManager()
->getStorageController('filter_format')
->load($original_format_id);
}
return new JsonResponse(editor_filter_xss($value, $filter_format, $original_format));
}
}
<?php
/**
* @file
* Contains \Drupal\editor\EditorXssFilter\Standard.
*/
namespace Drupal\editor\EditorXssFilter;
use Drupal\Component\Utility\Xss;
use Drupal\filter\FilterFormatInterface;
use Drupal\editor\EditorXssFilterInterface;
/**
* Defines the standard text editor XSS filter.
*/
class Standard implements EditorXssFilterInterface {
/**
* {@inheritdoc}
*/
public static function filterXss($html, FilterFormatInterface $format, FilterFormatInterface $original_format = NULL) {
// Apply XSS filtering, but blacklist the <script>, <style>, <link>, <embed>
// and <object> tags.
// The <script> and <style> tags are blacklisted because their contents
// can be malicious (and therefor they are inherently unsafe), whereas for
// all other tags, only their attributes can make them malicious. Since
// Xss::filter() protects against malicious attributes, we take no
// blacklisting action.
// The exceptions to the above rule are <link>, <embed> and <object>:
// - <link> because the href attribute allows the attacker to import CSS
// using the HTTP(S) protocols which Xss::filter() considers safe by
// default. The imported remote CSS is applied to the main document, thus
// allowing for the same XSS attacks as a regular <style> tag.
// - <embed> and <object> because these tags allow non-HTML applications or
// content to be embedded using the src or data attributes, respectively.
// This is safe in the case of HTML documents, but not in the case of
// Flash objects for example, that may access/modify the main document
// directly.
// <iframe> is considered safe because it only allows HTML content to be
// embedded, hence ensuring the same origin policy always applies.
$dangerous_tags = array('script', 'style', 'link', 'embed', 'object');
// Simply blacklisting these five dangerious tags would bring safety, but
// also user frustration: what if a text format is configured to allow
// <embed>, for example? Then we would strip that tag, even though it is
// allowed, thereby causing data loss!
// Therefor, we want to be smarter still. We want to take into account which
// HTML tags are allowed and forbidden by the text format we're filtering
// for, and if we're switching from another text format, we want to take
// that format's allowed and forbidden tags into account as well.
// In other words: we only expect markup allowed in both the original and
// the new format to continue to exist.
$format_restrictions = $format->getHtmlRestrictions();
if ($original_format !== NULL) {
$original_format_restrictions = $original_format->getHtmlRestrictions();
}
// Any tags that are explicitly blacklisted by the text format must be
// appended to the list of default dangerous tags: if they're explicitly
// forbidden, then we must respect that configuration.
// When switching from another text format, we must use the union of
// forbidden tags: if either text format is more restrictive, then the
// safety expectations of *both* text formats apply.
$forbidden_tags = self::getForbiddenTags($format_restrictions);
if ($original_format !== NULL) {
$forbidden_tags = array_merge($forbidden_tags, self::getForbiddenTags($original_format_restrictions));
}
// Any tags that are explicitly whitelisted by the text format must be
// removed from the list of default dangerous tags: if they're explicitly
// allowed, then we must respect that configuration.
// When switching from another format, we must use the intersection of
// allowed tags: if either format is more restrictive, then the safety
// expectations of *both* formats apply.
$allowed_tags = self::getAllowedTags($format_restrictions);
if ($original_format !== NULL) {
$allowed_tags = array_intersect($allowed_tags, self::getAllowedTags($original_format_restrictions));
}
// Don't blacklist dangerous tags that are explicitly allowed in both text
// formats.
$blacklisted_tags = array_diff($dangerous_tags, $allowed_tags);
// Also blacklist tags that are explicitly forbidden in either text format.
$blacklisted_tags = array_merge($blacklisted_tags, $forbidden