Closed Bug 1254731 Opened 4 years ago Closed 4 years ago

Crash [@ convertTimeToDate] due to a bug in libstagefright.

Categories

(Core :: Audio/Video: Playback, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 --- wontfix
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed
firefox-esr38 --- affected
firefox-esr45 --- affected

People

(Reporter: methos-bugzilla, Assigned: jya)

Details

(4 keywords, Whiteboard: [fuzzblocker][sg:dos])

Crash Data

Attachments

(4 files, 2 obsolete files)

The attached testcase crashes Firefox 48.0a1 due to a bug in libstagefright.

URL of build tested: https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-central-linux64-asan/1457435058/firefox-48.0a1.en-US.linux-x86_64-asan.tar.bz2
HG revision: mozilla-central 05c087337043dd8e71cc27bdb5b9d55fd00aaa26

This looks like a not exploitable null dereference. 

For detailed crash information, see attachment.
Attached video Testcase
Attachment #8728130 - Attachment mime type: application/octet-stream → video/mp4
The crash is a regression:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dc352a7bf234&tochange=0753f7b93ab7

Before that, Firefox wasn't able to read the video but didn't crash.

CR Win 7 FF48:
https://crash-stats.mozilla.com/report/index/b6360726-64a3-482b-9091-160ff2160309
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ convertTimeToDate] → [@ convertTimeToDate] [@ _invalid_parameter_noinfo | _Strftime_l ]
Ever confirmed: true
Flags: needinfo?(jyavenard)
I'm also hitting this in fuzzing now and it's happening very frequently. Marking as fuzzblocker.
Whiteboard: [fuzzblocker]
this is attempting to write as a date 4611686016344543104s from 1970 into a 32 bytes array declared on the stack.

it's not a simple null deref.
Group: core-security
Flags: needinfo?(jyavenard)
on mac it does crash weirdly. it's the libc code that crashes due to null deref

so maybe not a security issue. Without the source code of strftime it's hard to tell
here is the implementation of strftime on darwin.

https://opensource.apple.com/source/Libc/Libc-167/string.subproj/strftime.c

that's plain ugly, and not thread-safe at that as the maximum size of the buffer is stored in a static gsize
 and same for the buffer itself.
MozReview-Commit-ID: Cs33P9QyP2V
Attachment #8729506 - Flags: review?(gsquelart)
We just don't care about it in our use.
Additionally, gmtime and strftime are not thread safe at all (they use global static internally)

MozReview-Commit-ID: HfRpCyx4MpK
Attachment #8729507 - Flags: review?(gsquelart)
Assignee: nobody → jyavenard
Comment on attachment 8729506 [details] [diff] [review]
P1. Check for overflow and that conversion succeeded. r?gerald

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

r+, but please check the following:

