The attached crash information was submitted via the ASan Nightly Reporter on mozilla-central-asan-nightly revision 64.0a1-20181021220134-https://hg.mozilla.org/mozilla-central/rev/b2fa4b07f6f7e489047808bd0238301ea943b4b3. For detailed crash information, see attachment.

I'm looking into this together with :nika. My findings so far: 1) This code is really old. 2) This code is Windows only. 3) This code is hard to test because it relies on Windows checking certain assumptions about the data structure before processing it further. It also relies on bounds checking from Windows. 4) This code cannot be reached from content from what I can see, but it might be a viable attacking spot for a sandbox escape. Once you have control over a child, you can write the necessary data directly into the clipboard. As soon as the parent tries to read this data (e.g. when pasting), bad things can happen. 5) There are some integer overflows present in the code that we should fix, but they are likely not the source of the problem here. In order to find what exactly the problem is, I ported the code to Linux and hooked it up to libFuzzer. I also added our own bounds checking and overflow checks to the code, so it hopefully becomes fuzzable. I discussed this with :nika and we agreed that it might be best to add these checks to the code anyway and then add any additional checks required to fix fuzzing testcases, even if some of these checks might also be done by Windows. It is just the safest and easiest thing to do here.

Sounds good. For what it's worth, we rated sec-high, instead of moderate, because it appears that this code is triggered by our IPC, and not by a Windows event, so it's possibly usable in a sandbox escape.

While working on this code, I noticed something that might actually be responsible for exactly the ASan trace that we have here: 1) At https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/widget/windows/nsImageClipboard.cpp#317 there is an if statement that checks if the data has a color table. If there is one, it calculates how large it is (in one case using biClrUsed) and advances aInputBuffer by the amount of bytes, to skip over this table. This table is located between the bmiHeader and the actual bitmap data, see also: https://docs.microsoft.com/en-ca/windows/desktop/api/wingdi/ns-wingdi-tagbitmapinfo I believe we are talking about bmiColors being skipped over here. This fits with this sentence from https://msdn.microsoft.com/en-ca/02f8ed65-8fed-4dda-9b94-7343a0cfa8c1 as well: "When the bitmap array immediately follows the BITMAPINFO structure, it is a packed bitmap. Packed bitmaps are referenced by a single pointer. Packed bitmaps require that the biClrUsed member must be either zero or the actual size of the color table." 2) At https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/widget/windows/nsImageClipboard.cpp#328 we start reading bmiColors by directly accessing it through pBitMapInfo. We then advance aInputBuffer *again* by the number of bytes we just read (aInputBuffer += 3 * sizeof(DWORD) a few lines later), even though we already skipped over this data in step 1). It seems to me that we just skipped over this data twice and advanced the buffer further than it should actually have been. 3) A few lines later, at https://searchfox.org/mozilla-central/rev/72b1e834f384a2ffec6eb4ce405fbd4b5e881109/widget/windows/nsImageClipboard.cpp#341 we call InvertRows with the aInputBuffer as the argument. Since this pointer has been moved forward too much, we can then read out of bounds when reading the data rows in that function (and this is what the ASan trace attached shows). :nika, can you sanity check this? Feel free to also forward to someone from image/gfx to double-check this.

Forwarding the needinfo for comment 5 to :aosmond as discussed on IRC.

If I am reading this code correctly, we have a second implementation of a BMP decoder just for the Windows clipboard code. I think we should just use one of the imagelib abstractions to do the decoding for us, namely imgTools::DecodeImageFromBuffer.

(In reply to Andrew Osmond [:aosmond] from comment #7) > If I am reading this code correctly, we have a second implementation of a > BMP decoder just for the Windows clipboard code. I think we should just use > one of the imagelib abstractions to do the decoding for us, namely > imgTools::DecodeImageFromBuffer. Killing this code altogether is by far the best choice of course. We already test out main BMP decoder so we wouldn't need additional testing. Also, this code here would need additional refactoring per comment 3 to be testable and this bug here might not the only one present.

I'm working on this now.

Clean try run ripping out nsImageClipboard.h/.cpp and replacing with proper imagelib abstractions: https://treeherder.mozilla.org/#/jobs?repo=try&revision=22964c70aea62206902df677a96caab532db9cca

Comment on attachment 9025065 [details] Bug 1501482. [Security Approval Request] How easily could an exploit be constructed based on the patch?: The patch just rips out and replaces the original code entirely with much better tested/supported imagelib APIs. It doesn't modify any of the problematic lines, although certainly one could study the removed code to find the problems that prompted this bug. 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?: All If not all supported branches, which bug introduced the flaw?: None Do you have backports for the affected branches?: Yes If not, how different, hard to create, and risky will they be?: How likely is this patch to cause regressions; how much testing does it need?: It appears to be reasonably well covered by mochitests. I tested locally on a Windows machine to ensure copying an image and pasting into Paint continues to work as expected.

sec-approval+ for trunk. Can we get beta and ESR60 patches made and nominated?

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfb7ee7df99b34bc8e80dcecabcdcd85ff71fd45

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eaa82b1f949dba2823946206a7d2559163c28432

https://hg.mozilla.org/integration/mozilla-inbound/rev/6c28ad7f98a7629f5a1b2f123aaad66f1f6233f0 https://hg.mozilla.org/mozilla-central/rev/6c28ad7f98a7

Not convinced about the rating by comment 4, still seems like a sec-moderate -- especially since it's reading data and not writing as far as we know. If used as a sandbox escape the compromised process can already read all your memory.

Please request Beta/ESR60 approval on this when you get a chance.

Comment on attachment 9025682 [details] [diff] [review] [beta] 0001-Bug-1501482.patch, v1 [Beta/Release Uplift Approval Request] Feature/Bug causing the regression: None User impact if declined: Make cause buffer overflow if an improperly encoded BMP is somehow put into the clipboard. Is this code covered by automated tests?: Yes Has the fix been verified in Nightly?: Yes Needs manual test from QE?: Yes If yes, steps to reproduce: Make sure copying/pasting images from the clipboard works as expected (Windows only). List of other uplifts needed: None Risk to taking this patch: Medium Why is the change risky/not risky? (and alternatives if risky): It is somewhat risky because I replaced so much code, and my integration could have gone wrong somehow. On the counterbalance, it now uses the much better tested BMP decoder/encoder. String changes made/needed: N/A

Comment on attachment 9025683 [details] [diff] [review] [esr60] 0001-Bug-1501482.-r-tnikkel.patch, v1 [ESR Uplift Approval Request] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Make cause buffer overflow if an improperly encoded BMP is somehow put into the clipboard. Fix Landed on Version: 65 Risk to taking this patch: Medium Why is the change risky/not risky? (and alternatives if risky): It is somewhat risky because I replaced so much code, and my integration could have gone wrong somehow. On the counterbalance, it now uses the much better tested BMP decoder/encoder. String or UUID changes made by this patch: N/A

Comment on attachment 9025683 [details] [diff] [review] [esr60] 0001-Bug-1501482.-r-tnikkel.patch, v1 Given the large patch, the possible risk, and that the rating is now sec-moderate I don't think we should take this for esr60.

Comment on attachment 9025682 [details] [diff] [review] [beta] 0001-Bug-1501482.patch, v1 Sounds like we can let this bake in 65 to shake out issues.