723453 - Heap overrun (read + write) in nsBMPEncoder::ConvertHostARGBRow

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Julian Seward [:jseward]]

x86_64-linux, running on a 16 bit deep vnc server.
TEST_PATH=content/canvas/test/test_toDataURL_alpha.html

(DISPLAY=:1 TEST_PATH=content/canvas/test/test_toDataURL_alpha.html make -C ff-opt mochitest-plain EXTRA_TEST_ARGS='--close-when-done --debugger=vTRUNK --debugger-args="--tool=memcheck --suppressions=/home/sewardj/MOZ/fglrx-supp.supp --suppressions=/home/sewardj/MOZ/moz-supp.supp --error-limit=no --stats=yes --smc-check=all-non-file --trace-children=yes --child-silent-after-fork=yes '--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel' --track-origins=no"') 2>&1 | tee spew2-memcheck-1

produces the errors below -- looks like a read-modify-write of an invalid address

Invalid read of size 1
   at 0x808209D: nsBMPEncoder::ConvertHostARGBRow(unsigned char const*, unsigned char*, unsigned int) (nsBMPEncoder.cpp:450)
   by 0x80828CD: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:228)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
 Address 0x201e63ac is 0 bytes after a block of size 300 alloc'd
   at 0x4C28223: malloc (vg_replace_malloc.c:263)
   by 0x8082787: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (mozalloc.h:309)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)

Invalid write of size 1
   at 0x80820A6: nsBMPEncoder::ConvertHostARGBRow(unsigned char const*, unsigned char*, unsigned int) (nsBMPEncoder.cpp:450)
   by 0x80828CD: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:228)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
 Address 0x201e63ac is 0 bytes after a block of size 300 alloc'd
   at 0x4C28223: malloc (vg_replace_malloc.c:263)
   by 0x8082787: nsBMPEncoder::AddImageFrame(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (mozalloc.h:309)
   by 0x8081EE9: nsBMPEncoder::InitFromData(unsigned char const*, unsigned int, unsigned int, unsigned int, unsigned int, unsigned int, nsAString_internal const&) (nsBMPEncoder.cpp:107)
   by 0x835111B: nsCanvasRenderingContext2D::GetInputStream(char const*, unsigned short const*, nsIInputStream**) (nsCanvasRenderingContext2D.cpp:1345)
   by 0x83E9C9D: nsHTMLCanvasElement::ExtractData(nsAString_internal const&, nsAString_internal const&, nsIInputStream**, bool&) (nsHTMLCanvasElement.cpp:247)
   by 0x83EA1B2: nsHTMLCanvasElement::ToDataURLImpl(nsAString_internal const&, nsIVariant*, nsAString_internal&) (nsHTMLCanvasElement.cpp:340)
   by 0x88607FA: nsIDOMHTMLCanvasElement_ToDataURL(JSContext*, unsigned int, JS::Value*) (dom_quickstubs.cpp:17910)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)
   by 0x908762D: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2801)
   by 0x908DACC: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jsinterp.cpp:537)
   by 0x90554C2: js_fun_apply(JSContext*, unsigned int, JS::Value*) (jsinterp.h:157)
   by 0x908D9D6: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:311)

Comment 1

[Julian Seward [:jseward]]

Given that the offending line, allegedly nsBMPEncoder.cpp:450 is

    if (mBMPInfoHeader.bpp == 32) {
      pixelOut[3] = alpha;   // line 450
    }

it seems odd that there's invalid read as well as an invalid write.
It looks as if gcc has compiled it as if it were

   pixelOut[3] =  mBMPInfoHeader.bpp == 32  ?  alpha  :  pixelOut[3];

which is bizarre, and (I thought) disallowed by the C++11 memory model.

  862096:       66 83 7f 2a 20          cmpw   $0x20,0x2a(%rdi)
  86209b:       74 05                   je     8620a2
                                               <_ZN12nsBMPEncoder18ConvertHostARGBRowEPKhPhj+0x52>
  86209d:       44 0f b6 50 03          movzbl 0x3(%rax),%r10d
  8620a2:       49 83 c0 01             add    $0x1,%r8
  8620a6:       44 88 50 03             mov    %r10b,0x3(%rax)

Comment 2

[Joe Drew (not getting mail)]

Jeff is working on changing a lot of the ARGB conversion code, so he should be aware of this.

Brian did a significant amount of work on the BMP encoder; perhaps he has some ideas?

Comment 3

[Bas Schouten (:bas.schouten)]

I have to agree with Julian on this one. This seems to be gcc compiling to illegal code. I can see why this would be a useful optimization if pixelOut[3] were a legal address. But since it is perfectly fine in this case for that to only be a valid address when mBMPInfoHeader.bpp == 32 it cannot make that assumption.

Comment 4

[Julian Seward [:jseward]]

