768661 - Kuma: Templates should be exempt from CSS property whitelist

Status: RESOLVED WONTFIX

(developer.mozilla.org Graveyard :: Wiki pages, defect, P1)

Description

[Eric Shepherd [:sheppy]]

Since only specially authorized people can edit templates, they should be exempt from the CSS property whitelist. This is necessary because we use templates to do a lot of nifty UX stuff on the site in templates, including this template:

https://developer-new.mozilla.org/en-US/docs/Template:VersionTimeline

Which is used to output the fancy diagram box you see at the top of:

https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIMemoryReporter

This is a must fix ASAP bug.

Comment 1

[Les Orchard [:lorchard]]

The rule for templates so far has been that their output can't do anything above what a user could do by editing a document directly. That's because templates accept parameters from all users, and those parameters are included in output. That can cause security issues and open up exploits.

So, I don't think we can make templates exempt from the CSS whitelist. We can add to the whitelist, though, which can cover a lot of things. And, for anything that's seems too dicey to add to that whitelist, we can support through admin-editable CSS in bug 768492

Comment 2

[John Karahalis [:openjck]]

We had a discussion about this on IRC today. I will talk to the docs team about CSS editing in general and share the results here.

Comment 3

[Eric Shepherd [:sheppy]]

I will point out that since template editing is restricted to the select few, this problem can be mitigated in that we can train our template editors to be careful with their argument handling.

Perhaps we can come up with guidelines for doing so?

Comment 4

[Les Orchard [:lorchard]]

Guidelines mean that we'd need to have a review system before allowing any particular template change to be accepted. Webdev & security team start being hardasses about this stuff when any given exploit costs big $$$ in the bug bounty system.

Comment 5

[Eric Shepherd [:sheppy]]

Could we instead do things to limit the odds of damage thusly:

* Only permit images, scripts, media, and so forth to come from *.mozilla.com/.org domains.
* Run the output of the template through a less stringent Bleach that allows all CSS but is still cautious about XSS issues in terms of the HTML.

I admit I don't really know all the potential security concerns here, but it seems like we could eliminate or mitigate a lot of them with a few automated checks like this.

We only want scripts and media to be hosted on mozilla domains anyway, so having this restriction is a nice idea and will easily help mitigate a lot of potential attacks, especially since users have to have privileges to upload files, as well as to edit templates. Between these two things, that would boost security in this area a lot.

Comment 6

[Les Orchard [:lorchard]]

(In reply to Eric Shepherd [:sheppy] from comment #5)
> Could we instead do things to limit the odds of damage thusly:
> 
> * Only permit images, scripts, media, and so forth to come from
> *.mozilla.com/.org domains.

Bleach doesn't currently support this. Would take a bit of research time to see how we could filter media URLs based on a pattern like that. Probably doable with some work, though.

I'd suggest using CSP, but 1) we have way too much inline JS, and 2) that's a client-side technology.

> * Run the output of the template through a less stringent Bleach that allows
> all CSS but is still cautious about XSS issues in terms of the HTML.

I think :groovecoder was talking about us forking Bleach to switch from a whitelist to a blacklist. Currently, there's no way to allow all CSS.

One thing I'd suggested over in the security review bug is that maybe we can have a per-page list of additions to the whitelist? That is, add the CSS properties especially needed for a particular page. 

Trying to think if there are things we can do to enable content but not open it up to all pages everywhere on the wiki

> I admit I don't really know all the potential security concerns here, but it
> seems like we could eliminate or mitigate a lot of them with a few automated
> checks like this.

That's the problem, and why we use a whitelist: No one knows what all the potential security concerns are, and new ones appear daily. So, we've tried to constrain to the things we're reasonably sure are innocuous.

Comment 7

[Eric Shepherd [:sheppy]]

OK, for the stuff for which this is primarily a problem, what we can do is revise the templates to use CSS classes, and I can put those in the CustomCSS page; I presume that page is exempted from the whitelist? Please? :)

Comment 8

[Les Orchard [:lorchard]]

(In reply to Eric Shepherd [:sheppy] from comment #7)
> OK, for the stuff for which this is primarily a problem, what we can do is
> revise the templates to use CSS classes, and I can put those in the
> CustomCSS page; I presume that page is exempted from the whitelist? Please?
> :)

Oh, yeah, that CustomCSS page isn't processed by kumascript, is served up as text/css, and can only be changed by template editors. I think that skips past most, if not all, of the issues here.

Comment 9

[Eric Shepherd [:sheppy]]

OK, this essentially eliminates this entire problem; we'll simply define CSS classes for all the stuff templates do, CSS-wise. In fact, this will actually make the site better, since right now we duplicate CSS in all these templates, resulting in having to edit them all if we want to change how a given banner category looks.

I'm going to close this bug!