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.

Details

Related Gerrit Patches:

Event Timeline

Anomie created this task.Jun 13 2017, 3:50 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 13 2017, 3:50 PM
  • 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.

Reedy added a subscriber: Reedy.Jun 13 2017, 5:00 PM

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 triaged this task as High priority.Jun 13 2017, 8:37 PM
dpatrick added a subscriber: dpatrick.

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

Anomie closed this task as Resolved.Jun 15 2017, 6:46 PM
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
ggellerman moved this task from Up next to Done on the TemplateStyles board.Nov 21 2017, 7:15 PM