Closed
Bug 1415313
Opened 7 years ago
Closed 7 years ago
Assertion failure: isDouble(), at js/Value.h:344 with TypedObject
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
thunderbird_esr52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox57 | --- | unaffected |
firefox58 | --- | unaffected |
firefox59 | --- | verified |
People
(Reporter: decoder, Assigned: djvj)
References
Details
(6 keywords, Whiteboard: [jsbugmon:update,bisect])
Attachments
(1 file)
1.60 KB,
patch
|
jorendorff
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision 4e6df5159df3 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe): var Uint32Array = TypedObject.float32.array(3); const ONE_MINUS_EPSILON = 1 - Math.pow(2, -53); const f = new Float64Array([0, 0]); const u = new Uint32Array(f.buffer); const diff = function(a, b) { f[1] = b; u[3 - ENDIAN]; }; ENDIAN = 1; diff(1, ONE_MINUS_EPSILON) Backtrace: received signal SIGSEGV, Segmentation fault. #0 0x000000000055646c in JS::Value::setDouble (d=<optimized out>, this=0x7ffff42732d8) atdist/include/js/Value.h:344 #1 JS::Value::setNumber (this=this@entry=0x7ffff42732d8, d=<optimized out>) at dist/include/js/Value.h:415 #2 0x0000000000893ab9 in js::MutableWrappedPtrOperations<JS::Value, JS::MutableHandle<JS::Value> >::setNumber (d=<optimized out>, this=<optimized out>) at dist/include/js/Value.h:1359 #3 js::LoadScalarfloat::Func (cx=0x7ffff6948000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TypedObject.cpp:2794 #4 0x000000000055e631 in js::CallJSNative (cx=0x7ffff6948000, native=0x8939c0 <js::LoadScalarfloat::Func(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:291 #5 0x0000000000552dbf in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6948000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:472 #6 0x000000000055319d in InternalCall (cx=0x7ffff6948000, args=...) at js/src/vm/Interpreter.cpp:521 #7 0x0000000000546955 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:527 #8 Interpret (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:3061 #9 0x0000000000552965 in js::RunScript (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:422 #10 0x0000000000552e87 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6948000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:494 #11 0x000000000055319d in InternalCall (cx=0x7ffff6948000, args=...) at js/src/vm/Interpreter.cpp:521 #12 0x0000000000546955 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:527 #13 Interpret (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:3061 #14 0x0000000000552965 in js::RunScript (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:422 #15 0x0000000000552e87 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6948000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:494 #16 0x000000000055319d in InternalCall (cx=0x7ffff6948000, args=...) at js/src/vm/Interpreter.cpp:521 #17 0x0000000000546955 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:527 #18 Interpret (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:3061 #19 0x0000000000552965 in js::RunScript (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:422 #20 0x0000000000552e87 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6948000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:494 #21 0x000000000055319d in InternalCall (cx=0x7ffff6948000, args=...) at js/src/vm/Interpreter.cpp:521 #22 0x0000000000553300 in js::Call (cx=<optimized out>, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:540 #23 0x0000000000890c00 in Reify (cx=cx@entry=0x7ffff6948000, type=type@entry=..., typedObj=..., typedObj@entry=..., offset=8, to=..., to@entry=...) at js/src/builtin/TypedObject.cpp:171 #24 0x0000000000892508 in js::TypedObject::obj_getArrayElement (cx=0x7ffff6948000, typedObj=typedObj@entry=..., typeDescr=..., typeDescr@entry=..., index=index@entry=2, vp=vp@entry=...) at js/src/builtin/TypedObject.cpp:1860 #25 0x0000000000898f0f in js::TypedObject::obj_getElement (cx=0x7ffff6948000, obj=..., obj@entry=..., receiver=..., receiver@entry=..., index=2, vp=..., vp@entry=...) at js/src/builtin/TypedObject.cpp:1833 #26 0x00000000008991c3 in js::TypedObject::obj_getProperty (cx=0x7ffff6948000, obj=..., receiver=..., id=..., vp=...) at js/src/builtin/TypedObject.cpp:1770 #27 0x000000000055b3ad in js::GetProperty (cx=0x7ffff6948000, obj=..., receiver=..., id=..., vp=...) at js/src/vm/NativeObject.h:1603 #28 0x000000000055b46e in js::GetElement (cx=cx@entry=0x7ffff6948000, obj=obj@entry=..., receiver=receiver@entry=..., index=<optimized out>, vp=vp@entry=...) at js/src/jsobjinlines.h:238 #29 0x000000000053fff2 in js::GetObjectElementOperation (cx=0x7ffff6948000, op=<optimized out>, obj=..., receiver=..., key=..., res=...) at js/src/vm/Interpreter-inl.h:499 #30 0x0000000000541f52 in js::GetElementOperation (cx=0x7ffff6948000, op=JSOP_GETELEM, lref=..., rref=..., res=...) at js/src/vm/Interpreter-inl.h:627 #31 0x000000000054dd2a in Interpret (cx=0x7ffff6948000, state=...) at js/src/vm/Interpreter.cpp:2899 [...] #41 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8962 rax 0x0 0 rbx 0x7ffff42732e8 140737289597672 rcx 0x7ffff6c282ad 140737333330605 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffab60 140737488333664 rsp 0x7fffffffab60 140737488333664 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fe4740 140737354024768 r10 0x58 88 r11 0x7ffff6b9e7a0 140737332766624 r12 0x7ffff6948000 140737330315264 r13 0x8 8 r14 0x7ffff4700360 140737294369632 r15 0x7fffffffab70 140737488333680 rip 0x55646c <JS::Value::setNumber(double)+156> => 0x55646c <JS::Value::setNumber(double)+156>: movl $0x0,0x0 0x556477 <JS::Value::setNumber(double)+167>: ud2 Marking s-s because this assertion can indicate a type confusion.
Updated•7 years ago
|
Updated•7 years ago
|
Flags: needinfo?(sphink)
Priority: -- → P1
Comment 1•7 years ago
|
||
Do we think we can get this fixed and uplifted for the 58 cycle? Unsure of the urgency here.
Flags: needinfo?(jorendorff)
Comment 2•7 years ago
|
||
TypedObject is Nightly-only, but this is sec-critical in Nightly, trivially exploitable if attacker knows the address of any memory they control. Getting this fixed is urgent. Redirecting to a more appropriate ni? target. Kannan, can you take this? A JS::CanonicalizeNaN is missing *at least* in this one place (js::LoadScalar##T::Func). I didn't check whether any other places might have the same issue, or if the JITs optimize TypedObject element assignment.
Flags: needinfo?(sphink)
Flags: needinfo?(kvijayan)
Flags: needinfo?(jorendorff)
Comment 3•7 years ago
|
||
To be clear: due to this missing JS::CanonicalizeNaN, you can write any 64 bits to a location within a TypedArray buffer, then effectively reinterpret-cast those bits to a Value which is exposed to JS. Forge a pointer to any memory as a JSObject or JSString.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → kvijayan
Flags: needinfo?(kvijayan)
Assignee | ||
Comment 4•7 years ago
|
||
This fixes the issue. Try run (I realized in retrospect that it was probably unnecessary) here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e41f86a07625c72539d218629534a9564bbcd91 I searched around for any other cases of |Value::setNumber()| calls or instances in that file where we're pulling double bits out of somewhere and instantiating doubles. All the other uses seem fine.
Attachment #8933047 -
Flags: review?(jorendorff)
Comment 5•7 years ago
|
||
Comment on attachment 8933047 [details] [diff] [review] fix-bug-1415313.patch Review of attachment 8933047 [details] [diff] [review]: ----------------------------------------------------------------- Thanks. Please add a test.
Attachment #8933047 -
Flags: review?(jorendorff) → review+
Comment 6•7 years ago
|
||
Marking all versions unaffected, as this is Nightly-only.
status-firefox57:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Assignee | ||
Comment 7•7 years ago
|
||
This probably needs sec approval to land. It's a nightly-only bug (the feature is nightly-gated), but is an easy exploit and has been present in the code since 2013. Offending bug related to commit is: https://bugzilla.mozilla.org/show_bug.cgi?id=898362 The relevant commit is: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbdf3bb140c Specifically: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbdf3bb140c#l4.1646
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8933047 [details] [diff] [review] fix-bug-1415313.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Very easily. A constructed Float NaN with 22 controllable low bits at least can be lifted straight into a Value . Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes. Which older supported branches are affected by this flaw? None, this is a gated nightly feature, but the flaw has been present since 2013. If not all supported branches, which bug introduced the flaw? Bug 898362. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? N/A. But is in a feature that is gated to nightly. How likely is this patch to cause regressions; how much testing does it need? Very little testing needed. Fix is obvious, and a one-liner. No regressions expected.
Attachment #8933047 -
Flags: sec-approval?
Comment 9•7 years ago
|
||
(In reply to Kannan Vijayan [:djvj] from comment #8) > Which older supported branches are affected by this flaw? > > None, this is a gated nightly feature, but the flaw has been present since > 2013. This doesn't need sec-approval to land if it is disabled by default except on nightly.
Updated•7 years ago
|
Attachment #8933047 -
Flags: sec-approval?
Assignee | ||
Comment 11•7 years ago
|
||
Commit: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d8ec1c6eb16
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
status-firefox59:
--- → verified
Comment 12•7 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 13•7 years ago
|
||
For future reference, bugs don't get resolved until they're merged to m-c. https://hg.mozilla.org/mozilla-central/rev/2d8ec1c6eb16
Target Milestone: --- → mozilla59
Updated•6 years ago
|
Group: javascript-core-security → core-security-release
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•