Assertion failure: false [@mozilla::MP4TrackDemuxer::MP4TrackDemuxer]

RESOLVED FIXED in Firefox 42

Status

()

defect
P2
critical
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: tsmith, Assigned: gerald)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla44
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed, firefox44 fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [post-critsmash-triage][adv-main42+])

Attachments

(8 attachments, 12 obsolete attachments)

10.96 KB, text/plain
Details
1.65 KB, video/mp4
Details
8.05 KB, patch
gerald
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
26.20 KB, patch
gerald
: review+
Details | Diff | Splinter Review
11.63 KB, patch
gerald
: review+
Details | Diff | Splinter Review
11.29 KB, patch
gerald
: review+
Details | Diff | Splinter Review
7.72 KB, patch
gerald
: review+
Details | Diff | Splinter Review
8.19 KB, patch
gerald
: review+
Details | Diff | Splinter Review
Posted file call_stack.txt
Assertion failure: false, at /builds/slave/m-cen-l64-asan-d-ntly-00000000/build/src/dom/media/fmp4/MP4Demuxer.cpp:206
Posted video test_case.mp4
This assertion shows that reading a track failed, even though the parser previously said there was a track!

This should be handled earlier, either by trying to read the track before MP4TrackDemuxer is constructed in MP4Demuxer::GetTrackDemuxer; or (probably better if possible) in stagefright, to find out the correct number of *valid* tracks.

I'll have a look...
Assignee: nobody → gsquelart
Priority: -- → P2
Part 1: Moved test case contents from test source into separate file; easier to maintain and add to.
Attachment #8660683 - Flags: review?(giles)
Part 2: Go through test case files byte-by-byte (instead of every 4 bytes), to potentially catch more unhandled cases.
Attachment #8660684 - Flags: review?(giles)
Part 3: Test case filenames are now in a list, to allow adding more tests later on.
Attachment #8660685 - Flags: review?(giles)
Posted patch 1200326-p4-test-case.patch (obsolete) — Splinter Review
Part 4: Added test case from bug 1200326, with more detailed checks.
Attachment #8660687 - Flags: review?(giles)
Part 5: Moved fallible code from MP4TrackDemuxer constructor to caller MP4Demuxer::GetTrackDemuxer

In MP4TrackDemuxer constructor, getting the track info and indices could potentially fail, triggering crashing assertions.
The fallible work is now done before calling the constructor, and if it fails a nullptr is returned, which is correctly handled in MediaFormatReader.
Attachment #8660691 - Flags: review?(jyavenard)
Comment on attachment 8660683 [details] [diff] [review]
1200326-p1-open-test-file.patch

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

Thanks for adding a file reader. r=me with comments addressed.

Can you just ASSERT in the ReadTestFile helper? That seems easier than all the careful cleanup+nullptr code, and will deliver more accurate information on failure.

