Testcase requires https://github.com/MozillaSecurity/funfuzz/tree/master/dom/extension in order to call gczeal. Easiest way is to run funfuzz/dom/automation/domInteresting.py <build> <testcase>. Debug: [@ JSAutoCompartment::JSAutoCompartment] - Invalid read [@ js::CompartmentChecker::fail] - Compartment mismatch Debug+ASan: [@ JSAutoCompartment::JSAutoCompartment] - Invalid read Non-debug: Testcase does not run because gczeal is disabled.

Could you look at this, Andrea? The test case involves console.

Andrea and I looked at this a bit this morning. The ConsoleCallData is not tracing mArguments and mGlobal and they get moved on it. It needs to trace.

We store JSValues/JSObjects in 2 objects: ConsoleCallData and ConsoleProfileRunnable. For ConsoleCallData I trace its JSValues when on the main-thread. For workers, I don't do it because in ConsoleCallDataRunnable::ProcessCallData everything is JS::Rooted and, the previous JSValues have been clean up in ConsoleCallDataRunnable::PreDispatch. About ConsoleProfileRunnable, the mArguments is used only on main-thread and it comes from a Sequence of Rooted objects. So, I guess I don't have to trace them. For workers we have a similar approach as ConsoleCallData. This patch also adds some 'const' and some assertions about threads.

Comment on attachment 8715150 [details] [diff] [review] console.patch > for workers, I don't do it because in > ConsoleCallDataRunnable::ProcessCallData everything is JS::Rooted and, the > previous JSValues have been clean up in ConsoleCallDataRunnable::PreDispatch. More precisely, you do do it while the ConsoleCallData is holding on to stuff. Then when the ConsoleCallDataRunnable structured clones it and tells the ConsoleCallData to drop it, and then we don't trace it anymore. Which is fine, since at that point it's not holding on to anything. > So, I guess I don't have to trace them. [ConsoleProfileRunnable::mArguments] As currently constituted, you do have to trace them in ConsoleProfileRunnable. However, as discussed on IRC mArguments is only used during PreDispatch, which is running while the original aData of Console::ProfileMethod is on the stack, so we can just make mArguments a reference to aData and be ok here. The reason you need to trace if you copy is that, again, our GC can move objects. So say mArguments[0] is an ObjectValue for some object, and we made a copy as now. Now we have two separate ObjectValues pointing to this object: mArguments[0] and aData[0]. Say a GC comes along and moves the object. aData[0] is explicitly rooted in the binding code, so it gets updated to point to the new location. But mArguments[0] is not something the GC knows about, so it doesn't know to update it to point to the new location and we now have a dangling pointer. If mArguments is just a reference to aData this is not a problem, of course, because GC knows to update aData[0]. OK, on to the actual patch... The code in ConsoleCallDataRunnable::ProcessCallData is not ok. The reason it's not OK is that it's putting stuff in mCallData->mArguments and mCallData->mGlobal but they never get traced. And then you run into the problem described above. The rest of this looks reasonable, but we need to solve this issue.

Comment on attachment 8715925 [details] [diff] [review] console0.patch We should probably assert that Initialize() and the mConsole block inCleanupJSObjects() happen on the owning thread, since that's the only thread where refcounting mConsole is ok, right? >+ SequenceRooter<JS::Value> arguments(aCx, &values); This needs to go on the stack right after the Sequence<JS::Value>. Right now that sequence and the stuff it contains is not rooted while you're filling it! >+ const Sequence<JS::Value>& mArguments; This needs some lifetime documentation explaining why this is safe. In fact, mCopiedArguments could also use documentation explaining how it gets rooted/used and when... For example, why do we even need mCopiedArguments on the CallData? Could we not store it on the ConsoleCallDataRunnable instead, since that's what really uses them? > Console::IncreaseCounter(JSContext* aCx, const ConsoleStackEntry& aFrame, >- const nsTArray<JS::Heap<JS::Value>>& aArguments) >+ const Sequence<JS::Value>& aArguments) Fix the indent while you're here? r=me with the above fixed.

