Page MenuHomePhabricator

TemplateStyles HTML injection
Closed, ResolvedPublic

Description

It's possible to include a </style> in a stylesheet considered valid by css-sanitizer and TemplateStyles. Anything after that in the stylesheet will be interpreted as raw HTML.

Specifically, I found two ways to do it: content: "</style>" (or anything else that allows a string) and grid-area: \< / style 1 / \>; (which outputs as grid-area:\</style 1/\>, close enough for browsers to accept).

I'll upload patches momentarily to fix this in two ways:

  • In css-sanitizer, use numeric escapes for < and > in strings and identifiers.
  • In TempateStyles, reject anything containing the string </style.

Event Timeline

  • css-sanitizer:
  • TemplateStyles:

As far as deployment, TemplateStyles isn't currently live anywhere in production, but Beta Labs may be vulnerable. We'd probably want to get the second patch out there first, then follow up reasonably soon with the css-sanitizer patch plus patches to increment the version in TemplateStyles's composer.json and the mediawiki/vendor.

I guess the TemplateStyles patch can just go pretty much straight into public and be merged (when it's been CR'd)

The core patch will need to follow a more usual security deploy regime... But that's not going to get it onto beta any time soon

Neither one is a core patch. css-sanitizer is included in mediawiki/vendor, but as far as I know nothing uses it yet besides TemplateStyles in Beta Labs.

I thought this would have been dealt with by T133147

Since TemplateStyles has to wrap the CSS in a strip marker to protect it from doBlockLevels, it manages to bypass that fix too.

  • css-sanitizer:
  • TemplateStyles:

+1 to both these patches

dpatrick subscribed.

Since this hasn't been deployed, this can committed now, correct?

Anomie claimed this task.

Patches are in Gerrit and merged. Beta Labs is updated.

Anomie changed the visibility from "Custom Policy" to "Public (No Login Required)".Jun 15 2017, 6:47 PM