::: media/libstagefright/frameworks/av/media/libstagefright/MPEG4Extractor.cpp
@@ +635,4 @@
>      if (time_1970 < 0) {
>          return false;
>      }
> +    if (time_1970 > std::numeric_limits<time_t>::max()) {

If 'time_t' is int64_t on some platform, then this test will always be false, which could be caught by eager compilers or checkers.
I would recommend at least a 'try' on all platforms to see if that's the case now, or you could change the test to '>='.
Alternatively, use a CheckedInt<time_t>.
Attachment #8729506 - Flags: review?(gsquelart) → review+
Hmmm, we might not want to do a public 'try' of your patch, as this bug may be a security issue.
If you wanted to test my theory about a 'test always false' complaint, you could do it in an anonymous patch somewhere else, so as not to attract attention to this code.
Upon further thought, I think here in the worse case we have a null deref as gmtime returns null. In the worse case the returned would be rubbish. So I think the security restriction can be lifted and make the bug public once again. 

I can change the > into a >= that will do with your compiler concern. 

In any case, I removed it all in P2. P1 was more like a technical exercise, one that could provided to google to fix it upstream. 
Could use gmtime_r instead, that one is thread safe.
Attachment #8729507 - Flags: review?(gsquelart) → review+
MozReview-Commit-ID: Cs33P9QyP2V
Attachment #8730403 - Flags: review+
Attachment #8729506 - Attachment is obsolete: true
Attachment #8729507 - Attachment is obsolete: true
We just don't care about it in our use.
Additionally, gmtime and strftime are not thread safe at all (they use global static internally)

MozReview-Commit-ID: HfRpCyx4MpK
Attachment #8730404 - Flags: review?(gsquelart)
Attachment #8730404 - Flags: review?(gsquelart) → review+
Comment on attachment 8730403 [details] [diff] [review]
P1. Check for overflow and that conversion succeeded.

[Security approval request comment]
How easily could an exploit be constructed based on the patch? can't see how. Applied the better safe than sorry approach. I believe the worse case would be a null deref (verified with OS X libc and GNU libc).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? ever since stagefright was included

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? Patch should apply as-is on all branches

How likely is this patch to cause regressions; how much testing does it need? none. it was a code path that we didn't use anyway.

P1 was designed to be passed on to Google upstream.
Attachment #8730403 - Flags: sec-approval?
This needs a security rating before sec-approval can be given as we only need sec-approval for high and critical rated issues. Is this a safe crash or not?
(In reply to Al Billings [:abillings] from comment #16)
> This needs a security rating before sec-approval can be given as we only
> need sec-approval for high and critical rated issues. Is this a safe crash
> or not?

I believe so, at least with the two libc I looked into.

gmtime returns null when the input is too big.
Group: core-security
Whiteboard: [fuzzblocker] → [fuzzblocker][sg:dos]
Attachment #8730403 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/621392c805bb
https://hg.mozilla.org/mozilla-central/rev/1bc37c8758c3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hi Dan, Jean-Yves, Al, should we consider uplifting these fixes to Aurora 47 as status-firefox47 is affected? Please let me know.
Flags: needinfo?(jyavenard)
Flags: needinfo?(dveditz)
Flags: needinfo?(abillings)
The same question as comment 20 but also for Beta 46. Liz, fyi.
Flags: needinfo?(lhenry)
Ritu, I don't have an opinion on uploading this or not. From a security point of view, comment 17 says there isn't really an issue.
Flags: needinfo?(abillings)
It's not a security issue, but it's a DOS. The crafted MP4 will cause Firefox to crash. So yes, I would uplift it. The patches should apply easily (including to 38)
Flags: needinfo?(jyavenard)
Seems like a decent stability win and safe enough (two patches kind of confuses the issue, but it's just removing a bit of code).
Flags: needinfo?(dveditz)
It seems ok for this to ride the trains with 47 or 48, not exploitable, and I don't think we know of any evidence that people are hitting this crash. That considered, it's pretty late in beta to take this. If you disagree and really want this in beta 9, let me know.
Flags: needinfo?(lhenry)
Hi Jean-Yves, based on all the replies so far I think we could uplift it. Please nominate the patch for uplift.
Flags: needinfo?(jyavenard)
For which version?
Comment on attachment 8730403 [details] [diff] [review]
P1. Check for overflow and that conversion succeeded.

Approval Request Comment
[Feature/regressing bug #]: 1254731
[User impact if declined]: Crash with specially crafted MP4
[Describe test coverage new/current, TreeHerder]: In central for a few weeks
[Risks and why]: none, removed unused code
[String/UUID change made/needed]: None
Flags: needinfo?(jyavenard)
Attachment #8730403 - Flags: approval-mozilla-beta?
Attachment #8730403 - Flags: approval-mozilla-aurora?
Comment on attachment 8730403 [details] [diff] [review]
P1. Check for overflow and that conversion succeeded.

OK for aurora uplift. Too late for beta though.
Attachment #8730403 - Flags: approval-mozilla-beta?
Attachment #8730403 - Flags: approval-mozilla-beta-
Attachment #8730403 - Flags: approval-mozilla-aurora?
Attachment #8730403 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.