Closed Bug 862182 Opened 6 years ago Closed 6 years ago

Use refcounting when holding on to MediaResource* pointers

Categories

(Core :: Audio/Video, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox20 --- unaffected
firefox21 + wontfix
firefox22 + fixed
firefox23 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: Ehsan, Assigned: cpearce)

References

Details

(Keywords: csectype-uaf, regression, sec-critical, Whiteboard: [adv-main22+])

Attachments

(2 files, 7 obsolete files)

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...
Assignee: nobody → cpearce
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.
Attached patch Patch: refcount MediaResource (obsolete) — Splinter Review
* 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
Attachment #738736 - Flags: review?(roc)
Attachment #738736 - Flags: feedback?(ehsan)
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?
Attachment #738736 - Flags: feedback?(ehsan) → feedback-
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.
Attachment #738736 - Flags: review?(roc)
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.
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.
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?
Flags: needinfo?(jruderman)
Hopefully I can get it landed tomorrow.
Flags: needinfo?(jruderman) → needinfo?(cdiehl)
The JSEP fuzzer for WebRTC examines some of our media code by creating and using video elements and streams.
Flags: needinfo?(cdiehl)
Blocks: 862709
(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 ?
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
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.
Attachment #738736 - Attachment is obsolete: true
Attachment #742918 - Flags: review?(roc)
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.
Attachment #742919 - Flags: review?(chris.double)
Similar to previous patch, but for content/media/omx.

And I fixed some compiler warnings, to make this less painful to work on.
Attachment #742920 - Flags: review?(chris.double)
Attachment #742919 - Attachment description: Patch 2of3 v1: → Patch 2of3 v1: Refcount MediaResource in MPAPI plugin reader
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 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.
Attachment #738736 - Attachment is obsolete: false
Attachment #738736 - Flags: review?(roc)
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.
Attachment #742919 - Flags: review?(chris.double) → review+
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.
Attachment #742920 - Flags: review?(chris.double) → review+
With review comments addressed.
Attachment #738736 - Attachment is obsolete: true
Attachment #742918 - Attachment is obsolete: true
Attachment #742919 - Attachment is obsolete: true
Attachment #742920 - Attachment is obsolete: true
Attachment #738736 - Flags: review?(roc)
Attachment #742918 - Flags: review?(roc)
Attachment #743361 - Flags: review?(roc)
Review comments addressed. Just re-requesting review since there were a lot of changes.
Attachment #743362 - Flags: review?(chris.double)
Review comments addressed. Just re-requesting review since there were a lot of changes.
Attachment #743363 - Flags: review?(chris.double)
Attachment #743362 - Flags: review?(chris.double) → review+
Attachment #743363 - Flags: review?(chris.double) → review+
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.
Backed out for frequent orange in content/media/test/crashtests/459439-1.html

http://hg.mozilla.org/integration/mozilla-inbound/rev/6a0618f70be6
Because Windows has to be difficult too, the reftest bug686957.html was failing on Windows.
(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 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(...);
}
All three patches rolled into 1 for easier landing, with orange fixed.

https://tbpl.mozilla.org/?tree=Try&rev=e43682006591
Attachment #743361 - Attachment is obsolete: true
Attachment #743362 - Attachment is obsolete: true
Attachment #743363 - Attachment is obsolete: true
Attachment #744550 - Flags: review+
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.
Attachment #744550 - Flags: sec-approval?
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.
Attachment #744550 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/11cf235a84d6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
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?).
Flags: needinfo?(sworkman)
(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.
Flags: needinfo?(sworkman)
(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.
No longer blocks: 862709
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.
Flags: needinfo?(ehsan)
Attached patch Aurora PatchSplinter Review
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
Attachment #747145 - Flags: review+
Attachment #747145 - Flags: approval-mozilla-aurora?
Flags: needinfo?(ehsan)
This makes sense to me, FWIW.  Thanks, Chris!
Attachment #747145 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [adv-main22+]
Blocks: 799315
Group: core-security
You need to log in before you can comment on or make changes to this bug.