Bug 163748 – [JSC] crash via `new Function("}{")`

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Caitlin Potter (:caitp)]

Currently, the FunctionConstructor generates a source string in the form:

"{function anonymous() { <source body parameter> } }", which eventually reaches getFunctionExecutableFromGlobalCode().

getFunctionExecutableFromGlobalCode() asserts that the resulting AST contains a Block with a single statement (a function declaration).

However, it is possible to fail this assertion and crash the browser tab. There is no real guarantee that the source code will produce the expected AST, and it shouldn't fire assertions about the structure of the AST.

[Caitlin Potter (:caitp)]

/CC some folks who might be interested in a fix for that (feel free to mark as a sec critical bug if needed --- I don't tend to consider assertion failures to be exploitable, so I felt there was no need).

[Michael Catanzaro]

Looks like denial of service to me.

[Radar WebKit Bug Importer]

<rdar://problem/28879157>

[Darin Adler]

If you are saying that every crash that a webpage can trigger in WebKit is a security bug because they are all "denial of service", then I think that’s wrong.

[Michael Catanzaro]

(In reply to comment #4)
> If you are saying that every crash that a webpage can trigger in WebKit is a
> security bug because they are all "denial of service", then I think that’s
> wrong.

OK, I don't know what your policies are for such issues.

[Darin Adler]

It’s not so much policy but simply a practical consideration. There are *lots* of crashing bugs and I don’t think we treat them all as sensitive security bugs. I believe we try to distinguish exploitable crashes from ones that are simply an inconvenience.

[Brent Fulgham]

(In reply to comment #6)
> It’s not so much policy but simply a practical consideration. There are
> *lots* of crashing bugs and I don’t think we treat them all as sensitive
> security bugs. I believe we try to distinguish exploitable crashes from ones
> that are simply an inconvenience.

Exactly. We try to handle bugs that we judge to allow true "exploits" under our more severe 'Security' classification.

While crashes and "eat CPU spinning in a loop" bugs are super annoying, and are things we want to mitigate where possible, they don't rise to quite the same level for our purposes.

[André Bargull]

Another bug report for this issue (bug 106160) is publicly visible for more than three years, so there's probably no reason to hide this report.

[Michael Catanzaro]

OK, it looks like a duplicate and there's more discussion there.

*** This bug has been marked as a duplicate of bug 106160 ***

[Caitlin Potter (:caitp)]

Reopening to attach new patch.

[Caitlin Potter (:caitp)]

Created attachment 292345 [details]
Patch

[Mark Lam]

Comment on attachment 292345 [details]
Patch

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

Can you also add regression test(s) (in JSTests/stress) for these changes?

> Source/JavaScriptCore/runtime/CodeCache.cpp:189
> +    if (!statement) {
> +        statement = program->firstStatement();
> +        isError = true;
> +    }

It looks like what you're doing here is just stuffing "statement" with a value that will pass the assertions below.  Is that right?  Why not do the equivalent of your isError check here and just return early?  I think it is misleading to stuff "statement" with another statement that doesn't meet the singleStatement requirement.  Am I mistaken?

> Source/JavaScriptCore/runtime/CodeCache.cpp:200
> +    if (!funcDecl) {
> +        funcDecl = static_cast<BlockNode*>(statement)->firstStatement();
> +        isError = true;
> +    }

Ditto.  Why not return early here?

[Caitlin Potter (:caitp)]

Comment on attachment 292345 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CodeCache.cpp:189
>> +    }
> 
> It looks like what you're doing here is just stuffing "statement" with a value that will pass the assertions below.  Is that right?  Why not do the equivalent of your isError check here and just return early?  I think it is misleading to stuff "statement" with another statement that doesn't meet the singleStatement requirement.  Am I mistaken?

I wanted to share the error reporting code, but yes, I've considered this and it is simpler, so I'll do that instead.

[Caitlin Potter (:caitp)]

Created attachment 292360 [details]
Patch

[Mark Lam]

Comment on attachment 292360 [details]
Patch

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

Please fix the outstanding issue with the !statement->isBlock() check.  Thanks.

> Source/JavaScriptCore/runtime/CodeCache.cpp:185
> +    if (!statement) {

nit: You can make this: if (UNLIKELY(!statement)) {

> Source/JavaScriptCore/runtime/CodeCache.cpp:190
>      ASSERT(statement);

You can remove this assert since it can never happen thanks to the new if statement above.

> Source/JavaScriptCore/runtime/CodeCache.cpp:-188
> -    if (!statement || !statement->isBlock())
> -        return nullptr;

Is it true that we don't need the !statement->isBlock() check anymore?  You need to either:
1. reinstate the !statement->isBlock() check, or
2. add the check to your if (!statement) check above, and remove the ASSERT(statement->isBlock());

> Source/JavaScriptCore/runtime/CodeCache.cpp:194
> +    if (!funcDecl) {

nit: use UNLIKELY().

> Source/JavaScriptCore/runtime/CodeCache.cpp:199
>      ASSERT(funcDecl);

Ditto.  Remove because this is not needed anymore.

> Source/JavaScriptCore/runtime/CodeCache.cpp:-194
> -    if (!funcDecl || !funcDecl->isFuncDeclNode())
> -        return nullptr;

Ditto.

[Caitlin Potter (:caitp)]

Created attachment 292362 [details]
Remove redundant ASSERT()s and add branch prediction hints

[Caitlin Potter (:caitp)]

Comment on attachment 292360 [details]
Patch

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

>> Source/JavaScriptCore/runtime/CodeCache.cpp:-188
>> -        return nullptr;
> 
> Is it true that we don't need the !statement->isBlock() check anymore?  You need to either:
> 1. reinstate the !statement->isBlock() check, or
> 2. add the check to your if (!statement) check above, and remove the ASSERT(statement->isBlock());

I believe it's impossible for the first statement in the program to be anything BUT a Block, so long as the source string is formatted the way it is in FunctionConstructor (which appears to be the only caller of this).

Similarly, I believe it's impossible for the funcDecl to be anything BUT a function declaration, for the same reaason. If it's the only statement, it must be a funcDecl, unless the FunctionConstructor is doing something wrong (in which case, the ASSERT is helpful).

[Mark Lam]

Comment on attachment 292362 [details]
Remove redundant ASSERT()s and add branch prediction hints

r=me.  Please add your explanation for why it's safe to remove the !statement->isBlock() and !funcDecl->isFuncDeclNode() to the ChangeLog.  Thanks.

[Caitlin Potter (:caitp)]

Comment on attachment 292362 [details]
Remove redundant ASSERT()s and add branch prediction hints

>Subversion Revision: 207677
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 4c1164f4416dea3ba1c4d3e701f51d4ed15e98b1..e38b1d64a256cec38271eafc92e1da89ec92d434 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,13 @@
>[phone]  Caitlin Potter  <[e-mail]>
>+
>+        [JSC] don't crash when arguments to `new Function()` produce unexpected AST
>+        https://bugs.webkit.org/show_bug.cgi?id=163748
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The ASSERT(statement); and ASSERT(funcDecl); lines are removed, replaced with blocks
>+        to report a generic Parser error message. These lines are only possible to be reached
>+        if the input string produced an unexpected AST, which previously could be used to crash
>+        the process via ASSERT failure.
>+
>+        The node type assertions are left in the tree, as it should be impossible for a top-level
>+        `{` to produce anything other than a Block node. If the node turns out not to be a Block,
>+        it indicates that the (C++) caller of this function (E.g in FunctionConstructor.cpp), is
>+        doing something incorrect. Similarly, it should be impossible for the `funcDecl` node to
>+        be anything other than a function declaration given the conventions of the caller of this
>+        function.
>+
>+        * runtime/CodeCache.cpp:
>+        (JSC::CodeCache::getFunctionExecutableFromGlobalCode):
>+
> 2016-10-20  Keith Miller  <[e-mail]>
> 
>         Add support for WASM calls
>diff --git a/Source/JavaScriptCore/runtime/CodeCache.cpp b/Source/JavaScriptCore/runtime/CodeCache.cpp
>index 73efab1684b026308c160544c0be116496561f75..cc7a7f8dd1e6f1f0fe102a010a0bf80a9cc38910 100644
>--- a/Source/JavaScriptCore/runtime/CodeCache.cpp
>+++ b/Source/JavaScriptCore/runtime/CodeCache.cpp
>@@ -182,16 +182,20 @@ UnlinkedFunctionExecutable* CodeCache::getFunctionExecutableFromGlobalCode(VM& v
> 
>     // This function assumes an input string that would result in a single function declaration.
>     StatementNode* statement = program->singleStatement();
>-    ASSERT(statement);
>-    ASSERT(statement->isBlock());
>-    if (!statement || !statement->isBlock())
>+    if (UNLIKELY(!statement)) {
>+        JSToken token;
>+        error = ParserError(ParserError::SyntaxError, ParserError::SyntaxErrorIrrecoverable, token, "Parser error", -1);
>         return nullptr;
>+    }
>+    ASSERT(statement->isBlock());
> 
>     StatementNode* funcDecl = static_cast<BlockNode*>(statement)->singleStatement();
>-    ASSERT(funcDecl);
>-    ASSERT(funcDecl->isFuncDeclNode());
>-    if (!funcDecl || !funcDecl->isFuncDeclNode())
>+    if (UNLIKELY(!funcDecl)) {
>+        JSToken token;
>+        error = ParserError(ParserError::SyntaxError, ParserError::SyntaxErrorIrrecoverable, token, "Parser error", -1);
>         return nullptr;
>+    }
>+    ASSERT(funcDecl->isFuncDeclNode());
> 
>     FunctionMetadataNode* metadata = static_cast<FuncDeclNode*>(funcDecl)->metadata();
>     ASSERT(metadata);
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index ed1fbb8265cc3cca386c2d6000c116af3535f113..3c21fe8c9b4feca33a5d47e1b54d4a9cb3fcbda2 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,15 @@
>[phone]  Caitlin Potter  <[e-mail]>
>+
>+        [JSC] don't crash when arguments to `new Function()` produce unexpected AST
>+        https://bugs.webkit.org/show_bug.cgi?id=163748
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/regress-163748.js: Added.
>+        (assert):
>+        (shouldThrowSyntaxError):
>+        (GeneratorFunction):
>+
> 2016-10-20  Keith Miller  <[e-mail]>
> 
>         Add support for WASM calls
>diff --git a/JSTests/stress/regress-163748.js b/JSTests/stress/regress-163748.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..807798af81e7ef1d92ba4f99ce6bb48d292d9d06
>--- /dev/null
>+++ b/JSTests/stress/regress-163748.js
>@@ -0,0 +1,24 @@
>+function assert(cond, msg = "") {
>+    if (!cond)
>+        throw new Error(msg);
>+}
>+noInline(assert);
>+
>+function shouldThrowSyntaxError(str) {
>+    var hadError = false;
>+    try {
>+        eval(str);
>+    } catch (e) {
>+        if (e instanceof SyntaxError)
>+            hadError = true;
>+    }
>+    assert(hadError, "Did not throw syntax error");
>+}
>+noInline(shouldThrowSyntaxError);
>+
>+shouldThrowSyntaxError("var f = new Function('}{')");
>+shouldThrowSyntaxError("var f = new Function('}}{{')");
>+
>+var GeneratorFunction = function*(){}.constructor;
>+shouldThrowSyntaxError("var f = new GeneratorFunction('}{')");
>+shouldThrowSyntaxError("var f = new GeneratorFunction('}}{{')");

[Caitlin Potter (:caitp)]

Comment on attachment 292362 [details]
Remove redundant ASSERT()s and add branch prediction hints

Er, "edit attachment as comment" didn't do what I expected it to do, I don't think. It was worth a try, anyways.

[Caitlin Potter (:caitp)]

Created attachment 292366 [details]
fix the changelog

[WebKit Commit Bot]

Comment on attachment 292366 [details]
fix the changelog

Clearing flags on attachment: 292366

Committed r207684: <http://trac.webkit.org/changeset/207684>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.