(In reply to Boris Zbarsky [:bz] from comment #6) > Comment on attachment 8715925 [details] [diff] [review] > console0.patch > > We should probably assert that Initialize() and the mConsole block > inCleanupJSObjects() happen on the owning thread, since that's the only > thread where refcounting mConsole is ok, right? I submitted a patch for this follow up: bug 1245957. > >+ SequenceRooter<JS::Value> arguments(aCx, &values); > For example, why do we even need mCopiedArguments on the CallData? Could we > not store it on the ConsoleCallDataRunnable instead, since that's what > really uses them? Because I need for something else... I work on a storage of these ConsoleCallData in Console object.

Comment on attachment 8715925 [details] [diff] [review] console0.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Console API Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes. bz wrote important comments about it. Which older supported branches are affected by this flaw? all. Console API in C++ is in beta and aurora. (and release). Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? We can write it if needed. How likely is this patch to cause regressions; how much testing does it need? It's green on try... but it's a complex patch.

> Because I need for something else... For what else? When does that something else happen? Basically, please document all the lifetimes very carefully, since this code is making so many assumptions about them.

The current way we release ConsoleCallData object, after the previous patch is wrong. We must release it on the owning thread.

Comment on attachment 8716208 [details] [diff] [review] console1.patch r=me, I think....

Should we be taking the console0 patch or console1?

both. If you prefer I can merge them in 1 single patch.

Can I land this code to m-i?

I'll give sec-approval+. You realize it is the weekend right? :-) We'll want patch nominations for Aurora, Beta, and ESR38 as well. As a sec-high, this needs to go to those three also after it is on trunk.

Andrea, could you fill the uplift requests? Thanks

Comment on attachment 8715925 [details] [diff] [review] console0.patch Approval Request Comment [Feature/regressing bug #]: Console API [User impact if declined]: A crash can occur because the JS arguments are stored but not correctly traced. [Describe test coverage new/current, TreeHerder]: no tests. [Risks and why]: The patch is relatively simple: I don't see big risks. [String/UUID change made/needed]: none Please: uplift the patch I pushed on m-i and not these 2. The patch I pushed is just the 2 patches in one.

Comment on attachment 8715925 [details] [diff] [review] console0.patch sec high + fix a crash, taking it. Should be in 45 beta 5.

has problems with uplift to aurora grafting 327626:d1e05b9116cc "Bug 1244995 - Console should trace the arguments correctly, r=bz" merging dom/base/Console.cpp merging dom/base/Console.h warning: conflicts while merging dom/base/Console.h! (edit, then use 'hg resolve --mark') abort: unresolved conflicts, can't continue (use hg resolve and hg graft --continue) baku can you take a look ?

Baku, could you please nominate a patch for uplift to esr38? Thanks!

Comment on attachment 8715925 [details] [diff] [review] console0.patch [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: A crash can occur because the JS arguments are stored but not correctly traced. Fix Landed on Version: 46 47 48 Risk to taking this patch (and alternatives if risky): this patch is quite stable on current beta/aurora/inbound... it's not a small patch but seems very stable. String or UUID changes made by this patch: none In case the patch doesn't apply, please, NI me and I'll provide a new version of it.

Comment on attachment 8715925 [details] [diff] [review] console0.patch Sec-high issue that has been on Beta, Aurora for a week now. Let's uplift to ESR38.7

has problems uplifting to esr38 - grafting 328611:3935b61c3ed3 "Bug 1244995 - Console should trace the arguments correctly, r=bz a=sylvestre" merging dom/base/Console.cpp merging dom/base/Console.h warning: conflicts while merging dom/base/Console.cpp! (edit, then use 'hg resolve --mark') warning: conflicts while merging dom/base/Console.h! (edit, then use 'hg resolve --mark')

This doesn't apply because these patches are missing: 1049091 1154076 1125205 1167423 1184557 1223774. Probably not all of them are required, but the code in ESR38 is quite different than what we have in m-i... are we sure we want to uplift it? If this is not a top crash, we can probably keep ESR38 as it is. NI bz because he reviewed this code.

> If this is not a top crash, we can probably keep ESR38 as it is. Seems to me that the fact that this is likely exploitable is a problem, no?

Baku, if this is exploitable, we should consider backporting the minimal fix needed to address the sec issue. I haven't looked at the fixes from all the bugs you noted: 1049091 1154076 1125205 1167423 1184557 1223774 but is there a good middle ground here? We go live with esr38.7 two weeks from today so we do have time to fix this the right way. Dan, Al: See comment 28 and 29. Are you ok to wontfix this for esr38.7? If the likelihood of a chemspill is high, I think we should fix this.

This is a sec-high bug and seems bad. I think we should fix this on ESR38 if possible.

Of the six bugs Baku proposes we'd also need on ESR=38 I note one of them is a security bug regression from one of the others. Obviously that one is fixed (and it was not a complicated fix) but it does make me wary of messing with this code on the ESR branch--what important differences between the branch might have been missed? I'm especially concerned about taking those patches since the main thrust seems to be about adding functionality (console logging from workers) which we usually avoid. The symptoms of the testcase are different on ESR38: instead of a crash while running the testcase I instead experience a null de-ref on shutdown. It may still be caused by the memory corruption this patch fixes, but the differences in logging code that make it hard to apply this patch may make it harder to exploit.

(In reply to Daniel Veditz [:dveditz] from comment #32) > Of the six bugs Baku proposes we'd also need on ESR=38 I note one of them is > a security bug regression from one of the others. Obviously that one is > fixed (and it was not a complicated fix) but it does make me wary of messing > with this code on the ESR branch--what important differences between the > branch might have been missed? I'm especially concerned about taking those > patches since the main thrust seems to be about adding functionality > (console logging from workers) which we usually avoid. > > The symptoms of the testcase are different on ESR38: instead of a crash > while running the testcase I instead experience a null de-ref on shutdown. > It may still be caused by the memory corruption this patch fixes, but the > differences in logging code that make it hard to apply this patch may make > it harder to exploit. Thanks Al and Dan. I am ok with wontfix'ing this one if it isn't so easy to exploit. Please let me know if there are any concerns.

Terrence would know better whether this is harder to exploit or not... Note that the crash in this bug is due to the global moving, but the patch also fixes the _arguments_ moving, which the code totally didn't handle. And pages have way more control over those objects.

NI Terrence for comment 34.

I'm not entirely sure what "this" is, exactly. That said, if the ESR only uses it from the main thread and we care more about security than a bit of edge case performance: why don't we just make everything a PersistentRooted?