Closed Bug 1263549 Opened 8 years ago Closed 8 years ago

Inlined RegExpPrototypeOptimizable and RegExpInstanceOptimizable reads from wrong address.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main48-])

Attachments

(1 file)

Related to bug 1263532.

In inlined RegExpPrototypeOptimizable, optimizable shape address is calculated with following:

>     masm.loadJSContext(temp);
>     masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp);
>     masm.loadPtr(Address(temp, JSCompartment::offsetOfRegExps()), temp);
>     masm.loadPtr(Address(temp, RegExpCompartment::offsetOfOptimizableRegExpPrototypeShape()),
>                  temp);

But C++ version is following:

>     Shape* shape = cx->compartment()->regExps.getOptimizableRegExpPrototypeShape();

`regExps` member is *not* a pointer:

>     js::RegExpCompartment        regExps;

thus the inlined version should be following:

>     masm.loadJSContext(temp);
>     masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp);
>     masm.loadPtr(Address(temp,
>                          JSCompartment::offsetOfRegExps() +
>                          RegExpCompartment::offsetOfOptimizableRegExpInstanceShape()), temp);

Same for RegExpInstanceOptimizable.


The loaded wrong value is used only for comparison, and discarded instantly, so this won't be exploitable, but marking s-s just in case.

This means that inlined version always jumps to OOL.  fixing this might help performance issues.
this is from bug 887016.
Blocks: 887016
Adding this to javascript-security-sensitive group. (This was previously, and still is, "Security-Sensitive Core Bug")
Group: javascript-core-security
This doesn't introduce any visible behavior change, so no testcase attached.
Assignee: nobody → arai.unmht
Attachment #8739900 - Flags: review?(hv1989)
Group: core-security
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Adding this to javascript-security-sensitive group. (This was previously,
> and still is, "Security-Sensitive Core Bug")

"double-grouping" is worse than leaving it alone. Moving it to the Javascript bug group helps the javascript folks see it, but if you leave it in the default group also then only the INTERSECTION of the JS team and the triage team can see it.
(In reply to Daniel Veditz [:dveditz] from comment #4)
> "double-grouping" is worse than leaving it alone. Moving it to the
> Javascript bug group helps the javascript folks see it, but if you leave it
> in the default group also then only the INTERSECTION of the JS team and the
> triage team can see it.

I didn't have the permissions to remove it from the "core-security" group.
Comment on attachment 8739900 [details] [diff] [review]
Fix inlined RegExpPrototypeOptimizable and RegExpInstanceOptimizable.

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

Good find!

::: js/src/jit/CodeGenerator.cpp
@@ +2151,5 @@
>      masm.loadJSContext(temp);
>      masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp);
> +    masm.loadPtr(Address(temp,
> +                         JSCompartment::offsetOfRegExps() +
> +                         RegExpCompartment::offsetOfOptimizableRegExpPrototypeShape()), temp);

can you create a variable for the offset so this line would fit on one line?

something like:
>    size_t offsetOfOptimizableShapes = JSCompartment::offsetOfRegExps() +
>                                       RegExpCompartment::offsetOfOptimizableRegExpPrototypeShape());
>    masm.loadPtr(Address(temp, offsetOfOptimizableShapes, temp);

@@ +2220,5 @@
>      masm.loadJSContext(temp);
>      masm.loadPtr(Address(temp, JSContext::offsetOfCompartment()), temp);
> +    masm.loadPtr(Address(temp,
> +                         JSCompartment::offsetOfRegExps() +
> +                         RegExpCompartment::offsetOfOptimizableRegExpInstanceShape()), temp);

same as above. Please create local variable for the offset.
Attachment #8739900 - Flags: review?(hv1989) → review+
With this bug, it can load a single pointer-sized value from wrong address, but the value is used only for comparing with other pointer value (the Shape of JS object).  The result of the comparison is also not observable from script, so this should be sec-other (Bugs that may not be exploitable security issues)
https://hg.mozilla.org/integration/mozilla-inbound/rev/00cb5de040be36971fa941cb52fcc679fe67878d
Bug 1263549 - Fix inlined RegExpPrototypeOptimizable and RegExpInstanceOptimizable. r=h4writer
https://hg.mozilla.org/mozilla-central/rev/00cb5de040be
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Group: javascript-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main48-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.