Bug 216407 – Safely handle overly-long CSS variable values

• Component: CSS
• Status: RESOLVED FIXED

[Tyler Wilcock]

Safely handle and reject long CSS variable values to prevent unbounded memory consumption.

https://drafts.csswg.org/css-variables/#long-variables

From the spec:

.foo {
  --prop1: lol;
  --prop2: var(--prop1) var(--prop1);
  --prop3: var(--prop2) var(--prop2);
  --prop4: var(--prop3) var(--prop3);
  /* expand to --prop30 */
}

Results in --prop30 containing ~1 billion instances of this identifier.

[Tyler Wilcock]

Created attachment 408548 [details]
Patch

[Tyler Wilcock]

Created attachment 408571 [details]
Patch

[Tyler Wilcock]

Created attachment 408573 [details]
Patch

[Tyler Wilcock]

OK, this patch should be ready for a review.  I added some test machinery to set an explicit timeout for the new test, as if the implementation becomes bugged again in the future this test will consume lots of RAM very fast with a more lenient timeout (17gb in 2min on my machine).

Let me know if there is a better way to do this test vs. the approach I’ve taken here.  Or, if this approach is fine, should this new test machinery work go in a separate bug + patch?

Thanks!

[Darin Adler]

Comment on attachment 408573 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=408573&action=review

Thanks for contributing this. I am not sure it’s OK to have a test that needs a multi-second timeout. We should figure out how to handle this without it becoming a drag on the project.

> Source/WebCore/css/CSSVariableReferenceValue.h:55
> +    static const size_t maxSubstitutionTokens = 65536;

Please use constexpr for this kind of thing in new code.

> Tools/ChangeLog:9
> +        Add ability to parse `timeoutMs` from test headers for tests who need a timeout
> +        value different from the command-wide timeout.

Big picture, I am not so sure it’s good to have tests that take a super-long time to run, so not sure we should be adding this feature. Also, I’d think we’d need this feature in DumpRenderTree too, not just WebKitTestRunner, so we can run this test for WebKitLegacy.

> Tools/WebKitTestRunner/TestController.cpp:1407
> +    double val = WTF::String(value.c_str()).toDouble(&successfulConversion);

While this is OK, there’s no need to convert a std::string to a WTF::String just to use WTF’s parsing. The parseDouble function in <wtf/dtoa.h> works directly on C strings without having to first convert them to a WTF::String. Probably the best one to use is the overload that takes a StringView.

WebKit coding style uses words, so we would not use "val" here. We’d use a single word or two words, maybe "doubleValue" or "parsedValue".

> Tools/WebKitTestRunner/TestOptions.h:109
> +    Seconds timeout { Seconds() };

There’s no need to write { Seconds() } here: unlike scalar types, Seconds objects already get initialized that way without explicitly stating the value.

> LayoutTests/ChangeLog:9
> +        * fast/css/variables/invalidate-overly-long-variable-values.html: Added.
> +        * fast/css/variables/invalidate-overly-long-variable-values-expected.html: Added.

Should this be a WPT test instead of a WebKit-only test?

> LayoutTests/fast/css/variables/invalidate-overly-long-variable-values.html:1
> +<!-- webkit-test-runner [ timeoutMs=5000 ] -->

It’s not good for the project to add a test that takes so long to run; we’re constantly running tests on lots of different computers and a single multi-second test is a huge cost. We’ve already skipped some of the WPT tests since they take so long. Is there some other way we can deal with this?

[Tyler Wilcock]

Created attachment 408618 [details]
Patch

[Tyler Wilcock]

Thanks for looking this over!  I removed all of the `TestOptions`, `TestController`, etc. changes and simply made the test smaller so that it runs instantly, pass or fail.

> Should this be a WPT test instead of a WebKit-only test?

This is a good question.  Quoting the spec (https://drafts.csswg.org/css-variables/#long-variables):

> To avoid this sort of attack, UAs must impose a UA-defined limit on the allowed length of the token stream that a var() function expands into.
> ...
> This specification does not define what size limit should be imposed. However, since there are valid use-cases for custom properties that contain a kilobyte or more of text, it’s recommended that the limit be set relatively high.
> ...
> Note: The general principle that UAs are allowed to violate standards due to resource constraints is still generally true here

The spec intentionally does not set an explicit limit, while making this a WPT would, in practice, be setting a limit.  So I guess I think the answer to your question is no, but from an author's perspective I could see having a consistent limit being useful.

FWIW, the test as I have it in this latest patch would pass in Gecko and Chromium.

[Darin Adler]

(In reply to Tyler Wilcock from comment #7)
> The spec intentionally does not set an explicit limit

Such things are always a problem for interoperability. It seems harmless to not specify, but then the limit of the market leader ends up as the de facto standard, and interoperability has been lost and nothing has been gained by specifying that each user agent may come up with its own limit.

We should try to get the specification revised!

[Tyler Wilcock]

Agreed.  I'll open an issue on the spec and post back here.

[Tyler Wilcock]

Created attachment 408638 [details]
Patch

[Tyler Wilcock]

Created a new patch rebased off the latest mainline to get a fix for a constantly failing test.

Here's the spec issue: https://github.com/w3c/csswg-drafts/issues/5510

[EWS]

Committed r266989: <https://trac.webkit.org/changeset/266989>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408638 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68790776>