Ah; slight misunderstanding.  I was merely trying to figure out why
pixelOut[3] = alpha; lead to a read as well as a write.  But now Bas
points it out, perhaps we have no error at all, and both the invalid
read and write are caused gcc's code generation strategy.

In fact, if our code was really broken, we'd only see an invalid
write.  The invalid (read,write) pair implies gcc is broken.  I wonder
if there's a gcc bug open on this.  This was with vanilla 4.6.2, built
from source, building Fx at -O2 (IIRC).

Comment 5

[Kelsey Gilbert [:jgilbert]]

This is annoying, but at least the workaround should be easy.

The worrisome part is that this meme is not uncommon. Generally, you don't want to put a branch in the loop, but when we choose to, we shouldn't need to worry about an invalid access popping out of a branch.

Can we reduce the testcase and file a GCC bug?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Attachment: Split out the 32-bit case from ConvertHostARGB

This should do it.

Comment 7

[Kelsey Gilbert [:jgilbert]]

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

Comment 8

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Can we reduce the testcase and file a GCC bug?

I started a build of firefox to try to reproduce this, but if someone has a .ii file  and the necessary command line, that would save some time.

Comment 9

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

This reproduces with just

void
foo(char* aDest, int y)
{
  aDest[0] = 41;
  if (y == 32) {
    aDest[1] = 42;
  }
}

and
 /usr/libexec/gcc/x86_64-redhat-linux/4.6.2/cc1plus test.ii -quiet -Os -o nsBMPEncoder.s

This doesn't happen with our gcc, it produces:

_Z3fooPci:
.LFB0:
	cmpl	$32, %esi
	movb	$41, (%rdi)
	jne	.L1
	movb	$42, 1(%rdi)
.L1:
	ret

Should we make this bug block one about upgrading gcc?

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

This still reproduces with gcc trunk r184188.

Comment 11

[Daniel Holbert [:dholbert]]

Looks like this made it to m-c a month ago (shortly after the inbound landing in comment 7):
 https://hg.mozilla.org/mozilla-central/rev/b84e21604bc4

(I'll leave it to someone else to determine whether this should be RESOLVED|FIXED, given comment 10)

Comment 12

[Daniel Veditz [:dveditz]]

I'm calling this fixed based on the workaround checkin, although it's troubling we probably have more places like this. Although we do run Valgrind a fair bit and don't seem to hit this that I've seen.

Do we need a separate bug to track the gcc issue or did we file a bug in their tracker? If we did please add it to the See Also field

Comment 13

[Alex Keybl [:akeybl]]

Tracking for ESR 13+, since this is first fixed on 13. Is there something we can uplift to resolve this security bug on Beta 12?

Comment 14

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

> Do we need a separate bug to track the gcc issue or did we file a bug in
> their tracker? If we did please add it to the See Also field

We should probably have a bug in our bugzilla so that we can block upgrading gcc on it.

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 595172 [details] [diff] [review]
Split out the 32-bit case from ConvertHostARGB

[Approval Request Comment]
Regression caused by (bug #): None.
User impact if declined: Potential security issue with BMPs.
Testing completed (on m-c, etc.): m-c, currently on aurora.
Risk to taking this patch (and alternatives if risky): Very low.
String changes made by this patch: None.

Comment 16

[Alex Keybl [:akeybl]]

Comment on attachment 595172 [details] [diff] [review]
Split out the 32-bit case from ConvertHostARGB

[Triage Comment]
Approving for Beta 12 and the ESR branch given the fact that this is a critical security vulnerability and has been deemed low risk.

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Going to try to autoland this - might not get comments back here from automation because this is a security bug and our release@ account doesn't have sec-group permissions, but I will monitor this push and report back if successful.

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Lukas Blakk [:lsblakk] from comment #17)
> Going to try to autoland this - might not get comments back here from
> automation because this is a security bug and our release@ account doesn't
> have sec-group permissions, but I will monitor this push and report back if
> successful.

Autoland for security bugs doesn't currently work, so nothing has landed here - please go ahead with a normal landing.

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/releases/mozilla-beta/rev/6e53f939f795
http://hg.mozilla.org/releases/mozilla-esr10/rev/40c57cc64bbe

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

Sorry for the mis-push on esr10. Re-landed:
https://hg.mozilla.org/releases/mozilla-esr10/rev/9fe4480ebd01

What happened is the patch failed to apply and I didn't pay attention. The reject was ugly, so I copied and pasted the body of ConvertHostARGBRow from the mozilla-beta patch. Seems entirely safe...

Comment 21

[Kelsey Gilbert [:jgilbert]]

Note that this changes the functionality slightly, in that we no longer zero the pixel if alpha is zero. This should have no significant effect, though.

Functionality change is from bug 650720:
http://hg.mozilla.org/mozilla-central/diff/8abaa54efb2a/image/encoders/bmp/nsBMPEncoder.cpp

Comment 22

[u279076]

Julian, are you able to reproduce this bug anymore in Firefox 13.0 Beta 6?