Hit MOZ_CRASH(imgCacheValidator not thread-safe) at nsISupportsImpl.cpp:43

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cbook, Assigned: tnikkel)

Tracking

(Blocks 1 bug, 4 keywords)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56+ fixed)

Details

()

Attachments

(2 attachments)

Posted file bughunter stack
Found via bughunter and reproduced on latest trunk m-c tinderbox build on windows 7.

Affects according to bughunter linux/windows on trunk debug.

Steps to reproduce:
-> Load http://www.ceskatelevize.cz/sport/zive-vysilani/
--> Moz_Crash is hit on load 

Hit MOZ_CRASH(imgCacheValidator not thread-safe) at nsISupportsImpl.cpp:43

filing as s-s just in case
Bill, Nathan: Could you take look ? thanks !
Flags: needinfo?(wmccloskey)
Flags: needinfo?(nfroyd)
This is an imglib problem.  Maybe a recent one?  I would have suspected it would have shown up much sooner.
Component: XPCOM → ImageLib
Flags: needinfo?(wmccloskey)
Flags: needinfo?(tnikkel)
Flags: needinfo?(nfroyd)
This looks like it could be a regression from bug 1365519, which I think added an addref of the listener off the main thread to nsUnknownDecoder::OnDataAvailable().
Flags: needinfo?(dd.mozilla)
I made nsUnknownDecoder be able to deliver OnDataAvailable of the main thread. But any listener in the chain can veto on that if it is not thread safe. A listener that is thread safe to receive OnDataAvailable of the main thread implements nsIThreadRetargetableStreamListener. If it is not safe to receive OnDataAvailable of the main thread it does not implement that interface or it returns an error from function CheckListenerChain.
Flags: needinfo?(dd.mozilla)
I don't know this area super well but imgCacheValidator was made to subclass nsIThreadRetargetableStreamListener in https://hg.mozilla.org/mozilla-central/rev/799dffa9306b "Support OnDataAvailable and OnStopRequest off main thread for image loading". So presumably it is safe to call OnDataAvailable off the main thread on imgCacheValidator and the only problem here is that imgCacheValidator should have been made to be using threadsafe refcounting.
Flags: needinfo?(tnikkel)
Group: core-security → gfx-core-security
I can't reproduce the bug, so I can't be sure, but I think this should fix it. I need to go over the imgCacheValidator code to make sure that it isn't doing anything else that's not threadsafe, but it should be in good condition given that it's been used off main thread for years now.
Assignee: nobody → tnikkel
Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

I went over imgCacheValidator, the only method which can be called off main thread is OnDataAvailable, and it doesn't access any member variables, it just calls OnDataAvailable of the imgRequest,  which should be fully thread safe. So this should be the correct fix.
Attachment #8881922 - Flags: review?(aosmond)
Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

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

This would be an existing problem, but imgCacheValidator::~imgCacheValidator calls imgRequest::SetValidator. If imgCacheValidator somehow gets freed off the main thread, isn't it possible to conflict with the main thread calling imgRequest::GetValidator? For example:

http://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/image/imgRequestProxy.cpp#666

In imgRequestProxy::PerformClone, we first query imgRequest::GetValidator to see if it has an imgCacheValidator and then call imgCacheValidator::AddProxy if it does. Could it get freed during this window?
Tracking for 56 since this is rated sec-high. Sounds like this may affect all current versions? If so it should have sec approval before landing
(In reply to Andrew Osmond [:aosmond] from comment #8)
> This would be an existing problem, but imgCacheValidator::~imgCacheValidator
> calls imgRequest::SetValidator. If imgCacheValidator somehow gets freed off
> the main thread, isn't it possible to conflict with the main thread calling
> imgRequest::GetValidator? For example:

Good point. Do we have any guarantee that the networking code will release the last reference to the listener passed to SetNotificationsCallbacks and AsyncOpen2 on the main thread? Or do they have to deal with their destructor potentially running off the main thread? Since OnDataAvailable is the only method called off main thread and there needs to be a OnStopRequest call after that I think that should be a natural way to write the code.
Flags: needinfo?(dd.mozilla)
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> (In reply to Andrew Osmond [:aosmond] from comment #8)
> > This would be an existing problem, but imgCacheValidator::~imgCacheValidator
> > calls imgRequest::SetValidator. If imgCacheValidator somehow gets freed off
> > the main thread, isn't it possible to conflict with the main thread calling
> > imgRequest::GetValidator? For example:
> 
> Good point. Do we have any guarantee that the networking code will release
> the last reference to the listener passed to SetNotificationsCallbacks and
> AsyncOpen2 on the main thread? Or do they have to deal with their destructor
> potentially running off the main thread? Since OnDataAvailable is the only
> method called off main thread and there needs to be a OnStopRequest call
> after that I think that should be a natural way to write the code.

OnStartRequest and OnStopRequest is called on the main thread. OnStopRequest is called after all of OnDataAvailable, this is guaranteed.
Flags: needinfo?(dd.mozilla)
Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

Even if it can be freed off the main thread, I think this is all we can do inside imagelib (there would need to be a corresponding change in the owners of imgCacheValidator to ensure what I was concerned about doesn't happen).
Attachment #8881922 - Flags: review?(aosmond) → review+
Blocks: 1365519
Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

one would have to find the place that is doing the off main thread addref of this class, then get the timing of that to work out to conflict with another addref on the main thread. thats pretty unlikely given the structure of this code i would guess

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

no

Which older supported branches are affected by this flaw?

caused by bug 1365519 -> 56 only

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

caused by bug 1365519

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

don't need

How likely is this patch to cause regressions; how much testing does it need?

safe
Attachment #8881922 - Flags: sec-approval?
Comment on attachment 8881922 [details] [diff] [review]
threadsaferefcntimgcachevalidator

sec-approval+
Attachment #8881922 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/16fb069369c9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Group: gfx-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.