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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: AddressSanitizer: heap-buffer-overflow report

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

Beta/43, Aurora/44, Nightly/45 at least.

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind complaints

Comment 2

[Andrew McCreight [:mccr8]]

This looks like it could be a regression from bug 1194058. Can you look into this, Seth? Thanks.

Comment 3

[Carsten Book [:Tomcat]]

confirmed on windows 7 as assertion failure

Comment 4

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 5

[Tyson Smith [:tsmith]]

Attachment: asan_log.txt

Found this last night while fuzzing nightly.

Comment 6

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF44 since it's a sec-high.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Looks like I missed a case in bug 1233465. Patch up soon.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1233651-crash.patch

sigh.

Comment 11

[Andrew McCreight [:mccr8]]

(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.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 43. Sorry. 
Just a reminder, looks like this should have sec approval before it lands.

Comment 13

[Bob Clary [:bc] (inactive)]

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?

Comment 14

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 15

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

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?

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/df078713d748

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/df078713d748

Comment 18

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8701081 [details] [diff] [review]
1233651-crash.patch

Taking it. Should be in 44 beta 6.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5a51913d7daf
https://hg.mozilla.org/releases/mozilla-beta/rev/97c79bd9b8b9

Comment 21

[Mike Hommey [:glandium]]

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?

Comment 22

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Done.

Comment 23

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

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

Comment 24

[Timothy Nikkel (:tnikkel)]

(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.

Comment 25

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

(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

Comment 26

[Timothy Nikkel (:tnikkel)]

Oh you must mean because of comment 4? It doesn't look like the crash in comment 4 is related to this bug though.

Comment 27

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

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!

Comment 28

[Timothy Nikkel (:tnikkel)]

(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.

Comment 29

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

As Timothy says, those are different so a new bug should be filed. Clearing ni?.

Comment 30

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

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.