862182 - Use refcounting when holding on to MediaResource* pointers

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[(no longer active)]

This is a follow-up from bug 854319.  I tried to make ~MediaResource protected to avoid using things like nsAutoPtr with it, and then discovered that MediaDecoder::OpenResource tried to delete a raw MediaResource*.  On a cursory look, it doesn't seem like many of the MediaResource* users are using the refcounting ownership model.

Filing this as security sensitive since it might be possible to construct use-after-free bugs out of this...

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is my fault, when I switched MediaResource to being refcounted in Bug 836927 I didn't switch everywhere properly. :(

We'll want the fix for this everywhere that Bug 836927's patches are. That's on Aurora and Beta.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: refcount MediaResource

* Refcount MediaResource when it's created and when references are stored during it's startup, before the reference is passed off to the decoder to own.

Looks greenish on Try, modulo lots of random but already filed oranges:
https://tbpl.mozilla.org/?tree=Try&rev=1e540a71c875

Comment 3

[(no longer active)]

Comment on attachment 738736 [details] [diff] [review]
Patch: refcount MediaResource

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

::: content/html/content/src/HTMLMediaElement.cpp
@@ +2349,5 @@
>      decoder->SetTransportSeekable(aOriginal->IsTransportSeekable());
>      decoder->SetMediaSeekable(aOriginal->IsMediaSeekable());
>    }
>  
> +  nsRefPtr<MediaResource> resource = originalResource->CloneData(decoder);

Same here, you want to make both CloneData and Create below return already_AddRefed, otherwise the old version of these hunks would happily compile, and blow up on us.

::: content/media/MediaDecoder.h
@@ +300,5 @@
> +  // so it's OK to use the reference returned by this function without
> +  // refcounting, *unless* you need to store and use the reference after the
> +  // MediaDecoder has been destroyed. You might need to do this if you're
> +  // wrapping the MediaResource in some kind of byte stream interface to be
> +  // passed to a platform decoder.

This is just asking for trouble, I'm afraid.  Why not make this already_AddRefed?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 738736 [details] [diff] [review]
Patch: refcount MediaResource

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

::: content/media/MediaDecoder.h
@@ +300,5 @@
> +  // so it's OK to use the reference returned by this function without
> +  // refcounting, *unless* you need to store and use the reference after the
> +  // MediaDecoder has been destroyed. You might need to do this if you're
> +  // wrapping the MediaResource in some kind of byte stream interface to be
> +  // passed to a platform decoder.

Sigh... Yeah, you're right.

Comment 5

[Daniel Veditz [:dveditz]]

Since this regressed during the Fx21 cycle it'd be nice to get a fix in before we ship. We've only got a couple betas left.

Comment 6

[Alex Keybl [:akeybl]]

Any hope of this still landing today or tomorrow? If not, we should start discussing the discoverability of this security issue so that we can determine whether the risk is appropriate for one of our final two betas.

Comment 7

[(no longer active)]

I would expect these bugs to be discoverable by fuzzing this code a bit...  Or, by just reading the code, which is how I discovered it.

Jesse, do we have fuzzers which examine our media code?

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Hopefully I can get it landed tomorrow.

Comment 9

[Christoph Diehl [:posidron]]

The JSEP fuzzer for WebRTC examines some of our media code by creating and using video elements and streams.

Comment 10

[bhavana bajaj [:bajaj]]

