AddressSanitizer: heap-buffer-overflow in mozilla::image::Deinterlacer::PropagatePassToDownscaler

RESOLVED FIXED in Firefox 44

Status

()

defect
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: bc, Assigned: eflores)

Tracking

(Blocks 1 bug, {csectype-bounds, regression, sec-critical})

43 Branch
mozilla46
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ wontfix, firefox44+ verified, firefox45+ verified, firefox46+ verified, firefox-esr38 unaffected, firefox-esr45 fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.5 affected, b2g-v2.2r unaffected, b2g-master fixed)

Details

(Whiteboard: [adv-main44+], )

Attachments

(4 attachments)

Reporter

Description

4 years ago
1. http://autojavier.cochesdeocasion.com/
2. AddressSanitizer: heap-buffer-overflow in mozilla::image::Deinterlacer::PropagatePassToDownscaler

Beta/43, Aurora/44, Nightly/45 at least.
This looks like it could be a regression from bug 1194058. Can you look into this, Seth? Thanks.
Flags: needinfo?(seth)
confirmed on windows 7 as assertion failure
Group: core-security → gfx-core-security
Keywords: regression
(In reply to Andrew McCreight [:mccr8] from comment #2)
> This looks like it could be a regression from bug 1194058. Can you look into
> this, Seth? Thanks.

mozregression agrees. Like bug 1231121, it sometimes required a couple force-refreshes to reliably reproduce for me. Also confirmed that this doesn't reproduce on Fx42 or ESR38.5.

FWIW, here's an Fx43 crash reporter link: https://crash-stats.mozilla.com/report/index/def60afa-2c9b-4e22-90cd-289852151215
Posted file asan_log.txt
Found this last night while fuzzing nightly.
See Also: → 1233651
Tracked for FF44 since it's a sec-high.
Looks like I missed a case in bug 1233465. Patch up soon.
Assignee: nobody → edwin
Status: NEW → ASSIGNED
Duplicate of this bug: 1233651
Bug 1233651 was marked sec-critical. Should we be reconsidering the rating in this bug? Also, would be great if we could eventually land a test for this bug.
Flags: in-testsuite?
sigh.
Flags: needinfo?(seth)
Attachment #8701081 - Flags: review?(seth)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #9)
> Bug 1233651 was marked sec-critical. Should we be reconsidering the rating
> in this bug?

Sure. Looks like this is a write, too.
Keywords: sec-highsec-critical
Too late for 43. Sorry. 
Just a reminder, looks like this should have sec approval before it lands.
Reporter

Comment 13

4 years ago
This is a critical security issue that has existed since September and is a common crash in reproduced in Bughunter which is being experienced by our users.

What will it take to get this 2 line patch reviewed and checked in?
Attachment #8701081 - Flags: review?(seth) → review+
Comment on attachment 8701081 [details] [diff] [review]
1233651-crash.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It would be reasonably easy to construct a crashing GIF, or write to immediately adjacent memory on the heap.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Perhaps we should take the word "bounds" out of the check-in comment...

Which older supported branches are affected by this flaw?
43+

If not all supported branches, which bug introduced the flaw?
bug 1194058

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Backporting should be trivial.

How likely is this patch to cause regressions; how much testing does it need?
It couldn't possibly make things worse. Worst case is it fails to make things better, but it certainly won't make them worse.
Attachment #8701081 - Flags: sec-approval?
Comment on attachment 8701081 [details] [diff] [review]
1233651-crash.patch

sec-approval+ for trunk.
Edwin, can you make and nominate patches for Aurora and Beta?
Attachment #8701081 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/df078713d748
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8701081 [details] [diff] [review]
1233651-crash.patch

Applies cleanly on Aurora and Beta.

Approval Request Comment
[Feature/regressing bug #]: bug 1194058
[User impact if declined]: crashing on malformed gifs
[Describe test coverage new/current, TreeHerder]: Fixes the crash for me locally.
[Risks and why]: Very little; this patch just makes some bounds smaller.
[String/UUID change made/needed]: None.
Attachment #8701081 - Flags: approval-mozilla-beta?
Attachment #8701081 - Flags: approval-mozilla-aurora?
Comment on attachment 8701081 [details] [diff] [review]
1233651-crash.patch

Taking it. Should be in 44 beta 6.
Attachment #8701081 - Flags: approval-mozilla-beta?
Attachment #8701081 - Flags: approval-mozilla-beta+
Attachment #8701081 - Flags: approval-mozilla-aurora?
Attachment #8701081 - Flags: approval-mozilla-aurora+
Group: gfx-core-security → core-security-release
This broke builds without skia because of the lack of a FrameSize method in the Downscaler class: http://hg.mozilla.org/releases/mozilla-beta/file/tip/image/Downscaler.h#l150
FrameSize is now in central in that file, added in bug 1235859. Edwin, would you mind doing an approval request for aurora/beta on that bug?
Flags: needinfo?(edwin)
Done.
Flags: needinfo?(edwin)
Reproduced the crash using Firefox 43.0.4 and Nightly 45.0a1 2015-12-05 under Win 7 (x64).

The crash no longer occurs on Firefox 44 beta 8, latest Aurora 45.0a2 and latest Nightly 46.0a1 2016-01-13 under Win 7 (x64), Ubuntu 14.04 (x64) and Mac OS X 10.9.5.

Still, there are a few crash reports in Socorro, after the fix landed, such as:
44 beta 7: https://crash-stats.mozilla.com/report/index/0946d7e3-a9a6-46b2-8946-d233f2160110
44 beta 8: https://crash-stats.mozilla.com/report/index/0f774792-734d-41eb-8002-7f0802160113
46.0a1 2016-01-11: https://crash-stats.mozilla.com/report/index/0830dfa8-38b1-473e-9406-051ea2160112
45.0a2 2016-01-12: https://crash-stats.mozilla.com/report/index/57c5ead7-d9e1-495a-896f-8e4312160113

Edwin, could you please see if those are related? Thank you
Flags: needinfo?(edwin)
(In reply to Petruta Rasa [QA] [:petruta] from comment #23)
> Still, there are a few crash reports in Socorro, after the fix landed, such
> as:
> 44 beta 7:
> https://crash-stats.mozilla.com/report/index/0946d7e3-a9a6-46b2-8946-
> d233f2160110
> 44 beta 8:
> https://crash-stats.mozilla.com/report/index/0f774792-734d-41eb-8002-
> 7f0802160113
> 46.0a1 2016-01-11:
> https://crash-stats.mozilla.com/report/index/0830dfa8-38b1-473e-9406-
> 051ea2160112
> 45.0a2 2016-01-12:
> https://crash-stats.mozilla.com/report/index/57c5ead7-d9e1-495a-896f-
> 8e4312160113
> 
> Edwin, could you please see if those are related? Thank you

Why are those crash reports thought to be related to this bug? They have a completely different stack in a different area of gecko.
(In reply to Timothy Nikkel (:tnikkel) from comment #24)
> Why are those crash reports thought to be related to this bug? They have a
> completely different stack in a different area of gecko.

They all appear for the same signature in crash-stats:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=nsTextFrame%3A%3AClearTextRun#tab-reports
Oh you must mean because of comment 4? It doesn't look like the crash in comment 4 is related to this bug though.
I've got the same signature https://crash-stats.mozilla.com/report/index/bp-cc85f4a8-09fc-422a-9e3d-3b69a2160114 while hard refreshing the http://autojavier.cochesdeocasion.com/ website with 43.0.4.

If those are not related, how could I check this bug is fixed? Thank you!
(In reply to Petruta Rasa [QA] [:petruta] from comment #27)
> I've got the same signature
> https://crash-stats.mozilla.com/report/index/bp-cc85f4a8-09fc-422a-9e3d-
> 3b69a2160114 while hard refreshing the
> http://autojavier.cochesdeocasion.com/ website with 43.0.4.
> 
> If those are not related, how could I check this bug is fixed? Thank you!

Hmm, that crash is either a different issue, or it's getting the wrong stack somehow, perhaps it's showing the main thread stack (which isn't crashing) at the time that a decoder thread crashes?

In any case if you crashed before, and no longer crash you can call this bug verified. If there are still crashes with that signature they are likely different issues.
As Timothy says, those are different so a new bug should be filed. Clearing ni?.
Flags: needinfo?(edwin)
Whiteboard: [adv-main44+]
Thanks!

Reproduced the crash also on Linux x64 asan build from 2015-12-02. It no longer occurs on newer asan builds.

Marking as verified fixed based on this and the above comments.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.