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.