1249578 - Assertion failure: mTargetSize.height <= aOriginalSize.height (Created a downscaler, but height is larger), at Downscaler.cpp:71

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Carsten Book [:Tomcat]]

Found via bughunter and reproduced on win 7 debug based on todays m-c tip

-> Steps to reproduce: 
--> Load https://duckduckgo.com/?q=erol%2Bsahin%2Bkovan

marking as s-s but feel free to reopen

Assertion failure: mTargetSize.height <= aOriginalSize.height (Created a downscaler, but height is larger), at c:/Users/mozilla/debug-builds/mozilla-central/image/Downscaler.cpp:71
#01: mozilla::image::nsBMPDecoder::ReadBitfields[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d7b12c]
#02: mozilla::Maybe<enum mozilla::image::TerminalState>::operator*[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d740a3]
#03: mozilla::image::StreamingLexer<enum mozilla::image::nsBMPDecoder::State,16>::Lex<<lambda_4a20370f034d9119c083ef6418dfb377> >[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d6ca6b]
#04: mozilla::image::nsBMPDecoder::WriteInternal[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d7f098]
#05: mozilla::image::Decoder::Write[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d495f9]
#06: mozilla::image::nsICODecoder::WriteToContainedDecoder[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d827d6]
#07: mozilla::image::nsICODecoder::ReadBIH[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d7ab7c]
#08: mozilla::Maybe<enum mozilla::image::TerminalState>::operator*[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d74331]
#09: mozilla::image::StreamingLexer<enum mozilla::image::ICOState,32>::Lex<<lambda_df03cf4431f66e20e95916fe478c86e5> >[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d6d7bb]
#10: mozilla::image::nsICODecoder::WriteInternal[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d81118]
#11: mozilla::image::Decoder::Write[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d495f9]
#12: mozilla::image::Decoder::Decode[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d3c0b8]
#13: mozilla::image::DecodePool::Decode[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d3bdf4]
#14: mozilla::image::DecodePoolWorker::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x1d4776c]
#15: nsThread::ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x5c32f7]
#16: NS_ProcessNextEvent[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x62af52]
#17: mozilla::ipc::MessagePumpForNonMainThreads::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xc02ef2]
#18: MessageLoop::RunInternal[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xb8939d]
#19: MessageLoop::RunHandler[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xb89312]
#20: MessageLoop::Run[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0xb88eed]
#21: nsThread::ThreadFunc[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\xul.dll +0x5cdedf]
#22: _PR_NativeRunThread[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2fe4cb]
#23: _PR_MD_YIELD[c:\Users\mozilla\debug-builds\mozilla-central\firefox-debug\dist\bin\nss3.dll +0x2e3889]
#24: _get_flsindex[C:\Windows\system32\MSVCR120.dll +0x2c01d]
#25: _get_flsindex[C:\Windows\system32\MSVCR120.dll +0x2c001]
#26: BaseThreadInitThunk[C:\Windows\system32\kernel32.dll +0x4ef1c]
#27: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x63b53]
#28: RtlInitializeExceptionChain[C:\Windows\SYSTEM32\ntdll.dll +0x63b26]

Comment 1

[Carsten Book [:Tomcat]]

Timothy: is this something for you ?

Comment 2

[Tyson Smith [:tsmith]]

Timothy: Is this a security issue? If so could you please add a rating? If not feel free to open it up.

Comment 3

[Timothy Nikkel (:tnikkel)]

Yes, this is likely a sec issue, likely sec-crit I'm guessing.

Likely the issue is that the dir entries in the ico lie about the size of the contained bmp/png, which we've come across before, but the fix is somewhat involved. I am planning to work on the fix.

Comment 4

[Andrew McCreight [:mccr8]]

Assigning to Timothy because he said he'd work on it. Thanks!

Comment 5

[Carsten Book [:Tomcat]]

bughunter reported several crashes now for this URL, Timothy do you have a eta then this can be fixed ? 

Seems this affect beta -> nightly

