Staś Małolepszy :stas Assignee
	Description • 12 years ago

http://en.wikipedia.org/wiki/Billion_laughs and see the attached LOL file (I love how our file format extension fits here).

ctx.get('lolz') hangs the compiler.  Firefox display a dialog asking user to kill the script.

	Staś Małolepszy :stas Assignee
	Comment 1 • 11 years ago

Here's a patch based on http://msdn.microsoft.com/en-us/magazine/ee335713.aspx, "XML Denial of Service Attacks and Defenses".

I propose that we arbitrarily limit two things:

 - how many characters a placeable can be expanded to;  the patch sets this to 2,500, i.e. a one-standard-page length,

 - how many placeables there can be in a complex string;  the patch sets this to 100.

The latter protects from a Quadratic Blowup attack. From the article:

> However, another variation of the Exponential Entity Expansion XML bomb that does work is the Quadratic Blowup attack, discovered by Amit Klein of Trusteer. Instead of defining multiple small, deeply nested entities, the attacker defines one very large entity and refers to it many times:

> <?xml version="1.0"?>
> <!DOCTYPE kaboom [
>   <!ENTITY a "aaaaaaaaaaaaaaaaaa...">
> ]>
> <kaboom>&a;&a;&a;&a;&a;&a;&a;&a;&a;...</kaboom>

	Staś Małolepszy :stas Assignee
	Comment 2 • 11 years ago

Comment on attachment 766657 [details] [diff] [review]
Patch

Review of attachment 766657 [details] [diff] [review]:
-----------------------------------------------------------------

::: Makefile
@@ +13,5 @@
>    tests/lib/compiler/*.js \
>    tests/integration/*.js
> +ifeq ($(INSECURE), 1)
> +LIB_FILES += tests/lib/compiler/insecure/*.js
> +endif

The patch adds two tests but they're disabled by default.  Run make test INSECURE=1 to enable them.

You know, just in case.

	Staś Małolepszy :stas Assignee
	Comment 3 • 11 years ago

Comment on attachment 766657 [details] [diff] [review]
Patch

Review of attachment 766657 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n/parser.js
@@ +415,5 @@
>            complex = true;
>          }
> +        if (body.length > MAX_PLACEABLES) {
> +          throw error('Too many placeables, maximum allowed is ' +
> +                      MAX_PLACEABLES);

By checking this in the parser, we can avoid even creating a ComplexString in the compiler.  Note that this is always done on runtime, because parsing and compilation of ComplexStrings happens lazily.

	Staś Małolepszy :stas Assignee
	Comment 4 • 11 years ago

Comment on attachment 766657 [details] [diff] [review]
Patch

Review of attachment 766657 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/l20n/parser.js
@@ +415,5 @@
>            complex = true;
>          }
> +        if (body.length > MAX_PLACEABLES) {
> +          throw error('Too many placeables, maximum allowed is ' +
> +                      MAX_PLACEABLES);

One more thing that I just realized:  body.length isn't the number of placeables in the sctring, but number of content parts.  So in "a {{ b }} c", body.length is 3.  I'll submit a new patch.

	Staś Małolepszy :stas Assignee
	Comment 6 • 11 years ago

https://github.com/l20n/l20n.js/commit/c2c59a9efea725cebd55705a81b3b5b72c31e876