(In reply to Chris Pearce (:cpearce) from comment #8)
> Hopefully I can get it landed tomorrow.

Hey Chris, we will be going to build with our second last beta, on Tuesday ~afternoon PT . Are we planning to resolve this on m-c soon and get it landed on branches based on risk by then ?

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have been wrestling with getting this to work on android and B2G. It's not a small patch, and B2G <video> was already broken which threw off my testing. I have a patch on Try at the moment, so it should be up for review by tomorrow:

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

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1of3 v1: Hold MediaResource references in nsRefPtr

Looks like this is going green on try.

Hold references to MediaResource in nsRefPtrs.

Also removes MediaStream** parameter from MediaCacheStream::AreAllStreamsForResourceSuspended(), since it's not used, and there's not much point converting it to being refcounted compatible if we're not even using it.

This doesn't include the content/media/plugins or content/media/omx changes, they're in separate patches.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2of3 v1: Refcount MediaResource in MPAPI plugin reader

Make content/media/plugins store references to MediaResource in nsRefPtr.
MPAPI::Decoder also refcounts it's reference to MediaResource, and I fixed a leak; we weren't freeing the Decoder object in MediaPluginHost::CreateDecoder() if no compatible decoder was found.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3of3 v1: Refcount MediaResource in omx decoder

Similar to previous patch, but for content/media/omx.

And I fixed some compiler warnings, to make this less painful to work on.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 742918 [details] [diff] [review]
Patch 1of3 v1: Hold MediaResource references in nsRefPtr

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

This is OK.

However, I partially disagree with Ehsan. I think MediaDecoder::GetResource should return a MediaResource* and we should leave it up to the caller to ensure they hold a reference to the MediaDecoder at least as long as any non-addrefed MediaResource*. Otherwise we're injecting a lot of AddRefs and Releases of MediaResource here and those are expensive since it's a thread-safe refcount. It also adds some code that's annoying to read. We use this pattern (returning T* from a getter method and expecting the caller to keep the underlying object alive) a lot in many places already.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 738736 [details] [diff] [review]
Patch: refcount MediaResource

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #15)
> Comment on attachment 742918 [details] [diff] [review]
> Patch 1of3 v1: Hold MediaResource references in nsRefPtr
> 
> Review of attachment 742918 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is OK.
> 
> However, I partially disagree with Ehsan. I think MediaDecoder::GetResource
> should return a MediaResource* and we should leave it up to the caller to
> ensure they hold a reference to the MediaDecoder at least as long as any
> non-addrefed MediaResource*.

In that case, please review this earlier patch.

Comment 17

[cajbir (:cajbir)]

Comment on attachment 742919 [details] [diff] [review]
Patch 2of3 v1: Refcount MediaResource in MPAPI plugin reader

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

Probably need to make the same MPAPI.h/MediaPluginHost changes in the equivalent, duplicated stuff, in content/media/omx. 

r+ with those.

::: content/media/plugins/MPAPI.h
@@ +122,5 @@
>    void *mResource;
>    void *mPrivate;
>  
>    Decoder();
> +  ~Decoder();

The mResource casting/decrefing here should be done in MediaPluginHost::DestroyDecoder and manually deal with handling the failure case in CreateDecoder to keep Decoder mResource independent.

::: content/media/plugins/MediaPluginReader.cpp
@@ +171,5 @@
>        return true;
>  
>      currentImage = bufferCallback.GetImage();
> +    nsRefPtr<MediaResource> resource(mDecoder->GetResource());
> +    int64_t pos = resource->Tell();

Don't need to hold a reference here I think.

@@ +289,5 @@
>    NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>  
>    // This is the approximate byte position in the stream.
> +  nsRefPtr<MediaResource> resource(mDecoder->GetResource());
> +  int64_t pos = resource->Tell();

Don't need to hold a reference here I think.

Comment 18

[cajbir (:cajbir)]

Comment on attachment 742920 [details] [diff] [review]
Patch 3of3 v1: Refcount MediaResource in omx decoder

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

r+ with the GetResource() null return fix.

::: content/media/omx/MediaOmxReader.cpp
@@ +165,5 @@
>      }
>  
>      // This is the approximate byte position in the stream.
> +    nsRefPtr<MediaResource> resource(mDecoder->GetResource());
> +    int64_t pos = resource->Tell();

Don't need to hold reference here.

@@ +235,5 @@
>    NS_ASSERTION(mDecoder->OnDecodeThread(), "Should be on decode thread.");
>  
>    // This is the approximate byte position in the stream.
> +  nsRefPtr<MediaResource> resource(mDecoder->GetResource());
> +  int64_t pos = resource->Tell();