Comment 6

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

Tracking since this is sec-critical.

Comment 7

[Carsten Book [:Tomcat]]

bughunter still reports this since it seems people still hit this crash/assertion

Comment 8

[Carsten Book [:Tomcat]]

milan, tn: do you  guys have a eta when this could be fixed. Sec critical and a popular site like duckduckgo is maybe not good

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Top crasher getting in the way.

I see there were crashes mentioned in comment 5, can we get a pointer to some of those?

Comment 10

[Tyson Smith [:tsmith]]

Attachment: test_case.html

I managed to get a test case to repro this.

Comment 11

[Tyson Smith [:tsmith]]

Attachment: test_case.ico

Comment 12

[Carsten Book [:Tomcat]]

thanks tyson!

(In reply to Milan Sreckovic [:milan] from comment #9)
> Top crasher getting in the way.
> 
> I see there were crashes mentioned in comment 5, can we get a pointer to
> some of those?

the real world website where this crash happens is https://duckduckgo.com/?q=erol%2Bsahin%2Bkovan

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

Right - I meant a crash-stats type crash report, but I realize that perhaps the crashes mentioned in comment 5 are all bug hunter type debug build asserts?

Comment 14

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

Too late for a fix for 46, but we could still take a patch for 47.

Comment 15

[Andrew McCreight [:mccr8]]

Any updates here? This sec-critical bug has been inactive for a month.

Comment 16

[Carsten Book [:Tomcat]]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> Any updates here? This sec-critical bug has been inactive for a month.

was in email contact with Timothy and he said he will work on this soon

Comment 17

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

Won't fixing for 47. It has missed the window.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Taking this. I'll get it fixed within the next couple of working days. (This is a 3 day weekend in the US, just FYI, so that means "sometime between now and Wednesday evening".)

Comment 19

[Timothy Nikkel (:tnikkel)]

So looks like these types of files are rejected by chrome. Wish I knew that five months. Writing a patch to reject these files is a thousand times simpler than writing a patch to make them work.

Comment 20

[Carsten Book [:Tomcat]]

seems http://www.doviddebresser.info/Collection.php?letter=g is a new site that show this problem in the wild according to bughunter and reproduced the problem there too

Comment 21

[Carsten Book [:Tomcat]]

since this already missed 46 and 47 can make sure this does not miss anymore releases, since its a sec-critical bug

Comment 22

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

Argh. We need to uplift this all the way to beta. The same patch will work on
all branches, though.

The patch is simple: we don't want to let the size information from the BIH
override the size we read from the ICO directory entry. Instead, if there's a
mismatch, decoding should fail. That's important not only because of the
assertion in this bug, but also because if we allowed it, there'd be a mismatch
between the intrinsic size of the ICO (determined from the directory entry) and
the size of the surface we decoded for it, which is very bad and would lead to
all sorts of bugs.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a test that decoding ICOs with a size mismatch between the BIH header and the ICO directory entry fails.

This part adds a test for this problem.

Comment 24

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8763299 [details] [diff] [review]
(Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

Approval Request Comment
[Feature/regressing bug #]: See blocked bug.
[User impact if declined]: Possible memory corruption when downscaling a corrupt ICO file.
[Describe test coverage new/current, TreeHerder]: A new test is added in this bug.
[Risks and why]: Should be low risk. The fix isn't complicated. We just need to reject some corrupt ICOs that we previously accepted (and which Blink and Webkit also reject).
[String/UUID change made/needed]: None.

Comment 25

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8763299 [details] [diff] [review]
(Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It'd be pretty straightforward to construct an ICO that would overwrite a little bit of memory past the end of the surface we're decoding into. The difficulty of generating a more serious exploit from that is beyond my area of knowledge.

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

Pretty much, at least if the person reading the patch knows that this is a security bug.

Which older supported branches are affected by this flaw?

Branches all the way back to 46 are affected.

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

See the blocked bug.

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

The same patch should work for any affected branch.

How likely is this patch to cause regressions; how much testing does it need?

Likelihood of regression is low but not impossible. I'd be hesitant to land this patch on beta a few days before release, but really the patch is pretty simple so it should be hard to get wrong. It's also safe to say that the patch won't introduce a new security bug, since it strictly reduces the number of ICO images we accept, and does so using a very simple check.

Comment 26

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

Comment on attachment 8763299 [details] [diff] [review]
(Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

sec-approval+

Comment 27

[Nicholas Nethercote [inactive]]

Comment on attachment 8763299 [details] [diff] [review]
(Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

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

I'm worried that this will cause us to reject a non-trivial number of previously accepted images. Isn't it quite common for the directory sizes to not match the BMP sizes? I seem to recall that this is the case. So landing this on Beta immediately worries me.

Also, I'm not happy with the width handling at the moment, see below.

::: image/decoders/nsICODecoder.cpp
@@ +117,5 @@
> +    // the ICO width.
> +    if (width == 256) {
> +      mDirEntry.mWidth = 0;
> +    } else {
> +      mDirEntry.mWidth = (int8_t)width;

This whole function feels iffy. I think the cast to int8_t should instead be a cast to uint8_t, and it's not clear to me what happens if |width| is negative. And the |width == 256| test is repeated.

If we instead test for (width < 0 || width > 256) then I don't think the overflow check below is necessary and the whole function will be shorter and simpler.

Also, I've never got my head around the "256 becomes 0" thing here. What does it mean? Is it specified in the file format? A comment about that would be helpful.

@@ +123,5 @@
> +
> +    // Verify that the BMP width matches the width we got from the ICO directory
> +    // entry. If not, decoding fails, because if we were to allow it to
> +    // continue the intrinsic size of the image wouldn't match the size of the
> +    // decoded surface.

Actually, now I'm confused. Is this an overflow check? Maybe not... how are we verifying the BMP width matches the directory width when we just overwrote the latter with the former? The old code let the BMP width override the directory width, but you're not allowing that any more...

Comment 28

[Carsten Book [:Tomcat]]

note for the main patch review is minus, so can't be uplifted currently

Comment 29

[Nicholas Nethercote [inactive]]

> I'm worried that this will cause us to reject a non-trivial number of
> previously accepted images. Isn't it quite common for the directory sizes to
> not match the BMP sizes? I seem to recall that this is the case. So landing
> this on Beta immediately worries me.

tnikkel, what do you think about this? I'd like a second opinion.

Comment 30

[Seth Fowler [:seth] [:s2h]]

(In reply to Nicholas Nethercote [:njn] from comment #27)
> I'm worried that this will cause us to reject a non-trivial number of
> previously accepted images. Isn't it quite common for the directory sizes to
> not match the BMP sizes? I seem to recall that this is the case. So landing
> this on Beta immediately worries me.

Such images are rejected already by Blink and WebKit; see above.

> This whole function feels iffy. I think the cast to int8_t should instead be
> a cast to uint8_t, and it's not clear to me what happens if |width| is
> negative. And the |width == 256| test is repeated.

Yeah, I should probably rewrite this function a little more thoroughly. This is a holdover from the old ICO decoder code; it only looks like a rewrite because I changed the indentation. I can clean it up more.

> Also, I've never got my head around the "256 becomes 0" thing here. What
> does it mean? Is it specified in the file format? A comment about that would
> be helpful.

I'm not sure that a "spec" for the ICO file format truly exists, but yes, that's the standard behavior. See e.g. here:

https://blogs.msdn.microsoft.com/oldnewthing/20101018-00/?p=12513

Nothing about our behavior has changed here.

> Actually, now I'm confused. Is this an overflow check? Maybe not... how are
> we verifying the BMP width matches the directory width when we just
> overwrote the latter with the former? The old code let the BMP width
> override the directory width, but you're not allowing that any more...

Actually, what's confusing here is that I appear to have broken this patch during a rebase - some of the old code is mixed in with the new code. (The "We should always trust..." stuff should not be there.) Argh. Let me fix it and reupload.

Comment 31

[Nicholas Nethercote [inactive]]

> tnikkel, what do you think about this? I'd like a second opinion.

Oh, I missed comment 19. Sounds like tnikkel is happy with this approach.

Comment 32

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

OK, here's an updated version of part 1. The changes are:

(1) I removed the three lines that were mistakenly included from the old version
of the code, as mentioned above. I think those lines caused a lot of confusion
during the review; hopefully everything makes sense now.

(2) I tried to improve the comments to clarify what's happening in this code.

(3) I made the local variable |width| const in keeping with my general policy of
making things const where possible.

Comment 33

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add a test that decoding ICOs with a size mismatch between the BIH header and the ICO directory entry fails.

I updated part 2 by adding separate tests for width mismatch and height
mismatch. If I had had a test for the width mismatch case, that rebase error
would never have slipped through.

Comment 34

[Nicholas Nethercote [inactive]]

Comment on attachment 8763758 [details] [diff] [review]
(Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

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

r=me with the comments below addressed.

::: image/decoders/nsICODecoder.cpp
@@ +112,5 @@
> +    //   https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376(v=vs.85).aspx
> +    //
> +    // However, we reject negative widths since they aren't meaningful.
> +    const int32_t width = LittleEndian::readInt32(aBIH + 4);
> +    if (width < 0 || width > 256) {

I think the first conjunct should be |width <= 0|. Otherwise, we might succeed when comparing a BMP of width 0 to an ICO dir entry of width 0, which should actually fail because the latter should be interpreted as 256. (Well, it depends on how the comparison below is handled. But it would still be clearer written with <=.)

@@ +123,5 @@
> +    // decoded surface.
> +    const int32_t expectedDirEntryWidth = width == 256 ? 0 : width;
> +    if (mDirEntry.mWidth != expectedDirEntryWidth) {
> +      return false;
> +    }

As discussed on IRC, this appears to be correct, but it sure is confusing. This would be much clearer:

  if (GetRealWidth() != width) {
    return false;
  }

i.e. convert from ICO crazy units into normal units before comparing, instead of converting from normal units into ICO crazy units :)

@@ +141,5 @@
> +    // the AND mask. This is true even if the AND mask is not present.
> +    height /= 2;
> +    if (height > 256) {
> +      return false;
> +    }

Add a |height <= 0| test, like the width case?

@@ +148,5 @@
> +    // entry. If not, again, decoding fails.
> +    const int32_t expectedDirEntryHeight = height == 256 ? 0 : height;
> +    if (mDirEntry.mHeight != expectedDirEntryHeight) {
> +      return false;
> +    }

As above, make this:

  if (GetRealHeight() != height) {
    return false;
  }

@@ +152,5 @@
> +    }
> +
> +    // Fix the BMP height in the BIH so that the BMP decoder, which does not
> +    // know about the AND mask that may follow the actual bitmap, can work
> +    // properly. Note that because of the checks above

The last sentence is truncated.

Comment 35

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! It definitely made the patch cleaner.

(In reply to Nicholas Nethercote [:njn] (on vacation until July 11) from comment #34)
> Add a |height <= 0| test, like the width case?

I added a test for |height == 0| (since negative heights are OK).

> > +    // Fix the BMP height in the BIH so that the BMP decoder, which does not
> > +    // know about the AND mask that may follow the actual bitmap, can work
> > +    // properly. Note that because of the checks above
> 
> The last sentence is truncated.

The last sentence was intended to explain that |height| is guaranteed to match the value in the ICO header, but on reflection, since that's true, it's cleaner just to write |GetRealHeight()| into the BIH instead of |height|. I've made that change.

Comment 36

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

Here's the updated version of part 1. This is now ready to land.

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for build bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=30439916&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/2245bcadd89f

Comment 38

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Verify that the size in the BIH header matches the ICO directory entry instead of fixing it.

Added casts to fix a warnings-as-errors failure.

Comment 40

[Carsten Book [:Tomcat]]

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

https://hg.mozilla.org/mozilla-central/rev/63ed21f31fc7

thanks a lot seth!

Comment 41

[Seth Fowler [:seth] [:s2h]]

Pushed to Aurora as well. Per the discussion above, we'll wait a little while before pushing to beta.

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/29f08288ee29
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/08127897283d

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Seth, please don't forget to update the status flags when you manually push the changeset. Thanks!

Comment 43

[Carsten Book [:Tomcat]]

just for reference, security approval is in https://bugzilla.mozilla.org/show_bug.cgi?id=1249578#c26

can we also restore the beta-approval flag so this get not missed in uplifts

Comment 44

[Seth Fowler [:seth] [:s2h]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #42)
> Seth, please don't forget to update the status flags when you manually push
> the changeset. Thanks!

Whoops! Sorry about that.

(In reply to Carsten Book [:Tomcat] from comment #43)
> just for reference, security approval is in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1249578#c26
> 
> can we also restore the beta-approval flag so this get not missed in uplifts

I tried but I can't. =\

We should probably uplift this by the end of this week. No regressions reported so far that I can see.

Comment 45

[Carsten Book [:Tomcat]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #44)
> (In reply to Sylvestre Ledru [:sylvestre] from comment #42)
> > Seth, please don't forget to update the status flags when you manually push
> > the changeset. Thanks!
> 
> Whoops! Sorry about that.
> 
> (In reply to Carsten Book [:Tomcat] from comment #43)
> > just for reference, security approval is in
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1249578#c26
> > 
> > can we also restore the beta-approval flag so this get not missed in uplifts
> 
> I tried but I can't. =\
> 
> We should probably uplift this by the end of this week. No regressions
> reported so far that I can see.

shall i uplift this now to beta :)

Comment 46

[Seth Fowler [:seth] [:s2h]]

Yes, please!

Comment 47

[Carsten Book [:Tomcat]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #46)
> Yes, please!

done!

https://hg.mozilla.org/releases/mozilla-beta/rev/be34b11bfd07
https://hg.mozilla.org/releases/mozilla-beta/rev/13ce99eb20be

Comment 49

[Daniel Veditz [:dveditz]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #25)
> Which older supported branches are affected by this flaw?
> Branches all the way back to 46 are affected.
> 
> If not all supported branches, which bug introduced the flaw?
> See the blocked bug.

Which blocked bug? Sysiphus-crashes is a meta bug and bug 1262333 is a duplicate, not a regressor, and has no regression ranges. Duplicate bug 1277741 blames bug 1201796 so I guess we'll go with that. That means this goes back to fx43, not 46

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?
> 
> Pretty much, at least if the person reading the patch knows that this is a
> security bug.

We need this on ESR-45.

Comment 50

[Petruta Horea [:phorea], Desktop QA]

Reproduced the crash/plugin container stop/assertion failure with old debug builds under Win 10 64-bit.

The ico images in the test cases are no longer loaded in Nightly 50.0a1, Aurora 49.0a2 and beta 48 debug and normal builds under Win 10. 

On the other platforms I checked that normal builds don't show those ico anymore. Marking as verified.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Sheriff, could you land this in esr45? Thanks

Comment 52

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/62375186b606
remote:   https://hg.mozilla.org/releases/mozilla-esr45/rev/1188098e26d5

Comment 53

[Ryan VanderMeulen [:RyanVM]]

I'm assuming that the landed gtest is sufficient test coverage for this issue.

Comment 54

[Petruta Horea [:phorea], Desktop QA]

Verified that using Firefox 45.3ESR (under Win 10 64-bit and Mac OS X 10.9.5), the ico images are not displayed.

Comment 55

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

We are still seeing some reports with this signature, reported in comments by the RMA bot in bug 1239392.  Seth, do they look related to this issue, or should we open a new bug?