Last Comment Bug 907727 - (CVE-2013-1737) Accessor user-defined properties on DOM proxies get the wrong "this" object: the expando object
(CVE-2013-1737)
: Accessor user-defined properties on DOM proxies get the wrong "this" object: ...
Status: RESOLVED FIXED
[still needs a test checked in][adv-m...
: sec-moderate
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla26
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-21 06:56 PDT by Boris Zbarsky [:bz]
Modified: 2014-11-19 20:12 PST (History)
9 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
verified
+
verified
+
verified
24+
verified
24+
fixed
wontfix
wontfix
fixed


Attachments
Testcase (376 bytes, text/html)
2013-08-21 06:56 PDT, Boris Zbarsky [:bz]
no flags Details
Make sure to properly forward gets to the expando object for DOM proxies. (1.23 KB, patch)
2013-08-21 08:40 PDT, Boris Zbarsky [:bz]
peterv: review+
abillings: approval‑mozilla‑aurora+
abillings: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑b2g18+
abillings: sec‑approval+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2013-08-21 06:56:13 PDT
Created attachment 793481 [details]
Testcase

If you load the attached testcase, you get an exception when you should get the HTMLOptionElement.  The reason is this callstack:

#11 0x000000010187efd1 in JS_GetPropertyById (cx=0x115e0f640, objArg=0x116190220, idArg={asBits = 4643216832}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::UnbarrieredMutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x10f9440b8}) at jsapi.cpp:3753
#12 0x0000000106394b20 in mozilla::dom::HTMLSelectElementBinding::DOMProxyHandler::get (this=0x1093d1b40, cx=0x115e0f640, proxy={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf9470}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::UnbarrieredMutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x10f9440b8}) at HTMLSelectElementBinding.cpp:1313
#13 0x0000000101a12889 in js::Proxy::get (cx=0x115e0f640, proxy={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf9440}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf9470}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::UnbarrieredMutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x10f9440b8}) at jsproxy.cpp:2487

and now we're doing the get on the expando object, and that's what the getter will see.

This is bad, because if you just use a scripted getter you get the expando object as "this"!  Going to mark as security-sensitive for that reason....
Comment 1 Boris Zbarsky [:bz] 2013-08-21 08:40:25 PDT
Created attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.
Comment 2 Boris Zbarsky [:bz] 2013-08-21 08:40:59 PDT
Note: no testcase in the patch because the security situation worries me.
Comment 3 Boris Zbarsky [:bz] 2013-08-21 08:44:18 PDT
Comment on attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Unclear; I don't know an obvious way this can be exploited.

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

Which older supported branches are affected by this flaw?  I think everything back to b2g18 or so.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  I think this patch should apply as-is to the various branches.

How likely is this patch to cause regressions; how much testing does it need?  The main regression risk would be custom getters on Xrays for DOM proxies, which I expect to be rare if they exist at all.
Comment 4 Andrew McCreight [:mccr8] 2013-08-21 09:13:05 PDT
Conservatively setting this to "high".
Comment 5 Al Billings [:abillings] 2013-08-21 09:31:10 PDT
Comment on attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.

sec-approval+ for trunk. Approving for aurora and beta after that. I assume Peter still needs to r+ it?
Comment 6 Boris Zbarsky [:bz] 2013-08-21 09:39:42 PDT
Comment on attachment 793522 [details] [diff] [review]
Make sure to properly forward gets to the expando object for DOM proxies.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Longstanding bug in DOM proxy bindings
User impact if declined: Unclear: we have an invariant violation, but I'm not
   sure what can be done with it.
Testing completed: Basic testing of the functionality in webpages
Risk to taking this patch (and alternatives if risky): I think it's low risk. 
   The other option is to just not take it.
String or UUID changes made by this patch: None.
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-08-22 13:00:39 PDT
https://hg.mozilla.org/mozilla-central/rev/83d003b30be4
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2013-08-28 09:16:22 PDT
Please nominate for esr17 approval as well (or prepare a branch-specific patch if need be).
Comment 11 Ryan VanderMeulen [:RyanVM] 2013-08-28 10:03:16 PDT
https://hg.mozilla.org/releases/mozilla-esr17/rev/8236a3be778d
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-08-31 08:52:12 PDT
b2g18 is frozen for non-leo+ blockers. Please request blocking status if you feel that this must land there.
Comment 13 Boris Zbarsky [:bz] 2013-08-31 18:04:31 PDT
That's something branch drivers need to decide...
Comment 14 Alex Keybl [:akeybl] 2013-09-03 12:37:25 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> b2g18 is frozen for non-leo+ blockers. Please request blocking status if you
> feel that this must land there.

I'm sorry, I fucked this up. I fed RyanVM bad information. The v1.1 branch will be open until 3/3/2014, as it's being maintained for security until that time.
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-09-04 06:59:52 PDT
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f1dcd2d985f
Comment 17 Matt Wobensmith [:mwobensmith][:matt:] 2013-09-11 17:41:39 PDT
Confirmed bug in m-c 2013-08-22.
Verified fixed in FF24, FF25 and FF26, 2013-09-11.

On FF17.0.9esr only, I see no JS alert box. This seems weird. I do see the following in the error console, which does not appear in FF17.0.8esr:

"Warning: XUL box for _moz_generated_content_before element contained an inline #text child, forcing all its children to be wrapped in a block."

Is the behavior we desire?
Comment 18 Boris Zbarsky [:bz] 2013-09-11 17:59:01 PDT
That warning has nothing to do with this bug...
Comment 19 Daniel Veditz [:dveditz] 2013-09-17 02:09:30 PDT
(In reply to Andrew McCreight [:mccr8] (mostly PTO through 9/22) from comment #4)
> Conservatively setting this to "high".

if bz doesn't even know for sure if this can be abused (comment 6) that seems too high.

Note You need to log in before you can comment on or make changes to this bug.