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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mlhess’s picture

Issue tags: +Security

Adding security tag

webchick’s picture

Issue tags: +D8 upgrade path

Very nice find.

This is probably also a D8 upgrade path blocker, given the ease of exploitation.

Wim Leers’s picture

Status: Active » Needs review
Issue tags: +JavaScript
FileSize
1.19 KB

:(

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.

Wim Leers’s picture

Title: XSS in quick-edit » XSS in Quick Edit

nod_ should be given access to this issue as well, and help review it.

pwolanin’s picture

Issue tags: +D8 Security Bounty
Wim Leers’s picture

Title: XSS in Quick Edit » XSS in Quick Edit: entity title is not safely encoded
Version: 8.0.0-beta11 » 8.0.x-dev
FileSize
1.56 KB
2.3 KB

This 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:

  • The module doesn't sufficiently filter entity titles under the scenario where the user starts in-place editing an entity.
  • The module also doesn't sufficiently filter node titles under the scenario where a node is displayed (albeit only on pages that are not the node page, so e.g. Views listings).
Wim Leers’s picture

Re-publishing.

The last submitted patch, 6: quickedit_xss-2503963-6-test-only-FAIL.patch, failed testing.

webchick’s picture

Out 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.

xjm’s picture

@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.

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Also, this looks correct to me, and has sufficient test coverage. We might want to consider htmlspecialchars() instead of checkPlain() 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.

xjm’s picture

I asked @Wim Leers to confirm why Twig's autoescape doesn't come into play here.

Wim Leers’s picture

Ah, 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.

xjm’s picture

@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.

webchick’s picture

Ok, 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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

the bug is caused because Quick Edit is returning HTML in a JSON response [without] going through the theme system

Makes sense. But let's add docs to MetadataGeneratorInterface (both generateEntityMetadata() and generateFieldMetadata()) that 'label' (and 'aria'?) are required to be HTML-safe.

webchick’s picture

Issue tags: -D8 upgrade path

I 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.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.78 KB
1.53 KB

Addressed #16.

dawehner’s picture

Here 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.

Wim Leers’s picture

@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:

  • In cases where the endpoints generating non-HTML responses are tightly coupled to Drupal JS, I think it's acceptable to say "the JS should be able to assume the data it is given is safe". That could be a valid design choice.
  • We don't have automated JS testing. We do have automated PHP testing. So we need to be practical here.
  • Precisely because the JS is only talking to a trusted endpoint (i.e. that you control), I think #18 is sufficient.

Though, yes, JS talking to non-trusted endpoints (i.e. any endpoint that you don't control), it should always do escaping.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review

Back to NR for the discussion in #19 + #20.

dawehner’s picture

We don't have automated JS testing. We do have automated PHP testing. So we need to be practical here.

Well, 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.

Wim Leers’s picture

I'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?

dawehner’s picture

Do it in both places and document why we do it in both?

Wim Leers’s picture

Sounds good!

effulgentsia’s picture

Wouldn'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().

dawehner’s picture

I think this is a great thing to talk about in the d8 critical call in 8 minutes.

Wim Leers’s picture

Conclusion from the D8 EU criticals call:

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
1.15 KB

Interdiff against #19.

Fabianx’s picture

Why can't we use JS SafeString objects instead of normal strings?

Especially here where we control the data?

Wim Leers’s picture

Why can't we use JS SafeString objects instead of normal strings?

We don't have SafeString in JS.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Looks 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.

Wim Leers’s picture

You 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 :))

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/quickedit/js/theme.js
@@ -59,7 +59,8 @@
-    return '<span class="field">' + settings.fieldLabel + '</span>' + settings.entityLabel;
+    return '<span class="field">' + Drupal.checkPlain(settings.fieldLabel) + '</span>' + Drupal.checkPlain(settings.entityLabel);

If 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 !.

Wim Leers’s picture

Ugh, yes, of course. /facepalm.

nlisgo’s picture

Assigned: Unassigned » nlisgo
nlisgo’s picture

I'll help this one along and address the feedback in #34

nlisgo’s picture

Status: Needs work » Needs review
FileSize
1017 bytes
2.13 KB

I have addressed the feedback in #34.

nlisgo’s picture

Assigned: nlisgo » Unassigned
xjm’s picture

+++ b/core/modules/quickedit/src/MetadataGenerator.php
@@ -89,10 +89,10 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
-      'aria' => t('Entity @type @id, field @field', array('@type' => $entity->getEntityTypeId(), '@id' => $entity->id(), '@field' => $label)),
+      'aria' => t('Entity !type !id, field !field', array('!type' => $entity->getEntityTypeId(), '!id' => $entity->id(), '!field' => $label)),

We 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.

nlisgo’s picture

What's the action here then @xjm? Is patch #39 the one to put forward?

effulgentsia’s picture

We were planning to deprecate ! for D8

We 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?

dawehner’s picture

+++ b/core/modules/quickedit/src/MetadataGenerator.php
@@ -89,10 +89,10 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
-      'aria' => t('Entity @type @id, field @field', array('@type' => $entity->getEntityTypeId(), '@id' => $entity->id(), '@field' => $label)),
+      'aria' => t('Entity !type !id, field !field', array('!type' => $entity->getEntityTypeId(), '!id' => $entity->id(), '!field' => $label)),

Well, the altnerative is to put that string into javascript itself, right? Then you can translate it from there and escape it there.

dawehner’s picture

+++ b/core/modules/quickedit/src/MetadataGenerator.php
@@ -89,10 +89,10 @@ public function generateFieldMetadata(FieldItemListInterface $items, $view_mode)
-      'aria' => t('Entity @type @id, field @field', array('@type' => $entity->getEntityTypeId(), '@id' => $entity->id(), '@field' => $label)),
+      'aria' => t('Entity !type !id, field !field', array('!type' => $entity->getEntityTypeId(), '!id' => $entity->id(), '!field' => $label)),

I 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

dawehner’s picture

If we don't use it, we should remove it. Adding this in javascript later would be the right thing to do.

Status: Needs review » Needs work

The last submitted patch, 45: 2503963-45.patch, failed testing.

xjm’s picture

Well, 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.

dawehner’s picture

The 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.

catch’s picture

Mentioned 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).

Wim Leers’s picture

We 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…

dawehner’s picture

Don'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?

Wim Leers’s picture

Yes, we can!

'aria' => t('Entity @type @id, field @field', array('@type' => $entity->getEntityTypeId(), '@id' => $entity->id(), '@field' => $label)),

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.

xjm’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs followup

Great, 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.

dawehner’s picture

+1 for the RTBC
I think it is sane that we solve things the right way!

Wim Leers’s picture

I think it is sane that we solve things the right way!

+1 :)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f3ed981 and pushed to 8.0.x. Thanks!

  • alexpott committed f3ed981 on 8.0.x
    Issue #2503963 by Wim Leers, dawehner, nlisgo, mlhess, xjm, effulgentsia...

Status: Fixed » Closed (fixed)

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