Or here.

@@ +300,5 @@
>    if (!durationUs)
>      return NS_OK;
>  
>    // Special case completely cached files.  This also handles local files.
> +  nsRefPtr<MediaResource> stream(mDecoder->GetResource());

Or here.

::: content/media/omx/OmxDecoder.h
@@ +172,5 @@
>    bool ReadAudio(AudioFrame *aFrame, int64_t aSeekTimeUs);
>  
> +  already_AddRefed<MediaResource> GetResource() {
> +    nsRefPtr<MediaResource> resource;
> +    return resource.forget();

Isn't this returning a null resource? Nothing is assigned to the local 'resource'. I don't understand why GetResource() returns an 'already addrefed' with a nsRefPtr/forget in the body. Why not just return mResource, with the result being an nsRefPtr? What you are are doing may be a common idiom I don't know so ignore if it is.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1of3 v2: Hold MediaResource references in nsRefPtr

With review comments addressed.

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2of3 v2: Refcount MediaResource in plugin decoder

Review comments addressed. Just re-requesting review since there were a lot of changes.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3of3 v2: Refcount MediaResource in omx decoder

Review comments addressed. Just re-requesting review since there were a lot of changes.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Greenish on Try:
https://tbpl.mozilla.org/?tree=Try&rev=a15567933635

Comment 23

[bhavana bajaj [:bajaj]]

We should be mindful of the risk of uplift this patch may have as we are in our final two beta's.Explaining the risk here explicitly & considering if its appropriate for our final beta's here when nominating for sec-approval will be helpful.

If this is something that is not easy discoverable we should discuss about landing this in the next cycle if appropriate & keep that in mind before landing on inbound.

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/50df37d5f768
https://hg.mozilla.org/integration/mozilla-inbound/rev/64d208186331
https://hg.mozilla.org/integration/mozilla-inbound/rev/5378db8362f5

I don't think this is easily exploitable, but we should get it on Aurora at least just in case.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Backed out for frequent orange in content/media/test/crashtests/459439-1.html

http://hg.mozilla.org/integration/mozilla-inbound/rev/6a0618f70be6

Comment 26

[Phil Ringnalda (:philor)]

Because Windows has to be difficult too, the reftest bug686957.html was failing on Windows.

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Chris Pearce (:cpearce) from comment #25)
> Backed out for frequent orange in content/media/test/crashtests/459439-1.html
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/6a0618f70be6

Chris, can we please make sure to request sec-approval on this before landed per https://wiki.mozilla.org/Security/Bug_Approval_Process ? Given we are close to release for current beta & considering this may not be exploitable, we may need to delay the landing on trunk/branches.Al or Dan can help with more details as well once you nominate the patch for sec-approval.Thanks !

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 743361 [details] [diff] [review]
Patch 1of3 v2: Hold MediaResource references in nsRefPtr

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

::: content/media/MediaResource.cpp
@@ +1611,1 @@
>    }

Here is the cause of the orange, this should be:

if (fc || IsBlobURI(uri)) {
  resource = new FileMediaResource(...);
} else {
  resource = new ChannelMediaResource(...);
}

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: With orange fixed

All three patches rolled into 1 for easier landing, with orange fixed.

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

Comment 30

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 744550 [details] [diff] [review]
Patch: With orange fixed

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
-> I don't think it's easy. I think only the Windows MP3/MP4 decoder backend is vulnerable, and the patch doesn't make it obvious what area of code is vulnerable.


Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
-> It's pretty obvious what is being fixed, but I don't think it's obvious how to exploit the problem.


Which older supported branches are affected by this flaw?
-> I think there is a possibility of use after free, but I think only for Windows builds that support MP3/MP4 playback, so current Beta and Aurora. MP3/MP4 on Windows is in Release, but it's preffed off, so I don't think we need to worry about that.


