Closed Bug 925070 Opened 6 years ago Closed 6 years ago

Crash with Worker constructor in JSVAL_TO_STRING

Categories

(Core :: DOM: Workers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox25 --- unaffected
firefox26 --- unaffected
firefox27 + fixed
firefox-esr17 --- unaffected
firefox-esr24 --- unaffected
b2g18 --- unaffected

People

(Reporter: nils, Assigned: bent.mozilla)

References

Details

(Keywords: regression, sec-high, Whiteboard: [first filed by Atte Kettunen as bug 923649])

Attachments

(1 file)

HTML:
<script>
var w=new Worker('test.js');
w.constructor(0xbabababababa);
</script>

and an empty test.js will lead to the following crash (the crash address will change with the argument to the constructor).

=================================================================
==9522==ERROR: AddressSanitizer: SEGV on unknown address 0x575757575740 (pc 0x7f6cbb9d68f7 sp 0x7fff55ec70d0 bp 0x7fff55ec7300 T0)
AddressSanitizer can not provide additional info.
    #0 0x7f6cbb9d68f6 in isRope /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:304:0
    #1 0x7f6cbb9d68f6 in isLinear /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:315:0
    #2 0x7f6cbb9d68f6 in isFlat /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:337:0
    #3 0x7f6cbb9d68f6 in ensureFlat /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/String.h:1089:0
    #4 0x7f6cbb9d68f6 in JS_GetStringCharsZAndLength(JSContext*, JSString*, unsigned long*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/jsapi.cpp:5381:0
    #5 0x7f6cb76d55d2 in JSVAL_TO_STRING /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/../base/nsJSUtils.h:129:0
    #6 0x7f6cb76d55d2 in init /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/../base/nsJSUtils.h:141:0
    #7 0x7f6cb76d55d2 in (anonymous namespace)::Worker::ConstructInternal(JSContext*, JS::CallArgs, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/Worker.cpp:110:0
    #8 0x7f6cb76d5cbf in (anonymous namespace)::Worker::Construct(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/workers/Worker.cpp:262:0
    #9 0x7f6cbb7c2b8a in native /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/jscntxtinlines.h:220:0
    #10 0x7f6cbb7c2b8a in js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:463:0
    #11 0x7f6cbb7b3626 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:2465:0
    #12 0x7f6cbb7a4a21 in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:420:0
    #13 0x7f6cbb7c4e0b in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:610:0
    #14 0x7f6cbb7c5166 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/vm/Interpreter.cpp:646:0
    #15 0x7f6cbb9d15fd in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/js/src/jsapi.cpp:4918:0
    #16 0x7f6cb748b86f in nsJSUtils::EvaluateString(JSContext*, nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions&, JS::Value*, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/base/nsJSUtils.cpp:280:0
    #17 0x7f6cb74795bf in nsJSContext::EvaluateString(nsAString_internal const&, JS::Handle<JSObject*>, JS::CompileOptions&, bool, JS::Value*, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/dom/base/nsJSEnvironment.cpp:995:0
    #18 0x7f6cb6c1860a in nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, nsString const&, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptLoader.cpp:1002:0
    #19 0x7f6cb6c15f1c in nsScriptLoader::ProcessRequest(nsScriptLoadRequest*, void**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptLoader.cpp:869:0
    #20 0x7f6cb6c152e2 in nsScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptLoader.cpp:696:0
    #21 0x7f6cb6c0d319 in nsScriptElement::MaybeProcessScript() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/content/base/src/nsScriptElement.cpp:140:0
    #22 0x7f6cb79b8040 in operator-> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/obj-firefox/parser/html/../../dist/include/nsIScriptElement.h:220:0
    #23 0x7f6cb79b8040 in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/parser/html/nsHtml5TreeOpExecutor.cpp:789:0
    #24 0x7f6cb79b5bdb in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/parser/html/nsHtml5TreeOpExecutor.cpp:593:0
    #25 0x7f6cb792fbf8 in nsHtml5ExecutorFlusher::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/parser/html/nsHtml5StreamParser.cpp:131:0
    #26 0x7f6cb9e2a069 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/xpcom/threads/nsThread.cpp:622:0
    #27 0x7f6cb9d52241 in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/xpcom/glue/nsThreadUtils.cpp:238:0
    #28 0x7f6cb8a20f21 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/glue/MessagePump.cpp:85:0
    #29 0x7f6cb9f47fc3 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/chromium/src/base/message_loop.cc:220:0
    #30 0x7f6cb9f47fc3 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/chromium/src/base/message_loop.cc:213:0
    #31 0x7f6cb9f47fc3 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/ipc/chromium/src/base/message_loop.cc:187:0
    #32 0x7f6cb87f482c in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/widget/xpwidgets/nsBaseAppShell.cpp:161:0
    #33 0x7f6cb81e01ae in nsAppStartup::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/components/startup/nsAppStartup.cpp:269:0
    #34 0x7f6cb567c2b0 in XREMain::XRE_mainRun() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/xre/nsAppRunner.cpp:3868:0
    #35 0x7f6cb567d1ea in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/xre/nsAppRunner.cpp:3936:0
    #36 0x7f6cb567e11b in XRE_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/toolkit/xre/nsAppRunner.cpp:4138:0
    #37 0x459b8d in do_main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/browser/app/nsBrowserApp.cpp:275:0
    #38 0x459b8d in main /builds/slave/m-cen-l64-asan-ntly-0000000000/build/browser/app/nsBrowserApp.cpp:635:0
    #39 0x7f6cc4d16ea4 in __libc_start_main /build/buildd/eglibc-2.17/csu/libc-start.c:260
    #40 0x45910c in _start ??:?
==9522==ABORTING
Looks like a regression from bug 643325.  The new code is:

    nsDependentJSString scriptURL;
    if (!scriptURL.init(aCx, aArgs[0])) {

but nsDependentJSString::init is:

139   bool init(JSContext* aContext, const JS::Value &v)
140   {
141       return init(aContext, JSVAL_TO_STRING(v));
142   }

which will, in this case, assert JSVAL_IS_STRING (which will fail for this case!) and then treat it as a a string.

Kyle, will bug 919885 fix this?
Blocks: 643325
Component: JavaScript Engine → DOM: Workers
Flags: needinfo?(khuey)
Keywords: regression
Looks like it will, since it will remove all this code.
Depends on: 919885
This looks very similar to bug 923649.  Or at least, it is crashing in the same line in ConstructInternal: https://crash-stats.mozilla.com/report/index/cd01212f-d20e-4ab3-8f63-459a82131008
That bug's testcase has:

  new Worker({toString:function(){}})

So yes, exactly the same issue.
Blocks: 923649
I'd prefer we fix this now rather than wait on bug 919885 though.
Attached patch changes.patchSplinter Review
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #815102 - Flags: review?(bzbarsky)
Comment on attachment 815102 [details] [diff] [review]
changes.patch

r=me
Attachment #815102 - Flags: review?(bzbarsky) → review+
Comment on attachment 815102 [details] [diff] [review]
changes.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.

Which older supported branches are affected by this flaw? None.

If not all supported branches, which bug introduced the flaw? Bug 643325.

How likely is this patch to cause regressions; how much testing does it need? Almost none, it restores old behavior.
Attachment #815102 - Flags: sec-approval?
Attachment #815102 - Flags: sec-approval? → sec-approval+
For future reference, trunk-only bugs don't need sec-approval+.
Flags: needinfo?(khuey)
(In reply to Andrew McCreight [:mccr8] from comment #9)
> For future reference, trunk-only bugs don't need sec-approval+.

Ah, ok. Thanks.
It should be fixed by the other bug, yes.  But lets land this anyways.
fixed in https://hg.mozilla.org/mozilla-central/rev/8aea67a6a17c - do we also need a testcase for this ?
Flags: in-testsuite?
Target Milestone: --- → mozilla27
FIXED per comment 13.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
type confusion sounds bad so I'm going to mark this sec-high.
Keywords: sec-high
Whiteboard: [dupe of bug 923649]
No longer depends on: 919885
Whiteboard: [dupe of bug 923649] → [first filed by Atte Kettunen as bug 923649]
I wasn't able to repro the original bug. If you get a chance Nils, it'd be great to verify that it no longer crashes for you. Thanks.
Group: core-security
You need to log in before you can comment on or make changes to this bug.