An interesting thing about this, is that it gives way more dynamic programmability to CSS. Since previously the CSS pages were static pages you had to edit to change. With variables, the variable can be set as inline style, so it can be set via lua, or in page previews without save, or even as a result of just a cross-domain GET request.

I don't think that really matters too much, but I thought i would point it out since that is a big change to the surface area and its kind of interesting to think about. There are CSS attacks that depend on things being dynamically set (e.g. using prefix selectors to exfiltrate attributes) but they require a different type of dynamicism, and there are static equivalents of the attack, so its not really that relevant.

My concerns are:

1. We sort of hand-wave about this being safe "because custom properties are only set to color values" but we don't actually enforce that. We enforce something slightly different: that custom properties are *used* only where color values *could be used*. I'm concerned that the gap between those two slightly different definitions could be exploited.
2. If our security *does* critically depend on the *values* given to custom properties, then we need to either sanitize those or at least communicate to reviewers what sorts of Bad Things they should look for when reviewing CSS in extensions/skins/gadgets/etc.
3. The way that custom properties work allows for token concatenation attacks, eg when border: --foo --bar --baz --bat gets expanded, the tokens that correspond to those four properties get concatenated and *potentially* the result could be unexpected. The spec *says* that the values, although unconstrained (The allowed syntax for custom properties is extremely permissive), *are* parsed as tokens ("they're a bare stream of CSS tokens" at least) and so you can't glue u and rl together to get url -- but I think it would be worthwhile to red-team that and check that browsers actually implement these things that way, and that there aren't other ways to concatenate tokens together to get Bad results.

For all of these three things, it would be avoided if we were doing full expansion of custom properties and then sanitizing *the result*, but that's not how this patch works (and it's not really how CSS custom properties are designed to work).

And then, furthering @Bawolff's point, I'm not really familiar with the state of the art of CSS attacks, but they do seem to involve dynamically shifting CSS queries in order to progressively exfiltrate data. I don't know if there are ways to leverage the dynamicism of custom properties in order to achieve those sorts of objectives. https://www.w3.org/TR/css-variables-1/#cycles has an entire discussion of cycles which makes it seem that there might be a thread here to pull on.

In T361956#9736949, @cscott wrote:

For all of these three things, it would be avoided if we were doing full expansion of custom properties and then sanitizing *the result*, but that's not how this patch works (and it's not really how CSS custom properties are designed to work).

Is this a nasty engineering problem to solve? If it's not, it might make sense to add this functionality as a hardening measure, given that failing to fully-expand variable- and dynamic-content has bitten us in other areas before (e.g. T363020)

I don't know if there are ways to leverage the dynamicism of custom properties in order to achieve those sorts of objectives.

AIUI, to exfil anything via css, you really just need to get to clever ways of calling url to send along any captured data.

Anyhow, I do plan to review this a bit more in the coming weeks. Not sure about red-teaming every browser, but hopefully we can at least arrive at some cautious and tested sanitization strategies.

In T361956#9736949, @cscott wrote:

1. We sort of hand-wave about this being safe "because custom properties are only set to color values" but we don't actually enforce that.

No, it's safe because custom properties can only be used where a color keyword is expected, and so the browser will sanitize them as <color> units which are harmless (mainly, can't include external resources).

There are two ways this can fail:

• if there is some browser version which parses color properties differently (because it uses an older spec or is simply buggy) and allows use of URLs, or some other value that has a side effect;
• or, if there is some construct which could both be parsed as a given variable reference representing a color value, and representing a value (or group of values - var() can expand to almost any kind of complex multi-part expression, as long as braces etc. are balanced) which includes an URL or something else that's sensitive.

The first seems unlikely (although also hard to review in a systematic way). For the second, from a quick review of where MatcherFactory::color() is used, this seems to be the case for the <final-bg-layer> construct in StylePropertySanitizer::cssBorderBackground3() which can either be a color or a <bg-image> which eventually resolves to an URL. Everything else seems safe, including border-color that was unsupported in rCSSS34b37a6a1c9a: Harden security by disallowing custom properties for `border-color` (since border does include anything other than lengths and static keywords).

The other thing that looks potentially scary is that <image> can include colors, but only as part of gradients, ie. the variable substitution will be inside a *-gradient(...) CSS function, and var() cannot break out of braces (in theory anyway).

