Last Comment Bug 848535 - (CVE-2013-1694) Use of PreserveWrapper in cases when we don't have a wrapper seems broken
: Use of PreserveWrapper in cases when we don't have a wrapper seems broken
: sec-high
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
-- normal (vote)
: mozilla24
Assigned To: Olli Pettay [:smaug] (pto-ish for couple of days)
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2013-03-06 13:30 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2014-11-19 19:48 PST (History)
10 users (show)
ryanvm: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.74 KB, patch)
2013-05-07 14:49 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
no flags Details | Diff | Splinter Review
patch (2.24 KB, patch)
2013-05-07 14:55 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
continuation: review+
abillings: sec‑approval+
Details | Diff | Splinter Review
for branches (2.17 KB, patch)
2013-05-21 11:52 PDT, Olli Pettay [:smaug] (pto-ish for couple of days)
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr17+
akeybl: approval‑mozilla‑b2g18+
Details | Diff | Splinter Review

Description User image Boris Zbarsky [:bz] (still a bit busy) 2013-03-06 13:30:35 PST
Consider the following scenario:

1)  XHR starts.
2)  Its wrapper, if any, is collected.
3)  A C++ readystatechange listener calls GetResponse, and the response type is
    JSON or arraybuffer.  This causes nsXMLHttpRequest::RootJSResultObjects to be
    called, which calls PreserveWrapper.
4)  JS touches the XHR object, causing it to be wrapped, which calls

In a debug build this will assert fatally. In an opt build this will clear the preserved-wrapper flag on the wrapper cache, which seems bad.

Should we just preserve that flag on SetWrapper and remove the assert in that method, perhaps?  Alternately, we should stop using PreserveWrapper to mean "hold JS objects" in XHR.

Note that the above scenario is pretty simple to produce with a worker XHR if extension JS can ever touch the main-thread nsXMLHttpRequest of it, as far as I can tell.
Comment 1 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-03-06 14:32:36 PST
nsDOMFileReader is possibly similarly broken.
Comment 2 User image Andrew McCreight [:mccr8] 2013-03-13 10:15:14 PDT
Sounds kind of bad.  Feel free to adjust as needed.
Comment 3 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-03-25 02:24:44 PDT
May not get to this before next week.
Comment 4 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-05-07 14:49:54 PDT
Created attachment 746645 [details] [diff] [review]

This should do it. Technically drop in unlink isn't absolutely needed, but
we do drop preserved wrapper there too if there is such (in nsDOMEventTargetHelper). These aren't super perf critical so extra drop isn't too bad.
Comment 5 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-05-07 14:53:50 PDT
But then, drop in unlink is rather useless... I'll remove it
Comment 6 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-05-07 14:55:15 PDT
Created attachment 746648 [details] [diff] [review]
Comment 7 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-05-07 16:39:26 PDT
Comment on attachment 746648 [details] [diff] [review]

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
As of now we don't know any exploit, but I don't think constructing one is too difficult.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
commit will be about simpler js object holding

Which older supported branches are affected by this flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
backports should be almost the same

How likely is this patch to cause regressions; how much testing does it need?
Should be relatively safe
Comment 8 User image Al Billings [:abillings] 2013-05-08 10:00:12 PDT
sec-approval+ for m-c on 5/14 after we branch and ship. Please prepare and nominate branch patches as well.
Comment 9 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-05-15 05:28:28 PDT
Comment 10 User image Ryan VanderMeulen [:RyanVM] 2013-05-15 18:38:57 PDT
Comment 11 User image Alex Keybl [:akeybl] 2013-05-20 13:28:46 PDT
Ready for uplift noms?
Comment 12 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-05-20 13:35:18 PDT
Preparing patches ....
Comment 13 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2013-05-21 11:52:12 PDT
Created attachment 752297 [details] [diff] [review]
for branches

This patch seems to apply to branches. esr17 needs --fuzz=4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Something ancient
User impact if declined: Possible GC/CC related crashes
Testing completed (on m-c, etc.): landed to m-c 2013-05-15
Risk to taking this patch (and alternatives if risky): Shouldn't be risky
String or IDL/UUID changes made by this patch: NA
Comment 14 User image Lukas Blakk [:lsblakk] use ?needinfo 2013-05-22 09:34:54 PDT
Comment on attachment 752297 [details] [diff] [review]
for branches

approving for desktop branches, holding off on b2g18 until we know if this needs to be uplifted to v1.0.1 - since it's only sec-high, I suspect it does not but will wait for confirmation.
Comment 16 User image Ryan VanderMeulen [:RyanVM] 2013-05-22 13:52:21 PDT
Comment 17 User image Matt Wobensmith [:mwobensmith][:matt:] 2013-06-19 16:56:57 PDT
Based on the steps in comment 0 and the lack of an existing test case, we're going to mark this qa- for verification purposes. If that changes, remove qa- and/or let us know how we could otherwise verify fixed. Thanks.

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