Problem/Motivation

template_preprocess_color_scheme_form calls SafeMarkup::set() which is meant to be for internal use only.

Proposed resolution

  • Remove the call by refactoring the code. (COMPLETED)
  • If refactoring is not possible, thoroughly document where the string is coming from and why it is safe, and why SafeMarkup::set() is required.

Remaining tasks

  1. Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 (DONE)
  2. Identify whether there is existing automated test coverage for the sanitization of the string. If there is, list the test in the issue summary. If there isn't, add an automated test for it.
  3. If the string cannot be refactored, the SafeMarkup::set() usage needs to be thoroughly audited and documented. NOT APPLICABLE

Manual testing steps (for double escaping)

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Enable the Color module and Bartik theme (not required if using Standard install profile as these are already enabled)
  3. Visit Bartik's setting page at admin/appearance/settings/bartik
  4. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.

User interface changes

N/A

API changes

N/A

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cdulude’s picture

Converted $variables['html_preview'] = SafeMarkup::set(file_get_contents($preview_html_path)); into an inline template.

Manual testing by going into Appearance section of site, modifying color settings on Bartik theme. Site didn't break.

cdulude’s picture

Status: Active » Needs review
star-szr’s picture

Status: Needs review » Needs work

I suggested this approach at the sprint. I don't think it's any less secure than the previous code but would probably be nice to hear from someone on the security team on this one.