::: media/libstagefright/gtest/TestParser.cpp
@@ +67,5 @@
> +nsRefPtr<MediaByteBuffer>
> +ReadTestFile(const char* aFilename)
> +{
> +  char dir[1024];
> +  getcwd(dir, 1024);

Do you need this? Should be unnecessary.

@@ +71,5 @@
> +  getcwd(dir, 1024);
> +  if (!aFilename) {
> +    return nullptr;
> +  }
> +  FILE* f = fopen("test_case_1187067.mp4",//aFilename,

Don't hardcode the filename here, use aFilename instead.

@@ +82,5 @@
> +    fclose(f);
> +    return nullptr;
> +  }
> +  size_t len = ftell(f);
> +  if (len == 0 || len == size_t(EOF)) {

Do we know EOF is -1? I guess long and size_t are the same width on all architectures we care about.
Attachment #8660683 - Flags: review?(giles) → review+
Comment on attachment 8660684 [details] [diff] [review]
1200326-p2-skim-byte-by-byte.patch

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

Please split the commit message into multiple lines. This one's too long. E.g.:

Bug 1200326 - p2: Go through test case files byte-by-byte - r=rillian

Instead of every 4 bytes, to catch more unhandled cases.
Attachment #8660684 - Flags: review?(giles) → review+
Comment on attachment 8660691 [details] [diff] [review]
1200326-p5-remove-asserts-from-MP4TrackDemuxer-constructor.patch

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

r=me with comments addressed.

Additionally:
The core issue is MP4Metadata via buggy stagefright returns that a track exists when it doesn't.

I want to see that fixed in a related patch so this doesn't feel like a bandaid solution.

::: dom/media/fmp4/MP4Demuxer.h
@@ +10,5 @@
>  #include "mozilla/Maybe.h"
>  #include "mozilla/Monitor.h"
>  #include "MediaDataDemuxer.h"
>  #include "MediaResource.h"
> +#include "mp4_demuxer/Index.h"

The whole idea behind MP4Metadata etc ; was for abstracting as much as possible.
unfortunate if we had to load header once again

@@ +88,5 @@
>    void EnsureUpToDateIndex();
>    void SetNextKeyFrameTime();
>    nsRefPtr<MP4Demuxer> mParent;
> +  // We do not actually need a monitor, however MoofParser will assert
> +  // if a monitor isn't held.

please keep it as the last member ; the typical use is to declare the monitor just above the members it protects.
and here it protects none.

It's also irrelevant to this bug and as such out of scope.
Attachment #8660691 - Flags: review?(jyavenard) → review+
Attachment #8660685 - Flags: review?(giles) → review+
Comment on attachment 8660687 [details] [diff] [review]
1200326-p4-test-case.patch

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

Thanks for writing more tests!

::: media/libstagefright/gtest/TestParser.cpp
@@ +134,5 @@
>      EXPECT_FALSE(metadata.GetTrackInfo(TrackInfo::kUndefinedTrack, 0));
>      EXPECT_FALSE(metadata.GetTrackInfo(TrackInfo::kAudioTrack, 0));
> +    UniquePtr<TrackInfo> trackInfo = metadata.GetTrackInfo(TrackInfo::kVideoTrack, 0);
> +    EXPECT_TRUE(!!trackInfo);
> +    if (trackInfo) {

NB if you ASSERT_TRUE(!!trackInfo) instead you can skip the conditional, since the text can't proceed past the assert.
Attachment #8660687 - Flags: review?(giles) → review+
Updated as per review in comment 8.
Carrying r+ from comment 8.
Attachment #8660683 - Attachment is obsolete: true
Attachment #8664652 - Flags: review+
As well as reformating the patch comment as requested in comment 9, this patch contains another improvement to the way 'skimming' tests are conducted, with significant code changes (compared to the very simple previous part-2 patch), hence the new r?.

Part 2: Parse subsets of the input stream.
In addition to starting parsing at different points across the input stream,
different sizes are given, from the minimum step size up to the remainder of the stream.
Some shortcuts ensure that we don't do N^2 tests, but the minimal amount needed to cover all useful cases.
Attachment #8660684 - Attachment is obsolete: true
Attachment #8664653 - Flags: review?(giles)
Rebase.
Carrying r+ from before comment 11.
Attachment #8660685 - Attachment is obsolete: true
Attachment #8664655 - Flags: review+
Posted patch 1200326-p4-test-case.patch v2 (obsolete) — Splinter Review
Rebase.
Carrying r+ from comment 11.

(In reply to Ralph Giles (:rillian) from comment #11)
> Comment on attachment 8660687 [details] [diff] [review]
> 1200326-p4-test-case.patch
> 
> Review of attachment 8660687 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for writing more tests!
> 
> ::: media/libstagefright/gtest/TestParser.cpp
> @@ +134,5 @@
> >      EXPECT_FALSE(metadata.GetTrackInfo(TrackInfo::kUndefinedTrack, 0));
> >      EXPECT_FALSE(metadata.GetTrackInfo(TrackInfo::kAudioTrack, 0));
> > +    UniquePtr<TrackInfo> trackInfo = metadata.GetTrackInfo(TrackInfo::kVideoTrack, 0);
> > +    EXPECT_TRUE(!!trackInfo);
> > +    if (trackInfo) {
> 
> NB if you ASSERT_TRUE(!!trackInfo) instead you can skip the conditional,
> since the text can't proceed past the assert.

That was intentional: ASSERT would stop the test completely, without testing more files. I think it's best to continue, to see if other files would experience the same issue.
Attachment #8660687 - Attachment is obsolete: true
Attachment #8664658 - Flags: review+
Rebase and small member reshuffle.
Carrying r+ from comment 10.

(In reply to Jean-Yves Avenard [:jya] from comment #10)
> Comment on attachment 8660691 [details] [diff] [review]
> 1200326-p5-remove-asserts-from-MP4TrackDemuxer-constructor.patch
> 
> Review of attachment 8660691 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with comments addressed.
> 
> Additionally:
> The core issue is MP4Metadata via buggy stagefright returns that a track
> exists when it doesn't.
> 
> I want to see that fixed in a related patch so this doesn't feel like a
> bandaid solution.

The next patch will address this concern.

> ::: dom/media/fmp4/MP4Demuxer.h
> @@ +14,1 @@
> > +#include "mp4_demuxer/Index.h"
> 
> The whole idea behind MP4Metadata etc ; was for abstracting as much as
> possible.
> unfortunate if we had to load header once again

A solution (to me) would have been to move MP4TrackDemuxer (which is only used internally by MP4Demuxer) to the cpp, so the ugly #include could be there too. After discussion with Jean-Yves, we have decided to keep things as-is.
A future bug could instead unite index types between Stagefright foundation and bindings; will file shortly.

> @@ +88,5 @@
> >    void EnsureUpToDateIndex();
> >    void SetNextKeyFrameTime();
> >    nsRefPtr<MP4Demuxer> mParent;
> > +  // We do not actually need a monitor, however MoofParser will assert
> > +  // if a monitor isn't held.
> 
> please keep it as the last member ; the typical use is to declare the
> monitor just above the members it protects.
> and here it protects none.
> 
> It's also irrelevant to this bug and as such out of scope.

It was necessary to clean up MP4TrackDemuxer members initialization. I've clarified that in the comment.
Attachment #8660691 - Attachment is obsolete: true
Attachment #8664672 - Flags: review+
As threatened in comment 16, this patch addresses the root cause of the reported assertion:

Part 6: Precheck MP4Metadata to avoid runtime surprises.

MP4Metadata::GetNumberTracks, GetTrackInfo and ReadTrackIndex could return
inconsistent results because they do different checks before returning their
answers.
Instead, we now precheck and check as much as possible, so that results from
one method (e.g. GetNumberTracks) can be trusted to carry on with the next
(e.g. GetTrackInfo for all expected tracks).

Note that ReadTrackInfo may still fail because of memory issues, so its return
should be properly checked (see part-5 patch).
Attachment #8664675 - Flags: review?(jyavenard)
Part 7: While debugging for this bug, I encountered one compiler error when stagefright's debugging is set to verbose.
Attachment #8664676 - Flags: review?(jyavenard)
And another small fix for a nearby compiler warning, and a typo in a log message.
Attachment #8664677 - Flags: review?(jyavenard)
patch 1 to 5 should have been pushed back then and those following in a new bug :(
Attachment #8664676 - Flags: review?(jyavenard) → review+
Comment on attachment 8664677 [details] [diff] [review]
1200326-p8-fix-warning-and-typo.patch

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

fwiw, we can no longer enter here if mFirstTrack is true ; this is a remnant of when the MP4Reader would do its Block & Retry business and could have called multiple times readMetadata() in a row.
We had to ensure that it wouldn't attempt to re-read the metadata unnecessarily.

the sadist in me think that it should be two different patch, one for changing the stagefright comment and making us fork even further and one to fix the initialization
Attachment #8664677 - Flags: review?(jyavenard) → review+
Comment on attachment 8664675 [details] [diff] [review]
1200326-p6-precheck-MP4Metadata.patch

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

Could you put that into another bug (or I can do it) and we can follow it up there. patch 1-5 are safe to push and can't introduce regression.

I don't have the same level of confidence with this one, and I believe 1-5 should be uplifted.

::: media/libstagefright/binding/MP4Metadata.cpp
@@ +25,5 @@
>      : mCanSeek(false) {}
>    sp<MediaExtractor> mMetadataExtractor;
> +  // If we don't have local data yet, try and read it.
> +  // Return true if there is at least one track.
> +  bool gotCachedData();

not moz style name
Attachment #8664675 - Flags: review?(jyavenard)
Blocks: 1207909
Comment on attachment 8664675 [details] [diff] [review]
1200326-p6-precheck-MP4Metadata.patch

Moved to bug 1207909.
Attachment #8664675 - Attachment is obsolete: true
Comment on attachment 8664676 [details] [diff] [review]
1200326-p7-fix-MPEG4Extractor-log.patch

Moved to bug 1207909.
Attachment #8664676 - Attachment is obsolete: true
Comment on attachment 8664677 [details] [diff] [review]
1200326-p8-fix-warning-and-typo.patch

Moved to bug 1207909.
Attachment #8664677 - Attachment is obsolete: true
Comment on attachment 8664653 [details] [diff] [review]
1200326-p2-using-own-teststream.patch v2

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

The Stream api here isn't great. I wish the code were better, but that's really the fault of the baseclass. Anyway, does what it says. r=me with comments addressed.

::: media/libstagefright/gtest/TestParser.cpp
@@ +24,5 @@
> +  }
> +  bool ReadAt(int64_t aOffset, void* aData, size_t aLength,
> +              size_t* aBytesRead) override
> +  {
> +    if (aOffset < 0 || size_t(aOffset) > mSize) {

You also need to check aOffset <= SIZE_MAX for 32 bit before you construct it as a size_t.

@@ +28,5 @@
> +    if (aOffset < 0 || size_t(aOffset) > mSize) {
> +      return false;
> +    }
> +    *aBytesRead =
> +      std::min(aLength, size_t(mSize - aOffset));

Is the size_t ctor necessary? aLength and mSize are already size_t, so I'd explect aOffset to get promoted. Not on 32-bit I guess?

@@ +31,5 @@
> +    *aBytesRead =
> +      std::min(aLength, size_t(mSize - aOffset));
> +    if (mHighestSuccessfulEndOffset < aOffset + int64_t(*aBytesRead))
> +    {
> +      mHighestSuccessfulEndOffset = aOffset + int64_t(*aBytesRead);

I had to stare at this for a long time to convince myself it wouldn't overflow. Maybe it would be better with CheckedInt<size_t> everywhere?

@@ +181,5 @@
>  TEST(stagefright_MPEG4Metadata, test_case_mp4_skimming)
>  {
> +  static const size_t step = 1u;
> +  nsTArray<uint8_t> buffer = ReadTestFile("test_case_1187067.mp4");
> +  ASSERT_TRUE(!buffer.IsEmpty());

ASSERT_FALSE(buffer.IsEmpty());

@@ +184,5 @@
> +  nsTArray<uint8_t> buffer = ReadTestFile("test_case_1187067.mp4");
> +  ASSERT_TRUE(!buffer.IsEmpty());
> +  // Just exercizing the parser starting at different points through the file,
> +  // making sure it doesn't crash.
> +  // No checks because results would differ for each position.

ASSERT_LE(step, buffer.Length()); or
ASSERT_TRUE(step <= buffer.Length());

@@ +187,5 @@
> +  // making sure it doesn't crash.
> +  // No checks because results would differ for each position.
> +  for (size_t offset = 0; offset < buffer.Length() - step; offset += step) {
> +    size_t size = buffer.Length() - offset;
> +    while (size != 0) {

(size > 0) is less error-prone if the type changes to a signed value.

@@ +214,5 @@
>  
>  TEST(stagefright_MoofParser, test_case_mp4)
>  {
> +  nsTArray<uint8_t> buffer = ReadTestFile("test_case_1187067.mp4");
> +  ASSERT_TRUE(!buffer.IsEmpty());

ASSERT_FALSE(buffer.IsEmpty());

@@ +242,5 @@
>  TEST(stagefright_MoofParser, test_case_mp4_skimming)
>  {
> +  const size_t step = 1u;
> +  nsTArray<uint8_t> buffer = ReadTestFile("test_case_1187067.mp4");
> +  ASSERT_TRUE(!buffer.IsEmpty());

ASSERT_FALSE

@@ +247,5 @@
>    Monitor monitor("MP4Metadata::HasCompleteMetadata");
>    MonitorAutoLock mon(monitor);
> +  // Just exercizing the parser starting at different points through the file,
> +  // making sure it doesn't crash.
> +  // No checks because results would differ for each position.

ASSERT_LE(step, buffer.Length());
Attachment #8664653 - Flags: review?(giles) → review+
Rebase. Carrying r+ from comment 8.
Attachment #8664652 - Attachment is obsolete: true
Attachment #8666339 - Flags: review+
Addressed review comments in comment 26.
Carrying r+ from comment 26.
Attachment #8664653 - Attachment is obsolete: true
Attachment #8666340 - Flags: review+
Rebase.
Carrying r+ from comment 11.
Attachment #8664655 - Attachment is obsolete: true
Attachment #8666341 - Flags: review+
Rebase.
Carrying r+ from comment 11.
Attachment #8664658 - Attachment is obsolete: true
Attachment #8666342 - Flags: review+
this needs to be uplifted to beta as when not asserting, this could cause undesired behaviour.
Depends on: 1207441
Group: core-security
Severity: normal → critical
Comment on attachment 8664672 [details] [diff] [review]
1200326-p5-remove-asserts-from-MP4TrackDemuxer-constructor.patch

Approval Request Comment
[Feature/regressing bug #]: 1156689
[User impact if declined]: Crash, potentially out of bound reads (how exploitable that is, is dubious)
[Describe test coverage new/current, TreeHerder]: in central, 
[Risks and why]: Low, moving logic from constructor to place creating object
[String/UUID change made/needed]: None
Attachment #8664672 - Flags: approval-mozilla-beta?
Attachment #8664672 - Flags: approval-mozilla-aurora?
Comment on attachment 8664672 [details] [diff] [review]
1200326-p5-remove-asserts-from-MP4TrackDemuxer-constructor.patch

Fix a crash, taking it.
Should be in 42 beta 3.
Attachment #8664672 - Flags: approval-mozilla-beta?
Attachment #8664672 - Flags: approval-mozilla-beta+
Attachment #8664672 - Flags: approval-mozilla-aurora?
Attachment #8664672 - Flags: approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #36)
> Comment on attachment 8664672 [details] [diff] [review]
> 1200326-p5-remove-asserts-from-MP4TrackDemuxer-constructor.patch
> 
> Fix a crash, taking it.
> Should be in 42 beta 3.

Thank you.
Please only uplift part 5.
The existing patch in attachment 8664672 [details] [diff] [review] applies to Aurora.

The patch here is a rebase for Beta. (Carrying r+ from comment 10.)
Attachment #8667146 - Flags: review+
Hi, Gerald, seems the beta patch failed to apply:

patching file dom/media/fmp4/MP4Demuxer.cpp
Hunk #1 FAILED at 3
Hunk #2 FAILED at 130
Hunk #3 FAILED at 184
3 out of 3 hunks FAILED -- saving rejects to file dom/media/fmp4/MP4Demuxer.cpp.rej
patching file dom/media/fmp4/MP4Demuxer.h
Hunk #1 FAILED at 7
Hunk #2 FAILED at 50
2 out of 2 hunks FAILED -- saving rejects to file dom/media/fmp4/MP4Demuxer.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 1200326-p5-remove-asserts-from-MP4TrackDemuxer-constructor-beta.patch

could you take a look, thanks!
Flags: needinfo?(gsquelart)
Group: core-security → media-core-security
Group: media-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
This should have had a security rating and sec-approval+ before checkin. Can you set sec-approval? and answer the template questions, please?

https://wiki.mozilla.org/Security/Bug_Approval_Process

Did this affect 41? I'm going to assume it did (but not ESR38) since bug 1156689 was fixed in 40.
Flags: needinfo?(gsquelart)
Comment on attachment 8664672 [details] [diff] [review]
1200326-p5-remove-asserts-from-MP4TrackDemuxer-constructor.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
If 'mInfo' is null, |mInfo->mTrackId| will either crash or read some random number near 0x0.
'ReadTrackIndex' handles random track IDs correctly, in the worst case it would read the wrong track, but it won't read outside of the media data.
And if ReadTrackIndex fails (wrong track, or OOM), the indices array will be empty, so mIndex will be empty and harmless.
So I don't see a way to exploit this.
Please have an expert verify this.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The check-in comment points at reading invalid track info/indices, the code shows which debug-only assertions have been changed into regular tests.
Tests (in another patch) shows a sample file that triggers a read at 0x0.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
This code was introduced in bug 1159027, released in 41.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Already backported to 42, it would be trivial to port to 41 with low risks.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely: Instead of crashes we'll now have failing playback (as expected with invalid files). Already tested with targeted gtest, and lots of media tests.
Flags: needinfo?(gsquelart)
Attachment #8664672 - Flags: sec-approval?
Attachment #8664672 - Flags: sec-approval? → sec-approval+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.