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]

Timothy: is this something for you ?

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

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.

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

bughunter reported several crashes now for this URL, Timothy do you have a eta then this can be fixed ? Seems this affect beta -> nightly

Tracking since this is sec-critical.

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

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

Top crasher getting in the way. I see there were crashes mentioned in comment 5, can we get a pointer to some of those?

I managed to get a test case to repro this.

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

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?

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

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

(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

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

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".)

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.

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

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

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.

This part adds a test for this problem.

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

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

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

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

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

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.

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

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.

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

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

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

https://hg.mozilla.org/mozilla-central/rev/dec7eb3938fb https://hg.mozilla.org/mozilla-central/rev/63ed21f31fc7 thanks a lot seth!

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

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

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

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

(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 :)

Yes, please!

(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

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

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.

Sheriff, could you land this in esr45? Thanks

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

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

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

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?