Bug 163748

Summary: [JSC] crash via `new Function("}{")`
Product: WebKit Reporter: Caitlin Potter (:caitp) <caitp>
Component: JavaScriptCoreAssignee: Caitlin Potter (:caitp) <caitp>
Status: RESOLVED FIXED    
Severity: Normal CC: andre.bargull, bfulgham, commit-queue, darin, keith_miller, mark.lam, mcatanzaro, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Remove redundant ASSERT()s and add branch prediction hints
none
fix the changelog none

Description Caitlin Potter (:caitp) 2016-10-20 12:24:56 PDT
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.
Comment 1 Caitlin Potter (:caitp) 2016-10-20 12:37:34 PDT
/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).
Comment 2 Michael Catanzaro 2016-10-20 15:25:37 PDT
Looks like denial of service to me.
Comment 3 Radar WebKit Bug Importer 2016-10-20 15:26:00 PDT
<rdar://problem/28879157>
Comment 4 Darin Adler 2016-10-20 17:47:35 PDT
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.
Comment 5 Michael Catanzaro 2016-10-20 18:26:02 PDT
(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.
Comment 6 Darin Adler 2016-10-20 18:31:10 PDT
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.
Comment 7 Brent Fulgham 2016-10-20 22:32:12 PDT
(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.
Comment 8 André Bargull 2016-10-21 04:56:27 PDT
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.
Comment 9 Michael Catanzaro 2016-10-21 05:13:47 PDT
OK, it looks like a duplicate and there's more discussion there.

*** This bug has been marked as a duplicate of bug 106160 ***
Comment 10 Caitlin Potter (:caitp) 2016-10-21 06:42:10 PDT
Reopening to attach new patch.
Comment 11 Caitlin Potter (:caitp) 2016-10-21 06:42:13 PDT
Created attachment 292345 [details]
Patch
Comment 12 Mark Lam 2016-10-21 09:39:26 PDT
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?
Comment 13 Caitlin Potter (:caitp) 2016-10-21 09:42:15 PDT
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.
Comment 14 Caitlin Potter (:caitp) 2016-10-21 09:58:59 PDT
Created attachment 292360 [details]
Patch
Comment 15 Mark Lam 2016-10-21 10:04:04 PDT
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.
Comment 16 Caitlin Potter (:caitp) 2016-10-21 10:07:34 PDT
Created attachment 292362 [details]
Remove redundant ASSERT()s and add branch prediction hints
Comment 17 Caitlin Potter (:caitp) 2016-10-21 10:10:54 PDT
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).
Comment 18 Mark Lam 2016-10-21 10:13:08 PDT
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.
Comment 19 Caitlin Potter (:caitp) 2016-10-21 10:20:00 PDT
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 @@
>+2016-10-21  Caitlin Potter  <caitp@igalia.com>
>+
>+        [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  <keith_miller@apple.com>
> 
>         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 @@
>+2016-10-21  Caitlin Potter  <caitp@igalia.com>
>+
>+        [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  <keith_miller@apple.com>
> 
>         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('}}{{')");
Comment 20 Caitlin Potter (:caitp) 2016-10-21 10:25:15 PDT
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.
Comment 21 Caitlin Potter (:caitp) 2016-10-21 10:27:17 PDT
Created attachment 292366 [details]
fix the changelog
Comment 22 WebKit Commit Bot 2016-10-21 10:52:15 PDT
Comment on attachment 292366 [details]
fix the changelog

Clearing flags on attachment: 292366

Committed r207684: <http://trac.webkit.org/changeset/207684>
Comment 23 WebKit Commit Bot 2016-10-21 10:52:20 PDT
All reviewed patches have been landed.  Closing bug.