Commit 2f903e47 authored by xjm's avatar xjm

Issue #2273925 by larowlan, aneek, lauriii, mikey_p, joelpittet, dimaro,...

Issue #2273925 by larowlan, aneek, lauriii, mikey_p, joelpittet, dimaro, Fabianx, xjm, jaredsmith, effulgentsia, lokapujya, iMiksu, chx, YesCT, googletorp, dawehner, Wim Leers, Cottser: Ensure #markup is XSS escaped in Renderer::doRender()
parent c29f8956
......@@ -282,4 +282,51 @@ public static function placeholder($text) {
return $string;
}
/**
* Replaces all occurrences of the search string with the replacement string.
*
* Functions identically to str_replace(), but marks the returned output as
* safe if all the inputs and the subject have also been marked as safe.
*
* @param string|array $search
* The value being searched for. An array may be used to designate multiple
* values to search for.
* @param string|array $replace
* The replacement value that replaces found search values. An array may be
* used to designate multiple replacements.
* @param string $subject
* The string or array being searched and replaced on.
*
* @return string
* The passed subject with replaced values.
*/
public static function replace($search, $replace, $subject) {
$output = str_replace($search, $replace, $subject);
// If any replacement is unsafe, then the output is also unsafe, so just
// return the output.
if (!is_array($replace)) {
if (!SafeMarkup::isSafe($replace)) {
return $output;
}
}
else {
foreach ($replace as $replacement) {
if (!SafeMarkup::isSafe($replacement)) {
return $output;
}
}
}
// If the subject is unsafe, then the output is as well, so return it.
if (!SafeMarkup::isSafe($subject)) {
return $output;
}
else {
// If we have reached this point, then all replacements were safe. If the
// subject was also safe, then mark the entire output as safe.
return SafeMarkup::set($output);
}
}
}
......@@ -46,7 +46,12 @@ public function getInfo() {
* Pre-render callback: Renders a generic HTML tag with attributes into #markup.
*
* Note: It is the caller's responsibility to sanitize any input parameters.
* This callback does not perform sanitization.
* This callback does not perform sanitization. Despite the result of this
* pre-render callback being a #markup element, it is not passed through
* \Drupal\Component\Utility\Xss::filterAdmin(). This is because it is marked
* safe here, which causes
* \Drupal\Component\Utility\SafeMarkup::checkAdminXss() to regard it as safe
* and bypass the call to \Drupal\Component\Utility\Xss::filterAdmin().
*
* @param array $element
* An associative array containing:
......@@ -94,7 +99,7 @@ public static function preRenderHtmlTag($element) {
$markup = SafeMarkup::set($markup);
}
if (!empty($element['#noscript'])) {
$element['#markup'] = '<noscript>' . $markup . '</noscript>';
$element['#markup'] = SafeMarkup::format('<noscript>@markup</noscript>', ['@markup' => $markup]);
}
else {
$element['#markup'] = $markup;
......
......@@ -350,9 +350,10 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
$elements['#children'] = '';
}
// @todo Simplify after https://www.drupal.org/node/2273925.
if (isset($elements['#markup'])) {
$elements['#markup'] = SafeMarkup::set($elements['#markup']);
// @todo Decide how to support non-HTML in the render API in
// https://www.drupal.org/node/2501313.
$elements['#markup'] = SafeMarkup::checkAdminXss($elements['#markup']);
}
// Assume that if #theme is set it represents an implemented hook.
......@@ -609,7 +610,7 @@ protected function createPlaceholder(array $element) {
$attributes['callback'] = $placeholder_render_array['#lazy_builder'][0];
$attributes['arguments'] = UrlHelper::buildQuery($placeholder_render_array['#lazy_builder'][1]);
$attributes['token'] = hash('sha1', serialize($placeholder_render_array));
$placeholder_markup = '<drupal-render-placeholder' . $attributes . '></drupal-render-placeholder>';
$placeholder_markup = SafeMarkup::format('<drupal-render-placeholder@attributes></drupal-render-placeholder>', ['@attributes' => $attributes]);
// Build the placeholder element to return.
$placeholder_element = [];
......
......@@ -9,6 +9,7 @@
use Drupal\Core\Template\Attribute;
use Drupal\Core\Render\Element\RenderElement;
use Drupal\Component\Utility\SafeMarkup;
/**
* Provides a contextual_links_placeholder element.
......@@ -47,7 +48,8 @@ public function getInfo() {
* @see _contextual_links_to_id()
*/
public static function preRenderPlaceholder(array $element) {
$element['#markup'] = '<div' . new Attribute(array('data-contextual-id' => $element['#id'])) . '></div>';
$element['#markup'] = SafeMarkup::format('<div@attributes></div>', ['@attributes' => new Attribute(['data-contextual-id' => $element['#id']])]);
return $element;
}
......
......@@ -8,6 +8,7 @@
namespace Drupal\filter\Element;
use Drupal\Component\Utility\NestedArray;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Render\BubbleableMetadata;
use Drupal\Core\Render\Element\RenderElement;
......@@ -118,9 +119,15 @@ public static function preRenderText($element) {
}
}
// Filtering done, store in #markup, set the updated bubbleable rendering
// metadata, and set the text format's cache tag.
$element['#markup'] = $text;
// Filtering and sanitizing have been done in
// \Drupal\filter\Plugin\FilterInterface. $text is not guaranteed to be
// safe, but it has been passed through the filter system and checked with
// a text format, so it must be printed as is. (See the note about security
// in the method documentation above.)
$element['#markup'] = SafeMarkup::set($text);
// Set the updated bubbleable rendering metadata and the text format's
// cache tag.
$metadata->applyTo($element);
$element['#cache']['tags'] = Cache::mergeTags($element['#cache']['tags'], $format->getCacheTags());
......
......@@ -284,6 +284,20 @@ public function execute() {
$header = [];
$header['Content-type'] = $this->getMimeType();
if ($this->view->getRequest()->getFormat($header['Content-type']) !== 'html') {
// This display plugin is primarily for returning non-HTML formats.
// However, we still invoke the renderer to collect cacheability metadata.
// Because the renderer is designed for HTML rendering, it filters
// #markup for XSS unless it is already known to be safe, but that filter
// only works for HTML. Therefore, we mark the contents as safe to bypass
// the filter. So long as we are returning this in a non-HTML response
// (checked above), this is safe, because an XSS attack only works when
// executed by an HTML agent.
// @todo Decide how to support non-HTML in the render API in
// https://www.drupal.org/node/2501313.
$output['#markup'] = SafeMarkup::set($output['#markup']);
}
$response = new CacheableResponse($this->renderer->renderRoot($output), 200);
$cache_metadata = CacheableMetadata::createFromRenderArray($output);
......
......@@ -390,6 +390,16 @@ public function testFieldapiField() {
$result = $this->drupalGetJSON('test/serialize/node-field');
$this->assertEqual($result[0]['nid'], $node->id());
$this->assertEqual($result[0]['body'], $node->body->processed);
}
// Make sure that serialized fields are not exposed to XSS.
$node = $this->drupalCreateNode();
$node->body = [
'value' => '<script type="text/javascript">alert("node-body");</script>' . $this->randomMachineName(32),
'format' => filter_default_format(),
];
$node->save();
$result = $this->drupalGetJSON('test/serialize/node-field');
$this->assertEqual($result[1]['nid'], $node->id());
$this->assertTrue(strpos($this->getRawContent(), "<script") === FALSE, "No script tag is present in the raw page contents.");
}
}
......@@ -262,10 +262,19 @@
* and to discover available theme hooks, see the documentation of
* hook_theme() and the
* @link themeable Default theme implementations topic. @endlink
* - #markup: Specifies that the array provides HTML markup directly. Unless the
* markup is very simple, such as an explanation in a paragraph tag, it is
* normally preferable to use #theme or #type instead, so that the theme can
* customize the markup.
* - #markup: Specifies that the array provides HTML markup directly. Unless
* the markup is very simple, such as an explanation in a paragraph tag, it
* is normally preferable to use #theme or #type instead, so that the theme
* can customize the markup. Note that the value is passed through
* \Drupal\Component\Utility\Xss::filterAdmin(), which strips known XSS
* vectors while allowing a permissive list of HTML tags that are not XSS
* vectors. (I.e, <script> and <style> are not allowed.) See
* \Drupal\Component\Utility\Xss::$adminTags for the list of tags that will
* be allowed. If your markup needs any of the tags that are not in this
* whitelist, then you should implement a theme hook and template file and/or
* an asset library.
* @see core.libraries.yml
* @see hook_theme()
*
* JavaScript and CSS assets are specified in the render array using the
* #attached property (see @ref sec_attached).
......
......@@ -7,6 +7,7 @@
namespace Drupal\views\Tests\Handler;
use Drupal\Component\Utility\Xss;
use Drupal\views\Views;
/**
......@@ -90,11 +91,12 @@ public function testRenderArea() {
$view = Views::getView('test_example_area');
$view->initHandlers();
// Insert a random string to the test area plugin and see whether it is
// rendered for both header, footer and empty text.
$header_string = $this->randomString();
$footer_string = $this->randomString();
$empty_string = $this->randomString();
// Insert a random string with XSS injection in the test area plugin.
// Ensure that the string is rendered for the header, footer, and empty
// text with the markup properly escaped.
$header_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
$footer_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
$empty_string = '<script type="text/javascript">alert("boo");</script><p>' . $this->randomMachineName() . '</p>';
$view->header['test_example']->options['string'] = $header_string;
$view->header['test_example']->options['empty'] = TRUE;
......@@ -104,12 +106,13 @@ public function testRenderArea() {
$view->empty['test_example']->options['string'] = $empty_string;
// Check whether the strings exists in the output.
// Check whether the strings exist in the output and are sanitized.
$output = $view->preview();
$output = drupal_render($output);
$this->assertTrue(strpos($output, $header_string) !== FALSE);
$this->assertTrue(strpos($output, $footer_string) !== FALSE);
$this->assertTrue(strpos($output, $empty_string) !== FALSE);
$this->assertTrue(strpos($output, Xss::filterAdmin($header_string)) !== FALSE, 'Views header exists in the output and is sanitized');
$this->assertTrue(strpos($output, Xss::filterAdmin($footer_string)) !== FALSE, 'Views footer exists in the output and is sanitized');
$this->assertTrue(strpos($output, Xss::filterAdmin($empty_string)) !== FALSE, 'Views empty exists in the output and is sanitized');
$this->assertTrue(strpos($output, '<script') === FALSE, 'Script tags were escaped');
}
/**
......
......@@ -695,7 +695,7 @@ function views_pre_render_views_form_views_form($element) {
}
// Apply substitutions to the rendered output.
$element['output'] = array('#markup' => str_replace($search, $replace, drupal_render($element['output'])));
$element['output'] = ['#markup' => SafeMarkup::replace($search, $replace, drupal_render($element['output']))];
// Sort, render and add remaining form fields.
$children = Element::children($element, TRUE);
......
......@@ -147,21 +147,35 @@ function theme_views_ui_build_group_filter_form($variables) {
foreach (Element::children($form['group_items']) as $group_id) {
$form['group_items'][$group_id]['value']['#title'] = '';
$default = [
$form['default_group'][$group_id],
$form['default_group_multiple'][$group_id],
];
$link = [
'#type' => 'link',
'#url' => Url::fromRoute('<none>', [], [
'attributes' => [
'id' => 'views-remove-link-' . $group_id,
'class' => [
'views-hidden',
'views-button-remove',
'views-groups-remove-link',
'views-remove-link',
],
'alt' => t('Remove this item'),
'title' => t('Remove this item'),
],
]),
'#title' => SafeMarkup::format('<span>@text</span>', ['@text' => t('Remove')]),
];
$remove = [$form['group_items'][$group_id]['remove'], $link];
$data = array(
'default' => array(
'data' => array(
'#markup' => drupal_render($form['default_group'][$group_id]) . drupal_render($form['default_group_multiple'][$group_id]),
),
),
'default' => ['data' => $default],
'weight' => drupal_render($form['group_items'][$group_id]['weight']),
'title' => drupal_render($form['group_items'][$group_id]['title']),
'operator' => drupal_render($form['group_items'][$group_id]['operator']),
'value' => drupal_render($form['group_items'][$group_id]['value']),
'remove' => array(
'data' => array(
'#markup' => drupal_render($form['group_items'][$group_id]['remove']) . \Drupal::l(SafeMarkup::format('<span>@text</span>', array('@text' => t('Remove'))), Url::fromRoute('<none>', [], array('attributes' => array('id' => 'views-remove-link-' . $group_id, 'class' => array('views-hidden', 'views-button-remove', 'views-groups-remove-link', 'views-remove-link'), 'alt' => t('Remove this item'), 'title' => t('Remove this item'))))),
),
),
'remove' => ['data' => $remove],
);
$rows[] = array('data' => $data, 'id' => 'views-row-' . $group_id, 'class' => array('draggable'));
}
......
......@@ -188,4 +188,73 @@ function testPlaceholder() {
$this->assertEquals('<em class="placeholder">Some text</em>', SafeMarkup::placeholder('Some text'));
}
/**
* Tests SafeMarkup::replace().
*
* @dataProvider providerReplace
* @covers ::replace
*/
public function testReplace($search, $replace, $subject, $expected, $is_safe) {
$result = SafeMarkup::replace($search, $replace, $subject);
$this->assertEquals($expected, $result);
$this->assertEquals($is_safe, SafeMarkup::isSafe($result));
}
/**
* Data provider for testReplace().
*
* @see testReplace()
*/
public function providerReplace() {
$tests = [];
// Subject unsafe.
$tests[] = [
'<placeholder>',
SafeMarkup::set('foo'),
'<placeholder>bazqux',
'foobazqux',
FALSE,
];
// All safe.
$tests[] = [
'<placeholder>',
SafeMarkup::set('foo'),
SafeMarkup::set('<placeholder>barbaz'),
'foobarbaz',
TRUE,
];
// Safe subject, but should result in unsafe string because replacement is
// unsafe.
$tests[] = [
'<placeholder>',
'fubar',
SafeMarkup::set('<placeholder>barbaz'),
'fubarbarbaz',
FALSE,
];
// Array with all safe.
$tests[] = [
['<placeholder1>', '<placeholder2>', '<placeholder3>'],
[SafeMarkup::set('foo'), SafeMarkup::set('bar'), SafeMarkup::set('baz')],
SafeMarkup::set('<placeholder1><placeholder2><placeholder3>'),
'foobarbaz',
TRUE,
];
// Array with unsafe replacement.
$tests[] = [
['<placeholder1>', '<placeholder2>', '<placeholder3>',],
[SafeMarkup::set('bar'), SafeMarkup::set('baz'), 'qux'],
SafeMarkup::set('<placeholder1><placeholder2><placeholder3>'),
'barbazqux',
FALSE,
];
return $tests;
}
}
......@@ -8,6 +8,7 @@
namespace Drupal\Tests\Core\Render;
use Drupal\Component\Utility\Html;
use Drupal\Component\Utility\SafeMarkup;
use Drupal\Core\Render\Element;
use Drupal\Core\Cache\Cache;
......@@ -61,7 +62,11 @@ public function providerPlaceholders() {
unset($token_render_array['#cache']);
}
$token = hash('sha1', serialize($token_render_array));
return '<drupal-render-placeholder callback="Drupal\Tests\Core\Render\PlaceholdersTest::callback" arguments="0=' . $args[0] . '" token="' . $token . '"></drupal-render-placeholder>';
return SafeMarkup::format('<drupal-render-placeholder callback="@callback" arguments="@arguments" token="@token"></drupal-render-placeholder>', [
'@callback' => 'Drupal\Tests\Core\Render\PlaceholdersTest::callback',
'@arguments' => '0=' . $args[0],
'@token' => $token,
]);
};
$base_element_a1 = [
......
......@@ -80,6 +80,14 @@ public function providerTestRenderBasic() {
$data[] = [[
'child' => ['#markup' => 'bar'],
], 'bar'];
// XSS filtering test.
$data[] = [[
'child' => ['#markup' => "This is <script>alert('XSS')</script> test"],
], "This is alert('XSS') test"];
// Ensure non-XSS tags are not filtered out.
$data[] = [[
'child' => ['#markup' => "This is <strong><script>alert('not a giraffe')</script></strong> test"],
], "This is <strong>alert('not a giraffe')</strong> test"];
// #children set but empty, and renderable children.
$data[] = [[
'#children' => '',
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment