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

RESOLVED FIXED in Firefox 12

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jseward, Assigned: jgilbert)

Tracking

({valgrind})

Trunk
mozilla13
x86_64
Linux
valgrind
Points:
---

Firefox Tracking Flags

(firefox12+ fixed, firefox13+ fixed, firefox14+ fixed, firefox-esr1012+ fixed)

Details

(Whiteboard: [sg:vector-critical (gcc)][qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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)
(Reporter)

Updated

6 years ago
Keywords: valgrind
(Reporter)

Comment 1

6 years ago
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)
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?
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.
(Reporter)

Comment 4

6 years ago
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).
(Assignee)

Comment 5

6 years ago
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?
(Assignee)

Comment 6

6 years ago
Created attachment 595172 [details] [diff] [review]
Split out the 32-bit case from ConvertHostARGB

This should do it.
Assignee: nobody → jgilbert
Status: NEW → ASSIGNED
Attachment #595172 - Flags: review?(joe)
Attachment #595172 - Flags: review?(joe) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b84e21604bc4
Target Milestone: --- → mozilla13
> 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.
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?
This still reproduces with gcc trunk r184188.
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)
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → fixed
status-firefox14: --- → fixed
tracking-firefox13: --- → +
tracking-firefox14: --- → +
Resolution: --- → FIXED
Whiteboard: [sg:vector-critical (gcc)]
tracking-firefox-esr10: --- → 12+
tracking-firefox12: --- → +
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?
tracking-firefox-esr10: 12+ → 13+
> 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.
(Assignee)

Comment 15

5 years ago
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.
Attachment #595172 - Flags: approval-mozilla-esr10?
Attachment #595172 - Flags: approval-mozilla-beta?

Updated

5 years ago
tracking-firefox-esr10: 13+ → 12+
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.
Attachment #595172 - Flags: approval-mozilla-esr10?
Attachment #595172 - Flags: approval-mozilla-esr10+
Attachment #595172 - Flags: approval-mozilla-beta?
Attachment #595172 - Flags: approval-mozilla-beta+
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.
(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.
http://hg.mozilla.org/releases/mozilla-beta/rev/6e53f939f795
http://hg.mozilla.org/releases/mozilla-esr10/rev/40c57cc64bbe
status-firefox-esr10: affected → fixed
status-firefox12: affected → fixed
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...
(Assignee)

Comment 21

5 years ago
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
Depends on: 650720
Group: core-security
Julian, are you able to reproduce this bug anymore in Firefox 13.0 Beta 6?
Whiteboard: [sg:vector-critical (gcc)] → [sg:vector-critical (gcc)][qa-]
You need to log in before you can comment on or make changes to this bug.