If not all supported branches, which bug introduced the flaw?
-> 799315. This is in release branch, but preffed off, preffed on in Beta.


Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
-> Not yet, but they should be easy to make, the code being patched is pretty stable. The beta/aurora patches shouldn't be any more risky than this patch.


How likely is this patch to cause regressions; how much testing does it need?
-> I think this patch is low risk. But I wouldn't want to land it on Beta just before uplift, just in case, the patch touches resource management of a central data structure in the media decoder infrastructure.

Comment 31

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

Comment on attachment 744550 [details] [diff] [review]
Patch: With orange fixed

sec-approval+ for M-C. Nominate for Beta and Aurora. We're pretty close to release so I'm not sure that it will be approved there but I want to take this on central and Aurora this cycle.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11cf235a84d6

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/11cf235a84d6

Comment 34

[Chris Pearce [:cpearce (Not reading bugmail)]]

Steve: I'd like to uplift my patch here to properly refcount MediaResource to Aurora, but Aurora is still building with DASH support and so my patch has errors building in content/media/dash... Rather than me updating and testing my patch to support code that we're turning off, do you mind if we just disable DASH in the build on Aurora too? We don't need to pull in all your patch from bug 855064, we can just take your configure.in changes from attachment 732076 [details] [diff] [review] (right?).

Comment 35

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Chris Pearce (:cpearce) from comment #34)
> do you mind if we
> just disable DASH in the build on Aurora too?

No problem at all - block it out whenever you need from now on.

> We don't need to pull in all
> your patch from bug 855064, we can just take your configure.in changes from
> attachment 732076 [details] [diff] [review] (right?).

Not so sure about that. I'll try building it now myself with the configure.in changeset, but I seem to remember things failing on try because of some issue with tests. Unless there were any regressions caused by those patches in bug 855064, I'd advise getting that approved rather than spending time determining the minimum patch required. I'll comment on here as soon as I get build results.

Comment 36

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Steve Workman [:sworkman] from comment #35)
> Not so sure about that. I'll try building it now myself with the
> configure.in changeset, but I seem to remember things failing on try because
> of some issue with tests.

I was right, but removing DASH from tests should be ultra low risk. There is a separate patch in bug 855064 (attachment 732394 [details] [diff] [review] [diff] [review]) which removes DASH references in mochitests. (Note, a minor change to MediaResource slipped in there as well - sorry, bad patch cleanup on my part).

I have pushed this patch (without the MediaResource changes) and a minimal RemoveDASHFromCode patch (configure.in and a required change in WebMReader.cpp) to try for a full unit test run:
  https://tbpl.mozilla.org/?tree=Try&rev=74bee570a073

> Unless there were any regressions caused by those
> patches in bug 855064, I'd advise getting that approved rather than spending
> time determining the minimum patch required.

If the try run is successful, I'd now suggest going with the patches pushed in that job. Removing DASH from tests is low risk, and minimally removing DASH from Gecko is probably lower risk than taking the full patch from attachment 732076 [details] [diff] [review]. Up to you how you want to proceed. I'll not pollute the patch pool in this bug by uploading those patches until I hear you want them. Alternatively you can get them from the try run yourself.

Comment 37

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ehsan: Can we uplift Bug 854319 part 2 (i.e. changeset ee39d8eb931f) to Aurora? My patch here doesn't compile on Aurora without that change, and I think we should uplift my changes here.

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Aurora Patch

This patch is a rollup of my patch plus the other patches cherrypicked from m-c that need to be applied for my patch to build/work (sworkman's changesets from comment 36 above, and Ehsan's changeset ee39d8eb931f).

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 799315, H.264 playback on Windows.
User impact if declined: Potential for use after free
Testing completed (on m-c, etc.): Every change here has been on m-c for at least a week.
Risk to taking this patch (and alternatives if risky): Pretty low, no regressions have shown up on m-c.
String or IDL/UUID changes made by this patch: None.


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

Comment 39

[(no longer active)]

This makes sense to me, FWIW.  Thanks, Chris!

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2a1e083e069