imgDecoderObserver is refcounted on multiple threads

RESOLVED FIXED in Firefox 30

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: khuey, Assigned: milan)

Tracking

({sec-high})

unspecified
mozilla32
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox29 wontfix, firefox30 fixed, firefox31 fixed, firefox32+ fixed, firefox-esr2430+ fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, seamonkey2.26 wontfix)

Details

(Whiteboard: [qa-][adv-main30+][adv-esr24.6+])

Attachments

(3 attachments, 2 obsolete attachments)

Posted file Stack
But it does not have threadsafe refcounting.
This is probably at least sec-high.

Milan, is there somebody who can look into this?
Flags: needinfo?(milan)
Keywords: sec-high
I'm assuming security issue because of a possible UAF.

I don't know if it's a surprise that imgDecoderObserver is doing reference counting on multiple threads, and if it is, then this patch doesn't make sense, and a proper solution is needed. On the off chance that we do want reference counting on multiple threads, the fix is simple and I thought I'd save everybody some time.  I don't know if there are any performance implications with this.
Attachment #8407725 - Flags: review?(seth)
Flags: needinfo?(milan)
CC-ing bjacob on the off chance that this is related to https://bugzilla.mozilla.org/show_bug.cgi?id=987010#c38
Milan, can you please use NS_INLINE_DECL_THREADSAFE_REFCOUNTING instead of deriving from AtomicRefCounted?  We're switching away from using (Atomic)RefCounted in Gecko.  Bonus points for making the destructor protected as well!
(Oh and BTW you can get rid of the MOZ_DECLARE_REFCOUNTED_TYPENAME macro there as well.  thanks!)
Attachment #8407725 - Attachment is obsolete: true
Attachment #8408343 - Flags: review?(seth)
Attachment #8408343 - Flags: review?(ehsan)
Assignee: nobody → milan
Comment on attachment 8408343 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.

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

Looks great.  Thanks!
Attachment #8408343 - Flags: review?(ehsan) → review+
Are we confident there are no other thread safety issues here beyond the reference counting?
(In reply to (Away 4/19-5/7) from comment #9)
> Are we confident there are no other thread safety issues here beyond the
> reference counting?

Milan, can you please help find someone who can answer this question?  Thanks!
Flags: needinfo?(milan)
Seth is probably the right person. Once he reviews the existing patch, we can see if there are other issues to chase?
Flags: needinfo?(milan)
Duplicate of this bug: 1000822
Seth, review ping and maybe answer to comment 9?
Flags: needinfo?(seth)
Group: gfx-core-security
seth: ping?
Comment on attachment 8408343 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.

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

Thanks for taking a look at this, Milan.

It seems that the OMT reference counting is caused by nsICODecoder's use of a nested decoder, which is legitimate. It would be possible to restructure the code to avoid this, but that's a much bigger change and would probably result in a patch that would be hard to backport anywhere. I think this is the right fix for now.

The commit message should read 'r=seth,ehsan'.
Attachment #8408343 - Flags: review?(seth) → review+
Comment on attachment 8418202 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.  Carry r=seth,ehsan

Sorry!  Sent this to inbound and then realized I should have asked for security approval.  

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

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

Which older supported branches are affected by this flaw?
All of them.

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?
Easy to do.

How likely is this patch to cause regressions; how much testing does it need?
Not likely.
Attachment #8418202 - Flags: sec-approval?
Comment on attachment 8418202 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.  Carry r=seth,ehsan

Well, too late to back out now, so sec-approval+.

Let's get Aurora, Beta, and ESR24 patches.
https://hg.mozilla.org/mozilla-central/rev/656d626d8fd1
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attachment #8418202 - Flags: sec-approval? → sec-approval+
Comment on attachment 8418202 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.  Carry r=seth,ehsan

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Very difficult to implement security attack.
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): I guess we are open to the attack.
String or IDL/UUID changes made by this patch: None.

ESR24 version requires a different patch, aurora and beta can use the existing patch.
Attachment #8418202 - Flags: approval-mozilla-beta?
Attachment #8418202 - Flags: approval-mozilla-aurora?
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: It would be a very difficult way to attack through this, so I guess a theoretical impact?
Fix Landed on Version: 32
Risk to taking this patch (and alternatives if risky): Low risk 
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8418919 - Flags: review+
Attachment #8418919 - Flags: approval-mozilla-esr24?
Attachment #8418202 - Flags: feedback+
Attachment #8418202 - Flags: approval-mozilla-beta?
Attachment #8418202 - Flags: approval-mozilla-beta+
Attachment #8418919 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
I assume that the Aurora approval was meant to be set along with the others, but can we do so officially please? :)
Comment on attachment 8418202 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.  Carry r=seth,ehsan

Right!
Attachment #8418202 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: gfx-core-security
Marking [qa-] for verification.
Whiteboard: [qa-]
Whiteboard: [qa-] → [qa-][adv-main30+][adv-esr24.6+]
This was crashing on all platforms on me during transplant attempt --- so wontfix for SeaMonkey 2.26.1
Group: core-security
You need to log in before you can comment on or make changes to this bug.