994907 - imgDecoderObserver is refcounted on multiple threads

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Stack

But it does not have threadsafe refcounting.

Comment 1

[Andrew McCreight [:mccr8]]

This is probably at least sec-high.

Milan, is there somebody who can look into this?

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use thread safe reference counting for imgDecoderObserver.

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.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

CC-ing bjacob on the off chance that this is related to https://bugzilla.mozilla.org/show_bug.cgi?id=987010#c38

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=cbd6882a6e65

Comment 5

[(no longer active)]

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!

Comment 6

[(no longer active)]

(Oh and BTW you can get rid of the MOZ_DECLARE_REFCOUNTED_TYPENAME macro there as well.  thanks!)

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use thread safe reference counting for imgDecoderObserver.

Comment 8

[(no longer active)]

Comment on attachment 8408343 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.

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

Looks great.  Thanks!

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Are we confident there are no other thread safety issues here beyond the reference counting?

Comment 10

[(no longer active)]

(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!

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Seth is probably the right person. Once he reviews the existing patch, we can see if there are other issues to chase?

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Seth, review ping and maybe answer to comment 9?

Comment 14

[(no longer active)]

seth: ping?

Comment 15

[Seth Fowler [:seth] [:s2h]]

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'.

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Use thread safe reference counting for imgDecoderObserver. Carry r=seth,ehsan

Just change the commit message.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/656d626d8fd1

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 19

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/656d626d8fd1

Comment 21

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: 24ESR patch, otherwise same as above. Carry r=seth,ehsan

[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.

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

I assume that the Aurora approval was meant to be set along with the others, but can we do so officially please? :)

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8418202 [details] [diff] [review]
Use thread safe reference counting for imgDecoderObserver.  Carry r=seth,ehsan

Right!

Comment 25

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/1f39d0449e09

Comment 26

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4c247bfe9d75
https://hg.mozilla.org/releases/mozilla-beta/rev/adafd31af170
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/655771204329
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/f8f1bcd48c9a

Comment 27

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/adafd31af170

Comment 28

[Matt Wobensmith [:mwobensmith][:matt:]]

Marking [qa-] for verification.

Comment 29

[Justin Wood (:Callek)]

This was crashing on all platforms on me during transplant attempt --- so wontfix for SeaMonkey 2.26.1