Change #1017080 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[css-sanitizer@master] feat(css-sanitizer): Improve color parsing var func fallback values

https://gerrit.wikimedia.org/r/1017080

Why do we need fallback values? On project, the custom properties should always be defined by codex, etc, and I'd think we'd actually want to hard fail with an error if an unknown property is referenced, rather than fallback.

Custom properties are a signficant potential security issue, and I think we'd want to keep our support as simple as possible. The more weird cases we introduce, the more likely it is that we open up a hole where someone can bypass the sanitizer by combining properties in unusual ways. Significantly, "The allowed syntax for custom properties is extremely permissive. The <declaration-value> production matches any sequence of one or more tokens..." so I think we want to error on the side of sharply limiting what custom properties we allow where, rather than move in the direction of allowing custom properties in their full flexibility.

The idea of skin-specific custom properties arose, but I think that can be done without adding additional var() syntax like so:

background-color: #fa11bac;
background-color: var(--skin-specific-custom-property);

As I understand it, the second declaration is "guaranteed invalid" if the skin specific custom property isn't defined, so this will use the first property in this case.

EDIT: actually I think I misunderstood the meaning of "guaranteed invalid" in the spec (https://www.w3.org/TR/css-variables-1/#invalid-variables) and the inheritance won't actually work the way I expected it would. Can someone else double-check my logic and figure out whether this can be done in another way without requiring more custom-property syntax?

EDIT: yes, I misunderstood. See below for other concerns.

Change #1017080 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[css-sanitizer@master] feat(css-sanitizer): Improve color parsing var func fallback values

https://gerrit.wikimedia.org/r/1017080

In T361934#9692567, @cscott wrote:

EDIT: actually I think I misunderstood the meaning of "guaranteed invalid" in the spec (https://www.w3.org/TR/css-variables-1/#invalid-variables) and the inheritance won't actually work the way I expected it would. Can someone else double-check my logic and figure out whether this can be done in another way without requiring more custom-property syntax?

Copying the comment: unless you hard-code something to switch it to var( --skin-specific, #fff ), this example would lead to override of #fff to an empty value in all the browsers supporting var() in case --skin-specific is undefined.

I'm already worried about the token-gluing possible with:

border: var(--foo) var(--bar) var(--bat);

which is allowed by the current patch. I guess adding default values doesn't make this *worse* but the semantics of var are already very worrying from a security standpoint. As long as users can't define their own custom properties we're probably okaaay, but we're relying on Codex and Skin authors not to inadvertently define custom properties that can be abused by concatenating them in unusual and unexpected ways.

We could consider locking this down by allowing var *only* in places where repetition (UnorderedGroup::someOf in css-sanitizer) isn't allowed, like allowing in background-color but not background, and in border-*-color but not border-color (which allows 4 vars to be concatenated) or border.

EDIT: UnorderedGroup::someOf actually prohibits repetition. It's only Quantifier which would be problematic, and even there Quantifier::optional is fine and Quantifier::hash requires the values to be comma-separated, which would probably complicate any concatenation-based attack. With all that in mind, border-color seems the only problematic property, which allows up to four var properties to be concatenated:

* { border-color: var(--foo) var(--bar) var(--bat) var(--baz); }

As another mitigation, apparently url() is treated as a single token, and var must evaluate to a series of tokens. So you /probably/ can't abuse this to prepend javascript: to a URL, for example.

But I still think it may be worthwhile to restrict var in border-color to be safe, and I might consider restricting it in the comma-separated cases, box-shadow and text-shadow, although in both of those cases there are also required numeric elements as well complicating the concatenation.

Change #1017299 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[css-sanitizer@master] Harden security by disallowing custom properties for `border-color`

https://gerrit.wikimedia.org/r/1017299

Change #1017299 merged by jenkins-bot:

[css-sanitizer@master] Harden security by disallowing custom properties for `border-color`

https://gerrit.wikimedia.org/r/1017299

Change #1017080 merged by jenkins-bot:

[css-sanitizer@master] feat(css-sanitizer): Improve color parsing var func fallback values

https://gerrit.wikimedia.org/r/1017080

I discussed this with C Scott yesterday and we agreed to push ahead with a release to make sure the solution we've landed on is complete. This may be delayed as @cscott is attending an event this week.

Once that's done I'll follow up with the security team on T361956.

Change #1021564 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/TemplateStyles@master] Update wikimedia/css-sanitizer to 5.3.0

https://gerrit.wikimedia.org/r/1021564

Change #1021567 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/vendor@master] Update CSS sanitizer to 5.3.0

https://gerrit.wikimedia.org/r/1021567

Change #1021564 merged by jenkins-bot:

[mediawiki/extensions/TemplateStyles@master] Allow wikimedia/css-sanitizer at 5.3.0

https://gerrit.wikimedia.org/r/1021564

Change #1021567 merged by jenkins-bot:

[mediawiki/vendor@master] Update CSS sanitizer to 5.3.0

https://gerrit.wikimedia.org/r/1021567

I have confirmed on beta cluster the requirements have been met.

Before I start using this onwiki: is there a risk of this later being revoked following the security review?

Hi @hgzh I checked in with the security team and we feel like it's unlikely we'd completely revoke the functionality, particularly for color properties. The worst that could probably happen is further sanitizing or limiting certain css fallback values and that would need to consider at least temporary backwards compatibility with on-wiki usages. I personally I would feel comfortable recommending using the following:

.foo {
  color: #202122; /* define this in case next line gets stripped (very unlikely in this particular case) , this also supports really old browsers*/
  color: var( --color-base, #202122 ); /* make sure to use the fallback if important for when color-base is not defined by the skin - true for Monobook for example */
}

Hope this helps!