2. If our security *does* critically depend on the *values* given to custom properties, then we need to either sanitize those or at least communicate to reviewers what sorts of Bad Things they should look for when reviewing CSS in extensions/skins/gadgets/etc.

One easy thing to do would be to require that custom properties be prefixed by their css type (so, --color-* instead of --*). That would make setting them to an URL immediately suspicious (and in sanitized-css pages it could be enforced if we ever allow defining custom properties).

In T361956#9742379, @Tgr wrote:

Everything else seems safe

Of course what is and isn't safe can change over time as web standards evolve (e.g. if the border shorthand incorporated border-image that would make the use or variables in border-related properties unsafe), which is very different from every other aspect of css-sanitizer where vulnerabilities can only be introduced by changing the code.

In T361956#9724511, @Bawolff wrote:

With variables, the variable can be set as inline style

We should probably disallow that in wikitext ASAP (currently Sanitizer doesn't seem to filter it). It's a potential attack vector, and not just in TemplateStyles.

In T361956#9742426, @Tgr wrote:

We should probably disallow that in wikitext ASAP (currently Sanitizer doesn't seem to filter it). It's a potential attack vector, and not just in TemplateStyles.

We did disallow var at one point via a Sanitizer rule, for a separate security issue, but that was reverted.

Anyhow, I should have a review completed and posted for the rest of this task sometime this week or next.

In T361956#9742426, @Tgr wrote:

In T361956#9724511, @Bawolff wrote:

With variables, the variable can be set as inline style

We should probably disallow that in wikitext ASAP (currently Sanitizer doesn't seem to filter it). It's a potential attack vector, and not just in TemplateStyles.

To clarify, you mean if some stylesheet had background: url(var(--foo) ); then the attacker could set --foo in an inline style and inject the url, bypassing the sanitizer?

AFAICT, you would need the var() to be inside the url() as you wouldn't be able to pull off this attack if the css rule was just background: var(--foo). That probably limits the surface area quite a bit, but certainly doesn't eliminate it.

Edit: actually, even that doesn't work, as url() is not allowed to contain var(). You would need to use src(var(...)) which is not supported by web browsers yet. So i don't think this attack is possible, at least not yet.

I see on the other task there was a lot of duscussion about token gluing attacks. Reading the css spec that sounds impossible since var substitution works on a token level, and no amount of gluing together ident tokens is going to turn them into a url or function token. (To quote: Note that var() substitution takes place at the level of CSS tokens [css-syntax-3], not at a textual level; you can’t build up a single token where part of it is provided by a variable). At least as far as sneaking a function into a place you dont want goes - i guess there could be other restrictions being bypassed with regards to css sanitizer. The inline style sanitizer seems to be all about banning specific functions. Admittedly i'm not 100% confident in my understanding of the css spec

Security Review Summary - T361956 - 2024-06-06
Last commit reviewed: dbe2db3dad

Summary

After evaluating the conversations on this task and pen-testing various CSS custom properties examples, I'm not currently seeing any obvious means of exploiting them in conjunction with the validation happening via v5.3.0 of the css-sanitizer (limiting to only the calling of custom properties, limiting custom properties to "color" values, disallowing multiple var calls for background values, etc.) and the fact that we actively block url and now src via Sanitizer::checkCss rules (though css-sanitizer notably does not disallow url), the two most likely means of introducing CSS-based attacks. While something like background: var(--foo, url(https://upload.wikimedia.org/wikipedia/commons/a/a9/Example.jpg?20100314172020)) is legitimate CSS, it is currently disallowed by css-sanitizer and I could not find any obvious means of "token-gluing", string and byte manipulations or other means of arriving at the url and src sinks or any other injection-based attack.

This isn't to say that such custom property-related attacks aren't possible or that some clever attacker won't find real vulnerabilities in the future. But for now, there just isn't enough compelling evidence for me to rate the changes from css-sanitizer v5.1.0 to v5.3.0 and the implied, expanded, on-wiki CSS functionality as anything beyond low risk.

In addition to the above analysis, I also ran some of our basic security tooling against the v5.3.0 tag of css-sanitizer. There wasn't much there beyond several outated composer packages (via composer outdated) and a large number of false positives from various semgrep policies and rule sets. I'm happy to share the results of those scans if there's any interest.