1356704 - Write beyond bounds caused by conflicting buffer size calculations between nsImageFromClipboard::ConvertColorBitMap() and GetEncodedImageStream()

Status: RESOLVED DUPLICATE of bug 1501482

(Core :: Widget: Win32, defect, P3)

Description

[q1]

nsImageFromClipboard::ConvertColorBitMap() (widget\windows\nsImageClipboard.cpp) can write beyond the end of the output buffer provided by nsImageFromClipboard::GetEncodedImageStream().

The bug is that GetEncodedImageStream() calculates its output buffer size using the image header's |bmiHeader.biWidth| and |bmiHeader.biHeight| field (size = width * height * 3), but ConvertColorBitMap() determines how many rows to write by using the image header's |bmiHeader.biSizeImage| field.

If |bmiHeader.biSizeImage| describes a larger image than calculated by GetEncodedImageStream(), ConvertColorBitMap() writes beyond the end of its output buffer.


You can modify the POC attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1356650 to show this bug by modifying the POC to use:

	imagePixelWidth  = 4;
	imagePixelHeight = 16;
	...
	p->bV5SizeImage = 256;

and building/running the POC as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1356650 .

Set a BP on line 235 of nsImageFromClipboard::GetEncodedImageStream(). When the BP triggers, view the disassembly window to see the multiplication in line 235 calculate 0xc0 and allocate that many bytes for the output buffer |rgbData|. Then trace into nsImageFromClipboard::ConvertColorBitmap() and watch it write 0x102 bytes into the output buffer (while also reading that many bytes from the clipboard image data). Thus ConvertColorBitmap() overruns the output buffer by 0x42 bytes.

Comment 1

[q1]

ConvertColorBitmap() generally needs to validate its arguments.

BTW, you can also cause an overflow on line 338:

338:     imageSize = rowSize * pBitMapInfo->bmiHeader.biHeight;

by modifying the POC to use:

        imagePixelWidth  = 32768;
	imagePixelHeight = 43691;
        ...
        p->bV5SizeImage  = 0;

This bug causes the function to process only part of the image, potentially leaving uninitialized heap data in |aOutBuffer|.

Comment 2

[q1]

(In reply to q1 from comment #1)
> ConvertColorBitmap() generally needs to validate its arguments.
> 
> BTW, you can also cause an overflow on line 338:
> 
> 338:     imageSize = rowSize * pBitMapInfo->bmiHeader.biHeight;
> 
> by modifying the POC to use:
> 
>         imagePixelWidth  = 32768;
> 	imagePixelHeight = 43691;
>         ...
>         p->bV5SizeImage  = 0;
> 
> This bug causes the function to process only part of the image, potentially
> leaving uninitialized heap data in |aOutBuffer|.

Oops, that modification of the POC does indeed cause line 338 to overflow, but it also triggers the crash in https://bugzilla.mozilla.org/show_bug.cgi?id=1356668 because |imageSize| ends up smaller than |aNumBytesPerRow|. Anyway, you get the idea.

Comment 3

[Frederik Braun [:freddy]]

Jim, can you please triage/re-assign?

Comment 4

[Jim Mathies [:jimm]]

I'm wondering why this is considered sec-high. AFAICT it requires malicious bitmap data to be copied into the Windows clipboard, then the user would have to paste that into a web page. Open question I have is could a malicious web page inject the malicious data in the clipboard without user involvement?

Comment 5

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #4)
> I'm wondering why this is considered sec-high. AFAICT it requires malicious
> bitmap data to be copied into the Windows clipboard, then the user would
> have to paste that into a web page. Open question I have is could a
> malicious web page inject the malicious data in the clipboard without user
> involvement?

Looks like we have measures to prevent this in place - 

"document.execCommand(‘cut’/‘copy’) was denied because it was not called from inside a short running user-generated event handler."

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Jim Mathies [:jimm] from comment #5)
> "document.execCommand(‘cut’/‘copy’) was denied because it was not called
> from inside a short running user-generated event handler."

If the user clicks on the page the document can execute document.execCommand("copy"), so the user would probably be able to copy a bitmap to the clipboard without too much user interaction, just a click event.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Michael Layzell [:mystor] from comment #6)
> (In reply to Jim Mathies [:jimm] from comment #5)
> > "document.execCommand(‘cut’/‘copy’) was denied because it was not called
> > from inside a short running user-generated event handler."
> 
> If the user clicks on the page the document can execute
> document.execCommand("copy"), so the user would probably be able to copy a
> bitmap to the clipboard without too much user interaction, just a click
> event.

I seriously doubt a corrupt bitmap image loaded into content would get transferred untouched over to the clipboard buffer. Worth investigating I guess.

Comment 8

[Frederik Braun [:freddy]]

q1, last comment here is that there is doubt about this being a valid attack. Can you come up with a bitmap that would be bad to have in clipboard to test this?

Comment 9

[q1]

(In reply to Frederik Braun [:freddyb] from comment #8)
> q1, last comment here is that there is doubt about this being a valid
> attack. Can you come up with a bitmap that would be bad to have in clipboard
> to test this?

From comment 0, you can show this bug by modifying the POC attached to https://bugzilla.mozilla.org/show_bug.cgi?id=1356650 by changing the following values in the POC:

	imagePixelWidth  = 4;
	imagePixelHeight = 16;
	...
	p->bV5SizeImage = 256;

and building/running the POC as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1356650 . Then use the POC as directed in the last paragraph of comment 0.


BTW, I think it probably is currently true that an unprivileged script running in FF cannot place a bad bitmap header onto the clipboard. I have experimented with various ways of using |document.execCommand("copy")}| to attempt this, and have run up against prohibitions:

1. on placing an image directly onto the clipboard, as by handling the |copy| event and using

   e.clipboardData.mozSetDataAt ("application/x-moz-nativeimage", image, 0);

or

   e.clipboardData.mozSetDataAt ("application/x-moz-nativeimage", uint8array, 0);

which is prohibited by DataTransfer::PrincipalMaySetData();

2. on using something else, like a Blob or string containing binary data in the mozSetDataAt() call, which is prohibited by nsDataObj::GetDib() because Blob and strings don't expose the imgIContainer interface. (I can imagine that someone might add that interface to Blob, so this is perhaps a latent bug...); and

3. on creating a synthetic ClipboardEvent (|new ClipboardEvent ("copy")|) and trying (1) on it, which runs into the same prohibition as in (1).

Comment 10

[q1]

To summarize comment 9: (1) You definitely can cause a write beyond bounds by using a program (like the POC) to place a defective image on the clipboard, then pasting it into FF; and (2) You probably cannot use an unprivileged script to make *FF itself* place a defective image on the clipboard.

Comment 11

[Frederik Braun [:freddy]]

It seems Jim is right, this is a sec-moderate, just like bug 1356650 (which q1 refers to in comment 9 and 10).

Comment 12

[Frederik Braun [:freddy]]

This is a security bug either way. Jim, can you fix this or find someone else to fix it?

Comment 13

[Jim Mathies [:jimm]]

(In reply to Frederik Braun [:freddyb] from comment #12)
> This is a security bug either way. Jim, can you fix this or find someone
> else to fix it?

Yep, not a high priority over other P1 type work right now though.

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Bug 1501482 removed this code.