+++ b/core/modules/color/color.module
@@ -284,7 +284,11 @@ function template_preprocess_color_scheme_form(&$variables) {
+  $variables['html_preview'] = array (
+    '#type' => 'inline_template',
+    '#template' => file_get_contents($preview_html_path),
+  );

Minor (coding standards): Either remove the space between array and (, or convert to short array syntax (square brackets). https://www.drupal.org/coding-standards#array

joelpittet’s picture

Status: Needs work » Needs review
FileSize
726 bytes

Try this on for size.

cdulude’s picture

Thanks for the heads-up on the errant space in array() @Cottser!

@joelpittet : Thanks for the alternate solution! Just curious, why is SafeMarkup::checkAdminXss() okay, whereas SafeMarkup::set() is not, as far as being for internal use? (or not?)

joelpittet’s picture

@cdulude good question. SafeMarkup::checkAdminXss() will run Xss::filterAdmin() (which I guess we could just run directly, maybe that's better?) And that will filter out the tags and attributes that may be harmful in an Xss attack.

Though on the other hand, in this case we are loading a file in the theme directory and it's very unlikely that there will be an attack from that kind of file.

We can go either way but this will save us from having to justify it's usage and the very slight chance we are wrong and someone finds out how to exploit this.

@cdulude What do you think?

cdulude’s picture

@joelpittet Thanks -- that makes sense. I appreciate the thorough explanation!

One more question if you wouldn't mind: Was @Cottser's rationale for suggesting the inline template in comment #3 along the lines of: once the data is passed to a Twig template, the auto-escaping would sanitize any potential security problems; but then your suggestion of using SafeMarkup::checkAdminXss() accomplishes basically the same thing, but with less code?

(Sorry if this is really basic knowledge; last weekend was my first time digging into D8 code, so I'm trying to figure this all out!)

joelpittet’s picture

#7 it's not basic knowledge, there is actually another issue where the other maintainers and committers are debating a very similar problem with valid points on both sides. Just @Cottser and I are on the same page for most things so it makes it easier to get things done:P

But to be clear one thing we are trying to avoid is passing variables into templates. This problem is very similar to variables passed to the t() function in D7.

So if you pass a variable to SafeMarkup::set($danger) or t($danger) or $variables['html_preview'] = ['#type' => 'inline_template', '#template' => $danger];. You are essentially doing the exact same thing, passing a safe variable as markup and it won't be filtered for Xss attacks.

The t() function docs say it in D7:

You should never use t() to translate variables, such as calling t($text); , unless the text that the variable holds has been passed through t() elsewhere (e.g., $text is one of several translated literal strings in an array). It is especially important never to call...

https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

cdulude’s picture

@joelpittet Ah, that makes sense. Thanks for the explanation!

akalata’s picture

Assigned: cdulude » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Manually tested #4, HTML is identical.

Also want to note that the use of SafeMarkup::checkAdminXss is preferred over an inline template based on joel's explanation in #8.

akalata’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work

Updating my review notes based on YesCT's input:

Patch replaces one instance of SafeMarkup::set with SafeMarkup::checkAdminXss

Do we need a test for checking preview.html specifically (Remaining Task #2), or will that now be covered under the tests for checkAdminXss?

star-szr’s picture

It looks like this is a nice security improvement. Good stuff!

pwolanin’s picture

I'm re-rolling this for the more direct method call

pwolanin’s picture

Status: Needs work » Needs review
FileSize
4.8 KB
4.81 KB

progress, but the new test isn't passing - not sure I'm on the right path (and path needs to be replaced by route name) and need to put e.g. SCRIPT tags into the new html file to make sure they are stripped.

Status: Needs review » Needs work

The last submitted patch, 14: 2501701-14.patch, failed testing.

akalata’s picture

Status: Needs work » Needs review
FileSize
3.86 KB
4.03 KB

I've consolidated pwolanin's new test theme with the existing color_test_theme, and updated the test to confirm that preview.html is rendered, and that offending <script> tags are removed.

akalata’s picture

pwolanin provided some feedback IRL at the NJ shore sprint.

  • Switched the test for the settings form page to use the menu route, rather than passing a string for the URL.
  • Verified that the change in color.module works by making sure the test fails when that change is not included.
star-szr’s picture

Title: Remove or document SafeMarkup::set in template_preprocess_color_scheme_form() » Remove SafeMarkup::set in template_preprocess_color_scheme_form()

The last submitted patch, 17: color_preview-2501701-17_test-fails.patch, failed testing.

pwolanin’s picture

Status: Needs review » Needs work

Needs a tweak to the test. The "test fails" patch should use color.module from HEAD as is is with SafeMarkup::set() and check for the literal <script> tag getting passed through to the user.

In both cases we need an added assertion in the test that a safe tag like H3 or EM get passed through as HTML in HEAD and with the change to Xss::filterAdmin()

akalata’s picture

Status: Needs work » Needs review
FileSize
2.96 KB
4 KB

I've updated the failing example so the new test is against HEAD, and added a new test that makes sure Xss:filterAdmin() does not get double-escaped (though shouldn't that test be will filterAdmin(), rather than our specific implementation of it?).

The last submitted patch, 21: color_preview-2501701-21_fails.patch, failed testing.

pwolanin’s picture

I think this new test method needs a better name

+  function testTwigCacheOverride()

I think that was copied from another test (maybe my fault)

pwolanin’s picture

FileSize
4 KB
409 bytes
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/color/src/Tests/ColorSafePreviewTest.php
@@ -0,0 +1,62 @@
+
+

Minor double line break can likely be fixed on commit.

This looks good to me even has tests which may be overkill but sure. Reviewed the tests, bigUser was a funny name, I like it:)

akalata’s picture

bigUser isn't new - actually coming from existing Color module tests used as an example :)

joelpittet’s picture

Still funny to me:D

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Can we use SafeMarkup::checkAdminXss() since we're trying to remove SafeMarkup side effects from the XSS class - see #2506195: Remove SafeMarkup::set() from Xss::filter()

Sorry for all the confusion about which methods to use - we're all trying to get this sorted asap.

GreenSkunk’s picture

Chris Luckhardt, Tony Nash and I are working on this at the #DrupalNorth sprint.

GreenSkunk’s picture

Issue tags: +#DrupalNorth
GreenSkunk’s picture

Crud ... I need to reapply an earlier patch and re-roll. Learning git

Anonymous’s picture

Issue tags: +DrupalNorth2015

Updating the issue tag to include a hashless DrupalNorth2015 on behalf of the Drupal North sprinting group.

akalata’s picture

Status: Needs work » Needs review

flagging as NR for testbot

Status: Needs review » Needs work

The last submitted patch, 31: color-remove-xss-2501701-29.patch, failed testing.

GreenSkunk’s picture

@Chris and Tony ... cross your fingers!!! Thank you Akalata!
New Patch made after applying #24.
Can I set this to Needs review or does someone else do that?

star-szr’s picture

Status: Needs work » Needs review
Issue tags: -#DrupalNorth

@GreenSkunk thanks, nice meeting you at Drupal North! You should set the status to needs review when uploading a new patch.

dawehner’s picture

Can we use SafeMarkup::checkAdminXss() since we're trying to remove SafeMarkup side effects from the XSS class - see #2506195: Remove SafeMarkup::set() from XSS::filter()

This makes sense here. It is great that we have added test coverage here!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Wanted to actually RTBC here.

lauriii’s picture

Sorry for jumping in

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks everyone! I agree that doing an admin XSS filter is the right fix here.

  1. +++ b/core/modules/color/color.module
    @@ -284,7 +284,7 @@ function template_preprocess_color_scheme_form(&$variables) {
    -  $variables['html_preview'] = SafeMarkup::set(file_get_contents($preview_html_path));
    

    Discussed with @alexpott. This should work equivalently with a #markup render array element actually. I.e.:

    $variables['html_preview']['#markup']
    

    That's also preferable because we are trying to reduce the use of SafeMarkup during the render/theme process in general (to reduce early rendering and escaping, reduce memory use, etc. etc. etc.).

  2. +++ b/core/modules/color/src/Tests/ColorSafePreviewTest.php
    @@ -0,0 +1,62 @@
    +    // Install the test theme
    

    Minor: missing period at the end of the sentence.

  3. +++ b/core/modules/color/src/Tests/ColorSafePreviewTest.php
    @@ -0,0 +1,62 @@
    +    $url_object = Url::fromRoute('system.theme_settings_theme', ['theme' => 'color_test_theme']);
    +
    +    $this->drupalLogin($this->bigUser);
    +    $this->drupalGet($url_object);
    +    $this->assertText('TEST COLOR PREVIEW');
    +
    +    $this->assertNoRaw('<script>alert("security filter test");</script>');
    +    $this->assertRaw('<h2>TEST COLOR PREVIEW</h2>');
    

    Nice complete test coverage!

    We should specifically reference the file where the preview is stored to make the source for these assertions clear. I'd suggest an inline comment. @alexpott suggested writing the file directly to the files directory during the test so that the markup is in the same place in the code. Either way. :)

xjm’s picture

Issue tags: +D8 Accelerate London
lauriii’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
1.66 KB
star-szr’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good and I think all feedback from #40 has been addressed.

  • xjm committed 9165b68 on 8.0.x
    Issue #2501701 by akalata, pwolanin, lauriii, GreenSkunk, cdulude,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Alright, this looks great! Thanks @lauriii and @Cottser.

This issue is a required part of a critical issue and so is acceptable during the beta per https://www.drupal.org/core/beta-changes. Committed and pushed to 8.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.