Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
- Evaluate whether the string can be refactored to one of the formats outlined in this change record: https://www.drupal.org/node/2311123 (DONE)
- 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.
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:
- Clean install of Drupal 8.
- Enable the Color module and Bartik theme (not required if using Standard install profile as these are already enabled)
- Visit Bartik's setting page at admin/appearance/settings/bartik
- 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
Comment | File | Size | Author |
---|---|---|---|
#42 | interdiff.txt | 1.66 KB | lauriii |
#42 | remove_safemarkup_set-2501701-42.patch | 3.83 KB | lauriii |
#39 | interdiff.txt | 1.22 KB | lauriii |
#39 | remove_safemarkup_set-2501701-39.patch | 3.71 KB | lauriii |
#35 | color-replaceXSS-2501701-35.patch | 3.72 KB | GreenSkunk |
Comments
Comment #1
cdulude CreditAttribution: cdulude commentedConverted
$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.
Comment #2
cdulude CreditAttribution: cdulude commentedComment #3
star-szrI 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.
Minor (coding standards): Either remove the space between
array
and(
, or convert to short array syntax (square brackets). https://www.drupal.org/coding-standards#arrayComment #4
joelpittetTry this on for size.
Comment #5
cdulude CreditAttribution: cdulude commentedThanks for the heads-up on the errant space in
array()
@Cottser!@joelpittet : Thanks for the alternate solution! Just curious, why is
SafeMarkup::checkAdminXss()
okay, whereasSafeMarkup::set()
is not, as far as being for internal use? (or not?)Comment #6
joelpittet@cdulude good question.
SafeMarkup::checkAdminXss()
will runXss::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?
Comment #7
cdulude CreditAttribution: cdulude commented@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!)
Comment #8
joelpittet#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)
ort($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:
https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7
Comment #9
cdulude CreditAttribution: cdulude commented@joelpittet Ah, that makes sense. Thanks for the explanation!
Comment #10
akalata CreditAttribution: akalata as a volunteer commentedManually 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.
Comment #11
akalata CreditAttribution: akalata as a volunteer commentedUpdating 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?
Comment #12
star-szrIt looks like this is a nice security improvement. Good stuff!
Comment #13
pwolanin CreditAttribution: pwolanin at Acquia commentedI'm re-rolling this for the more direct method call
Comment #14
pwolanin CreditAttribution: pwolanin at Acquia commentedprogress, 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.
Comment #16
akalata CreditAttribution: akalata as a volunteer commentedI'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.Comment #17
akalata CreditAttribution: akalata as a volunteer commentedpwolanin provided some feedback IRL at the NJ shore sprint.
Comment #18
star-szrComment #20
pwolanin CreditAttribution: pwolanin at Acquia commentedNeeds 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()
Comment #21
akalata CreditAttribution: akalata as a volunteer commentedI'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?).
Comment #23
pwolanin CreditAttribution: pwolanin at Acquia commentedI think this new test method needs a better name
I think that was copied from another test (maybe my fault)
Comment #24
pwolanin CreditAttribution: pwolanin at Acquia commentedComment #25
joelpittetMinor 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:)
Comment #26
akalata CreditAttribution: akalata as a volunteer commentedbigUser isn't new - actually coming from existing Color module tests used as an example :)
Comment #27
joelpittetStill funny to me:D
Comment #28
alexpottCan 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.
Comment #29
GreenSkunkChris Luckhardt, Tony Nash and I are working on this at the #DrupalNorth sprint.
Comment #30
GreenSkunkComment #31
GreenSkunkCrud ... I need to reapply an earlier patch and re-roll. Learning git
Comment #32
Anonymous (not verified) CreditAttribution: Anonymous as a volunteer commentedUpdating the issue tag to include a hashless DrupalNorth2015 on behalf of the Drupal North sprinting group.
Comment #33
akalata CreditAttribution: akalata as a volunteer commentedflagging as NR for testbot
Comment #35
GreenSkunk@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?
Comment #36
star-szr@GreenSkunk thanks, nice meeting you at Drupal North! You should set the status to needs review when uploading a new patch.
Comment #37
dawehnerThis makes sense here. It is great that we have added test coverage here!
Comment #38
dawehnerWanted to actually RTBC here.
Comment #39
lauriiiSorry for jumping in
Comment #40
xjmThanks everyone! I agree that doing an admin XSS filter is the right fix here.
Discussed with @alexpott. This should work equivalently with a
#markup
render array element actually. I.e.: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.).
Minor: missing period at the end of the sentence.
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. :)
Comment #41
xjmComment #42
lauriiiComment #43
star-szrChanges look good and I think all feedback from #40 has been addressed.
Comment #45
xjmAlright, 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!