Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

EmbedLiveSample: Avoid XSS, and add allow= parameter #786

Merged
merged 3 commits into from Sep 6, 2018

Conversation

Projects
None yet
3 participants
@jwhitlock
Copy link
Member

commented Aug 28, 2018

Refactor {{EmbedLiveSample}} to avoid some cross-site scripting (XSS) vectors, including some before-and-after tests. Kuma should bleach these away, but I'd like to start making these templates more XSS-aware, and @ExE-Boss triggered my imagination with PR #773.

Add a seventh optional parameter 馃槷 that sets an allow="<features>" attribute for the <iframe>, so that live samples using restricted features can continue working in Chrome and Safari (see bug 1482159).

This builds on and includes PR #777 and PR #785, which may be easier to review first.

@ExE-Boss
Copy link
Contributor

left a comment

This is a step in the right direction to help prevent stuff like this from ever getting through.

@jwhitlock

This comment has been minimized.

Copy link
Member Author

commented Aug 31, 2018

PR #785 merged, rebased on master.

@escattone escattone self-assigned this Sep 5, 2018

@escattone
Copy link
Member

left a comment

This will need a re-base after #777 is modified and merged, but what's here looks good! The additional tests for XSS attacks are nice additions! Thanks @jwhitlock!

@jwhitlock jwhitlock force-pushed the jwhitlock:embedlivesample-xss-allow branch from 5e3f36b to 1ee9b26 Sep 6, 2018

@escattone
Copy link
Member

left a comment

Nits. Some lines that can be removed now that you have the beforeEachMacro call.

@@ -59,6 +59,18 @@ describeMacro('EmbedLiveSample', function () {
'</iframe>'
);
});
itMacro('One argument: XSS attempt (failed)', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Now that you have this in the beforeEachMacro call above, it can be removed.

This comment has been minimized.

Copy link
@jwhitlock

jwhitlock Sep 6, 2018

Author Member

馃う鈥嶁檪锔 of course, good suggestion.

@@ -71,6 +83,19 @@ describeMacro('EmbedLiveSample', function () {
'</iframe>'
);
});
itMacro('Two arguments: ID, XSS attempt (failed)', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Same as above.

@@ -107,6 +132,19 @@ describeMacro('EmbedLiveSample', function () {
'</iframe>'
);
});
itMacro('Three arguments: ID, width, XSS attempt (failed)', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Same as above.

@@ -138,6 +176,26 @@ describeMacro('EmbedLiveSample', function () {
'</iframe></td></tr></tbody></table>'
);
});
itMacro('Four arguments: ID, width, height, XSS attempt (failed)', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Same as above.

@@ -172,6 +230,18 @@ describeMacro('EmbedLiveSample', function () {
'</iframe>'
);
});
itMacro('Five arguments: ID, "", "", "", XSS Attempt (failed)', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Same as above.

@@ -184,4 +254,45 @@ describeMacro('EmbedLiveSample', function () {
'</iframe>'
);
});
itMacro('Six arguments: ID, width, height, "", "", XSS attempt (failed)', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Same as above.

);
});
itMacro('Seven arguments: ID, width, height, "", "", "", features', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Same as above.

);
});
itMacro('Seven arguments: ID, width, height, "", "", "", XSS Attempt (failed)', function (macro) {
macro.ctx.env.live_samples = {'base_url': 'https://mdn.mozillademos.org'};

This comment has been minimized.

Copy link
@escattone

escattone Sep 6, 2018

Member

Same as above.

@jwhitlock jwhitlock force-pushed the jwhitlock:embedlivesample-xss-allow branch from 1ee9b26 to ae458eb Sep 6, 2018

@jwhitlock

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

Rebased to remove per-test setting of macro.ctx.env.live_samples like it never happened...

@jwhitlock

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

Oops, lost change from XSS attempt (success) to XSS attempt (failed), one more rebase coming

jwhitlock added some commits Aug 28, 2018

EmbedLiveSample: Add allow="feature" parameter
Add a seventh optional argument that includes an allow= parameter to
allow restricted features to be used in an iframe. See bug 1482159.

@jwhitlock jwhitlock force-pushed the jwhitlock:embedlivesample-xss-allow branch from ae458eb to cc1e5c3 Sep 6, 2018

@escattone
Copy link
Member

left a comment

馃憤 Thanks @jwhitlock!

@escattone escattone merged commit e2494b3 into mdn:master Sep 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jwhitlock jwhitlock deleted the jwhitlock:embedlivesample-xss-allow branch Sep 6, 2018

@jwhitlock

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

This is a step in the right direction to help prevent stuff like this from ever getting through.

@ExE-Boss, I should have replied to you 9 days ago, and maybe this should be documented somewhere other than a PR comment, but this makes for a good first draft.

MDN pages are user-submitted content, and so we don't trust them. We use a HTML5-aware library bleach that supports an allow-list for tags, attributes, and styles (see kuma/wiki/constants.py). MDN wiki content goes through bleaching and other content manipulation before the "rendered" version is displayed to users (see kuma/wiki/content.py).

This keeps users safe, but causes some other issues. The "bleached" content is not fed back to the "raw" content, so users can get confused why the source looks different than the output. Some processes do not preserve attribute order, resulting in large diffs for small changes around tool updates. It could be better, but it is also battle tested over years, so we avoid radical changes.

KumaScript macro output is treated as unsafe content, and goes through the same bleaching process:

https://github.com/mozilla/kuma/blob/6e7fc6489836b9824fa40c65bf61c50ad4c67b87/kuma/wiki/kumascript.py#L213-L217

You'd have to find a weakness in the whole bleaching system in order to get an attack through to visitors. Mozilla/Add-ons/WebExtensions$revision/1396309 was a weird attempt to add a browser-based coin miner to MDN, embedded as a table. Even if the author had formatted it correctly, the <script> elements would have been escaped or removed, neutering it.

See the developer.mozilla.org:Security topic in Bugzilla for the long, painful history of successful XSS attacks and their mitigations. And, if you find something, submit it with the "Security issue" flag, and you might show up on the Hall of Fame. However, MDN is not one of the sites for bounty payouts, so do it for the love, not the money.

We're thinking of how to evolve KumaScript for the future, and some options are a web-accessible KumaScript API or even client-side KumaScript, to fix preview, or to allow alternate contribution paths. Part of that will be making macros safer, so that the output can be trusted to run on user's browsers without Kuma-based bleaching. We have a long way to go, and haven't decided if we should start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.