With the quick edit module enabled, an untrusted user can create a node with a title of
alert('hi')If an admin user, uses the quickedit module to edit the node, the script will execute.
This was reported via the security team's d8 bug bounty program by https://www.drupal.org/u/JvE
To reproduce
1) Create a node
Make the title field have javascript in it
2) Use quickedit to edit the node
3) Javascript runs
Comment | File | Size | Author |
---|---|---|---|
#52 | 2503963-52.patch | 5.24 KB | Wim Leers |
Comments
Comment #1
mlhess CreditAttribution: mlhess commentedAdding security tag
Comment #2
webchickVery nice find.
This is probably also a D8 upgrade path blocker, given the ease of exploitation.
Comment #3
Wim Leers:(
Reproduced. Fixed. Note that it is impossible to write test coverage for this is because this is pure JS.
This will likely apply to https://www.drupal.org/project/quickedit as well.
Comment #4
Wim Leersnod_ should be given access to this issue as well, and help review it.
Comment #5
pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commentedComment #6
Wim LeersThis was unpublished until today because it also affected the Drupal 7 Quick Edit module, for which a security release was made yesterday: see the SA and release node
The Drupal 7 backport suffered from another security hole.
So, of the two vulnerabilities listed in that SA, only the first applies to Drupal 8:
Comment #7
Wim LeersRe-publishing.
Comment #9
webchickOut of curiosity, should we not be fixing this in Views? I thought the whole point of SafeMarkup was so "consumers" didn't need to worry about needing to know all of these functions like checkMarkup, etc. and at worst you'd get double-escape errors.
Comment #10
xjm@webchick SafeMarkup is the class that communicates to Twig what strings have already been filtered by Drupal. It is Twig's autoescape that actually does the escaping if that safeness is not indicated. As far as I understand, QuickEdit's output is added to the already rendered page after that has been done, i.e. after the theme layer and the rest of rendering. Not sure that Views has anything to do with it.
Comment #11
xjmAlso, this looks correct to me, and has sufficient test coverage. We might want to consider
htmlspecialchars()
instead ofcheckPlain()
but that's still under discussion (see #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping and #2527360: Review all usages of Xss::filter(), Xss::filterAdmin(), and Html::escape() and #2526456: SafeMarkup::checkPlain() and SafeMarkup::escape()'s behaviour is confusing) so that is not worth blocking the critical.Comment #12
xjmI asked @Wim Leers to confirm why Twig's autoescape doesn't come into play here.
Comment #13
Wim LeersAh, yes, I read about #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping the other day but couldn't find it! I wanted to add a @todo so it'd be converted to use that instead.
Note that the sole line that was changed is in
MetadataGenerator::generateEntityMetadata()
, which is called by\Drupal\quickedit\QuickEditController::metadata()
, which outputs it as a JSON response.There is zero relation to Views (mentioned in #9, I don't understand why) nor Twig (mentioned in #12, I also don't understand why).
i.e. this is a JSON response at a route that Quick Edit's JS talks to. There's no security vulnerability in any of the markup. Quick Edit in D8 only adds
data-
attributes and those are safe.Comment #14
xjm@Wim Leers, yep, that's what I thought, thanks! (I.e., the fact that there is zero relation to Twig on the JSON request is exactly why Twig doesn't escape it.) :) So this is good to go.
Comment #15
webchickOk, I get it. #6 is listing the two vulns in D7 but explaining that only the first one applies to D8. Views is safe. Huzzah!
Also thanks for the clarification that the bug is caused because Quick Edit is returning HTML in a JSON response, and is not going through the theme system, thus is bypassing any safe markup filtering Twig would otherwise be doing.
If this isn't in by tomorrow I can commit it then.
Comment #16
effulgentsia CreditAttribution: effulgentsia at Acquia commentedMakes sense. But let's add docs to MetadataGeneratorInterface (both generateEntityMetadata() and generateFieldMetadata()) that 'label' (and 'aria'?) are required to be HTML-safe.
Comment #17
webchickI cannot see how this issue could affect the D8 upgrade path, so removing that tag.
EDIT: LOL I see I added that myself in #2. :) However, that was back when we were conflating D8 upgrade path to mean two different things.
Comment #18
Wim LeersAddressed #16.
Comment #19
dawehnerHere is a patch I wrote back before the patch was unpublished.
I'm not sure doing it in PHP is the right location, because you should escape if you output. The javascript still assumes that the data is correct.
Comment #20
Wim Leers@dawehner: heh, that's exactly the reasoning I used when I rolled #3 :)
I totally agree in principle: when JS is generating markup, it should be responsible for the proper escaping.
But:
Though, yes, JS talking to non-trusted endpoints (i.e. any endpoint that you don't control), it should always do escaping.
Comment #21
Wim LeersBack to NR for the discussion in #19 + #20.
Comment #22
dawehnerWell, Ideally we would know whether a string got marked as safe already in PHP. Being pratical about a security issue is kinda wrong, IMHO.
Ideally I would really want #19 in order to ensure that the JS is right as well and it gives examples to other people how to do things correctly.
It is a way too easy trap, so you have to make sure, that people are aware of the problem.
Comment #23
Wim LeersI'm totally fine with having the escaping happen both in PHP and JS. I agree that setting good examples in core is very important.
What do you think?
Comment #24
dawehnerDo it in both places and document why we do it in both?
Comment #25
Wim LeersSounds good!
Comment #26
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWouldn't doing it in both places result in double-escaping of html entities? I think we need to pick one or the other. I don't have a strong opinion on which place is better for this use-case, but let's be consistent between generateEntityMetadata() and generateFieldMetadata().
Comment #27
dawehnerI think this is a great thing to talk about in the d8 critical call in 8 minutes.
Comment #28
Wim LeersConclusion from the D8 EU criticals call:
Comment #29
Wim LeersInterdiff against #19.
Comment #30
Fabianx CreditAttribution: Fabianx as a volunteer commentedWhy can't we use JS SafeString objects instead of normal strings?
Especially here where we control the data?
Comment #31
Wim LeersWe don't have
SafeString
in JS.Comment #32
dawehnerLooks like my patch.
In a real world you would use a template engine to solve the issue rather than direct escaping, but this is work for other future.
Comment #33
Wim LeersYou mean a templating engine in JS? Yes. That's D9/D8 contrib material :)
(Also, with the three quadrillion JS libraries to do these things, and the five new ones per day on average, it's IMO a good thing that we don't ship with that. Some consolidation is needed first :))
Comment #34
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIf we Drupal.checkPlain() settings.fieldLabel, then we need to remove the SafeMarkup::checkPlain() from generateFieldMetadata().
Also, generateFieldMetadata() does a
'aria' => t('Entity @type @id, field @field', array('@type' => $entity->getEntityTypeId(), '@id' => $entity->id(), '@field' => $label)),
, but I don't see which JS code uses that. If we apply the same logic of the JS code being responsible for escaping, then those@
prefixes need to be converted to!
.Comment #35
Wim LeersUgh, yes, of course. /facepalm.
Comment #36
nlisgo CreditAttribution: nlisgo commentedComment #37
nlisgo CreditAttribution: nlisgo commentedI'll help this one along and address the feedback in #34
Comment #38
nlisgo CreditAttribution: nlisgo commentedI have addressed the feedback in #34.
Comment #39
nlisgo CreditAttribution: nlisgo commentedComment #40
xjmWe were planning to deprecate ! for D8 so I'm not sure about #34. Did we confirm that the placeholders would get double-escaped when the JS escaping was applied using
@
? (I guess this is another place we run into trouble with our translation API coupled to our sanitization API.)I can see the case for doing the escaping in JS since we want to remove early escaping in PHP in general and alternate output should not rely on sanitized output. My thought though was that since QuickEdit is specifically preparing data for its own specific JS, it was appropriate for that data to be safened in PHP.
I guess it's not worth blocking the critical on, but it does raise a lot of questions for me.
Comment #41
nlisgo CreditAttribution: nlisgo commentedWhat's the action here then @xjm? Is patch #39 the one to put forward?
Comment #42
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWe can deprecate
!
, but we can't deprecate the concept of translating strings whose output is for non-HTML usage. And if we're saying that code that returns JSON should return plain-text strings within that and make it the responsibility of the JS to htmlify those strings, then that's an example of non-HTML usage (at least as far as the caller of t() is concerned). In #2509218: Ensure that SafeString objects can be used in non-HTML contexts, I'm proposing to solve that with an'html' => FALSE
option to t(), but until that lands, the!
prefix is all we have.As far as that change to 'aria' is concerned though, we need to find which lines of JS consume that and how they insert that into the DOM, because we need to ensure that those lines deal with it as a plain-text string and therefore escape it themselves, or call a text-based DOM method (such as .attr(), though I don't know if .attr() expects a text or html string, so that would need to be researched/tested). Or, if we no longer have any JS code that consumes the 'aria' value, then maybe we need to remove it from the PHP?
Comment #43
dawehnerWell, the altnerative is to put that string into javascript itself, right? Then you can translate it from there and escape it there.
Comment #44
dawehnerI tried to figure out where this is actually used and of course, this seems to be not that easy. I can't find it, neither in /core/misc nor /core/modules/quickedit
Comment #45
dawehnerIf we don't use it, we should remove it. Adding this in javascript later would be the right thing to do.
Comment #47
xjmWell, it clearly has test coverage, but of course that's just testing the expected response, not the JS that adds it.
Maybe we could git blame the lines and look in the commit that added the aria support? It'd be good to know if we regressed the accessibility or if it was just never used in the first place.
Comment #48
dawehnerThe usage of this was removed as part of #1979784: Factor Create.js and VIE.js out of the Edit module a classical scope creep issue: It removes an external library and does not even think about splitting up the patch
into a reviewable chunk. Reading the comments its almost seems to be that it was entirely pushed through, without any review.
Sadly aria is not mentioned at all on this issue.
Comment #49
catchMentioned this on the EuroCriticals call but making sure it's documented here:
We have the double-escaping problem here because we escape. We wouldn't have it if we XSS filtered since that's re-entrant - although we don't have anything for that in JavaScript yet.
@alexpott has pointed out somewhere that core is already inconsistent between filtering and escaping entity titles (including exactly the same taxonomy term titles between term listings and forums).
Comment #50
Wim LeersWe can't remove
aria
, #1844220: Make in-place editing keyboard and aurally accessible needs it.#48: Sigh. It'd be great if you could look at things in perspective ;) Despite its claims, it had unfortunately turned out that Create.js and VIE.js were poorly abstracted, with loads and loads of assumptions specific to the original CMS and specific use cases it was written for. So, we were faced with the enormous and extremely painful task of factoring them away. We basically had no other choice.
So, not only were we forced to do that painful thing, which is A) never sanely reviewable in the ways we'd otherwise do it, B) due to lack of JS testing. We walked JS maintainer @nod_ through the enormous patch: see #1979784-34: Factor Create.js and VIE.js out of the Edit module.
In other words: given the constraints, I don't think we could've reasonably done it any other way.
Finally, if we had already had issue relationships on d.o, then it'd have linked to #1844220. But since that was not available…
Comment #51
dawehnerDon't feed the troll in me.
Anyway, so can we fix things by exporting the actual data (
$entity_type_id
,$entity_id
and$label
) and generate the string in JS?Comment #52
Wim LeersYes, we can!Upon further inspection, we already know the entity type, entity ID, and field label. So, in fact, it is safe to remove
aria
from the JSON response :)So, let's continue with #45. Here's a reroll that fixes the remaining test failures.
Comment #53
xjmGreat, thanks @Wim Leers and @dawehner. Pity we can't add test coverage without the followup, but I think this is RTBC. I'd be okay with escaping it in the metadata generator too, but this is a more future-proof solution.
Somehow, somewhere, we should make it clear that non-HTML responses are responsible for sanitization still even though HTML is autoescaped. While this might seem like an obvious distinction to some, it's not to everyone. Tagging for a followup for that.
Comment #54
dawehner+1 for the RTBC
I think it is sane that we solve things the right way!
Comment #55
Wim Leers+1 :)
Comment #56
alexpottCommitted f3ed981 and pushed to 8.0.x. Thanks!