Bug 792305 (CVE-2012-5829)

Heap-buffer-overflow in nsWindow::OnExposeEvent

RESOLVED FIXED in Firefox 18

Status

()

Core
Widget: Gtk
--
critical
RESOLVED FIXED
5 years ago
5 months ago

People

(Reporter: Abhishek Arya, Assigned: karlt)

Tracking

(5 keywords)

Trunk
mozilla18
x86_64
Linux
crash, csectype-bounds, regression, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox15 unaffected, firefox16 unaffected, firefox17 unaffected, firefox18+ fixed, firefox19+ fixed, firefox-esr1017+ fixed)

Details

(Whiteboard: [asan][adv-main18+][adv-esr17+])

Attachments

(6 attachments)

(Reporter)

Description

5 years ago
Reproduces on trunk. Testcase reproduces extremely flakily if i first run firefox with a clean profile (and testcase path on cmd line), then close the firefox completely, and then rerun it again with testcase. Trying to get a minimized testcase. The second-time need might be coming from this line in the testcase

InstallTrigger.install({
    "Unsigned XPI": "http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi"
  });

=================================================================
==12538== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f0886f066d8 at pc 0x7f08b8658875 bp 0x7fff30d69ab0 sp 0x7fff30d69aa8
READ of size 1 at 0x7f0886f066d8 thread T0
    #0 0x7f08b8658874 in UpdateMaskBits(char*, int, int, nsIntRect const&, unsigned char*, int) widget/gtk2/nsWindow.cpp:4261
    #1 0x7f08b8657a51 in nsWindow::OnExposeEvent(_GdkEventExpose*) widget/gtk2/nsWindow.cpp:2222
    #2 0x7f08b866816e in expose_event_cb(_GtkWidget*, _GdkEventExpose*) widget/gtk2/nsWindow.cpp:5042
0x7f0886f066d8 is located 0 bytes to the right of 5720-byte region [0x7f0886f05080,0x7f0886f066d8)
allocated by thread T0 here:
    #0 0x4343e0 in __interceptor_malloc 
    #1 0x7f08bc70c3a8 in moz_xmalloc memory/mozalloc/mozalloc.cpp:57
    #2 0x7f08b8657a51 in nsWindow::OnExposeEvent(_GdkEventExpose*) widget/gtk2/nsWindow.cpp:2222
    #3 0x7f08b866816e in expose_event_cb(_GtkWidget*, _GdkEventExpose*) widget/gtk2/nsWindow.cpp:5042
Shadow byte and word:
  0x1fe110de0cdb: fb
  0x1fe110de0cd8: 00 00 00 fb fb fb fb fb
More shadow bytes:
  0x1fe110de0cb8: 00 00 00 00 00 00 00 00
  0x1fe110de0cc0: 00 00 00 00 00 00 00 00
  0x1fe110de0cc8: 00 00 00 00 00 00 00 00
  0x1fe110de0cd0: 00 00 00 00 00 00 00 00
=>0x1fe110de0cd8: 00 00 00 fb fb fb fb fb
  0x1fe110de0ce0: fa fa fa fa fa fa fa fa
  0x1fe110de0ce8: fa fa fa fa fa fa fa fa
  0x1fe110de0cf0: fa fa fa fa fa fa fa fa
  0x1fe110de0cf8: fa fa fa fa fa fa fa fa
Stats: 293M malloced (320M for red zones) by 521376 calls
Stats: 41M realloced by 23856 calls
Stats: 248M freed by 255478 calls
Stats: 115M really freed by 96425 calls
Stats: 532M (136284 full pages) mmaped in 133 calls
  mmaps   by size class: 8:376809; 9:49146; 10:20475; 11:14329; 12:4096; 13:2560; 14:1280; 15:256; 16:576; 17:1248; 18:288; 19:40; 20:20;
  mallocs by size class: 8:419076; 9:53870; 10:19196; 11:16732; 12:4893; 13:2792; 14:1719; 15:362; 16:881; 17:1501; 18:294; 19:41; 20:19;
  frees   by size class: 8:178304; 9:39375; 10:14131; 11:13204; 12:3650; 13:2549; 14:1466; 15:306; 16:738; 17:1481; 18:219; 19:39; 20:16;
  rfrees  by size class: 8:64681; 9:13811; 10:3824; 11:9587; 12:1194; 13:844; 14:914; 15:151; 16:412; 17:990; 18:11; 19:5; 20:1;
Stats: malloc large: 1855 small slow: 2608
==12538== ABORTING

Updated

5 years ago
Component: General → Widget: Gtk
Product: Firefox → Core
If we're mis-calculating a tranlucency area I don't know why it would be flaky, but it does looks like if ASan caught it reading that byte it will turn around and write an updated value to it in the next statement. If it's just an off-by-one might be pretty hard to exploit. Would a different testcase result in a different offset?

It's in a loop at that point though, so maybe ASan stopped it at the first byte but it would have kept going. We'll have to debug and see.
Whiteboard: [asan]
(Assignee)

Comment 2

5 years ago
We could get into trouble here if mBounds is changed during PaintWindow, but I thought that couldn't happen now with viewmanager.refresh-driver-painting.enabled.

There could also be problems if the child window mBounds were greater than the toplevel window mBounds.  I don't know how that would happen.
(Reporter)

Comment 3

5 years ago
Due to InstallTrigger.install on unsigned xpi, I do see a small child window overhanging from the navigation bar that says "The add-on could not be downloaded because of a connection failure with xyz.com" (non-existent url). I think that is the root cause [its width is shortened when the download fails]. Reduction is going pretty poorly atm since it depends on the timing of that window.
Can you attach the unminimized test?  With some luck it's reproducible for
someone.
Severity: normal → critical
Keywords: crash, testcase-wanted
(Reporter)

Comment 5

5 years ago
(In reply to Mats Palmgren [:mats] from comment #4)
> Can you attach the unminimized test?  With some luck it's reproducible for
> someone.

Sorry, the unminimized test is a fuzzer testcase, so won't be able to attach it here. I am trying to get a somewhat minimized testcase, and will attach as soon as i get one.

Regarding Karl point - "There could also be problems if the child window mBounds were greater than the toplevel window mBounds.  I don't know how that would happen.", this part is easily reproducible by a reduced testcase (enclosing, run from cmd-line). you can just resize the top level window manually (shorten width) and the child window about the error will remain outside bounds.
(Reporter)

Comment 6

5 years ago
Created attachment 662841 [details]
Testcase to show child window is out of bounds (Karl's point)
(Reporter)

Comment 7

5 years ago
Created attachment 663051 [details]
Crasher testcase

Minimized testcase (almost :) that crashes under ASAN.
(Assignee)

Comment 8

5 years ago
It looks like widget bounds can change during PaintWindow if ForcedRepaint().
I assume we could make widget handle that, or perhaps we should also consider doing forced repaints from WillPaintWindow instead of PaintWindow.

Matt is considering changing painting from the refresh driver to WillPaintWindow for performance reasons, but that would also address this.
(Assignee)

Comment 9

5 years ago
Created attachment 663277 [details] [diff] [review]
delay shape mask update from resize to expose

Some initial clean-up.

There is no need to ResizeTransparencyBitmap in NativeResize because we'll get
to OnSizeAllocate from the size-allocate signal anyway.

There is not much point calling ResizeTransparencyBitmap in OnSizeAllocate
because the only difference it might make to appearance would be to make the
unpainted part of the widget visible.  Better to wait until that part is ready
to paint.  In that theme, this changes initialization of the mask on resize
to transparent.
Assignee: nobody → karlt
Status: NEW → ASSIGNED
Attachment #663277 - Flags: review?(roc)
(Assignee)

Comment 10

5 years ago
Created attachment 663278 [details] [diff] [review]
don't update mask bits outside the mask
Attachment #663278 - Flags: review?(roc)
Attachment #663277 - Flags: review?(roc) → review+
Attachment #663278 - Flags: review?(roc) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 663854 [details] [diff] [review]
remove more unnecessary shape code

I forgot to remove the ApplyTransparencyBitmap as well as the ResizeTransparencyBitmap from OnSizeAllocate in attachment 663277 [details] [diff] [review].
(Applying the old mask again won't have any effect.)
And there's some unreached code from null-checking new().
Attachment #663854 - Flags: review?(roc)
Attachment #663854 - Flags: review?(roc) → review+
(Assignee)

Comment 12

5 years ago
Folded attachment 663854 [details] [diff] [review] into attachment 663277 [details] [diff] [review].
https://hg.mozilla.org/integration/mozilla-inbound/rev/f92968ac30fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9ee59a8c3b

I couldn't actually reproduce here, so please check that this resolves the issue.
(Assignee)

Comment 13

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c84ffc9700ae
(Reporter)

Comment 14

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #12)
> Folded attachment 663854 [details] [diff] [review] into attachment 663277 [details] [diff] [review]
> [details] [diff] [review].
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f92968ac30fa
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5a9ee59a8c3b
> 
> I couldn't actually reproduce here, so please check that this resolves the
> issue.

Good news: security crash is fixed. Bad news: there is an ugly black enclosing rectangle visible on the edges. Let me add the screen shot. Just to reiterate on reproducer steps. Start with clean new profile and testcase on cmd-line, close firefox completely and then start firefox with that same profile and again testcase on cmd-line.
(Reporter)

Comment 15

5 years ago
Created attachment 663916 [details]
Screenshot of black enclosing rectangle issue.
(Assignee)

Comment 16

5 years ago
Thanks.  I don't see the black part here.

Can you tell me which window manager is running, please?
Does disabling or enabling compositing change the behavior?
Does the black rectangle disappear immediately on closing the window.
(Reporter)

Comment 17

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #16)
> Thanks.  I don't see the black part here.

Did you see the screenshot in c#15. There is a small black rectangle to the right of 'X' sign.

> 
> Can you tell me which window manager is running, please?

Metacity

> Does disabling or enabling compositing change the behavior?

No. same behavior, with and without compositing enabled.

> Does the black rectangle disappear immediately on closing the window.

Yes if I close this child window (the window that says "this add-on downloaded from example.com...."), then the blck rectangle disappears.
https://hg.mozilla.org/mozilla-central/rev/f92968ac30fa
https://hg.mozilla.org/mozilla-central/rev/5a9ee59a8c3b

Should this have a test?
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox18: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Assignee)

Comment 19

5 years ago
(In reply to Ryan VanderMeulen from comment #18)
> Should this have a test?

A test could be useful, but creating a test that will reproduce may be tricky.
(Assignee)

Comment 20

5 years ago
(In reply to Abhishek Arya from comment #17)
> Did you see the screenshot in c#15. There is a small black rectangle to the
> right of 'X' sign.

Yes.  I can see, thanks.

> Metacity

I can't reproduce the same behavior here with metacity, but I do see other glitches, similar to bug 635897, but more significant more often.

Rather than add more Shape code to work around this, bug 408284 is almost there, so I'll see about getting that completed.

> > Does disabling or enabling compositing change the behavior?
> 
> No. same behavior, with and without compositing enabled.

That's harder to explain.  I see no glitches with compositing disabled.  (The window manager is not involved with these override-redirect windows when compositing is disabled.)
If there is an X server interaction issue, then that needs a different solution.
Karlt: is this old code such that we need this fix on other branches (esr10, beta)?
(Assignee)

Comment 22

5 years ago
The nsWindow.cpp code was designed to expect widget resizes during WillPaintWindow but not during PaintWindow.

Bug 539356 changed things to so that ProcessPendingUpdates() can sometimes call ProcessPendingUpdates() during nsViewManager::Refresh() during PaintWindow.
That allows resizes to happen during PaintWindow() and is what I expect enabled this bug to be triggered.

That change landed for the 17 branch, but is disabled on that branch through viewmanager.refresh-driver-painting.enabled.

Before that change, going back as far as 15 at least, Refresh() does not call ProcessPendingUpdates().  The nsAutoScriptBlocker in Refresh() should prevent any nested event loops that may allow resizes.

So we should be fine on 15 to 17.

On esr10, things are different again:

In nsViewManager::DispatchEvent(), NS_PAINT (the equivalent of PaintWindow) will ProcessPendingUpdates() if mHasPendingUpdates is set.  I think that is fine because ProcessPendingUpdates() will do nothing because the resizes have already happened during NS_WILL_PAINT.

However, NS_PAINT can cause CallWillPaintOnObservers to be called.  I'm not familiar with the didResize logic there, but that can cause nsPresShell::FlushPendingNotifications() to be called, during which anything can happen.  I doubt that that function iterates until there are no more events pending, and so calling CallWillPaintOnObservers on NS_WILL_PAINT does not ensure that nothing happens on NS_PAINT AFAIK.

We can backport attachment 663278 [details] [diff] [review] to esr10.
There is already an assertion there that the patch should have no effect on results, unless this bug is possible there.
Blocks: 539356
status-firefox-esr10: --- → ?
status-firefox15: --- → unaffected
status-firefox16: --- → unaffected
status-firefox17: --- → unaffected
tracking-firefox-esr10: --- → ?

Updated

5 years ago
status-firefox-esr10: ? → unaffected
tracking-firefox-esr10: ? → -
(Assignee)

Comment 23

5 years ago
bajaj: why do you say esr10 is unaffected?
status-firefox-esr10: unaffected → ?
status-firefox-esr10: ? → affected
status-firefox19: --- → fixed
tracking-firefox-esr10: - → 17+
tracking-firefox18: --- → +
tracking-firefox19: --- → +
(In reply to Karl Tomlinson (:karlt) from comment #22)
> We can backport attachment 663278 [details] [diff] [review] to esr10.
> There is already an assertion there that the patch should have no effect on
> results, unless this bug is possible there.

Let's move forward with this for the next ESR. Please backport a patch as soon as possible.
(Assignee)

Comment 25

5 years ago
Picking sec-critical because unrelated memory is overwritten outside the buffer.
Perhaps, though, there are mitigating circumstances, including timing requirements.
Keywords: sec-critical
(Assignee)

Comment 26

5 years ago
Comment on attachment 663278 [details] [diff] [review]
don't update mask bits outside the mask

[Security approval request comment]
How easily can the security issue be deduced from the patch?
The presence of potential overflow bug can be deduced from the patch, but the conditions to generate the overflow are much more complicated.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Same answer as above.  The do not indicate how content might affect the size of a chrome popup.  There are also timing requirements, and on esr10 these include having to influence the chrome popup during an event dispatched because of reflow.

Which older supported branches are affected by this flaw?
esr10

If not all supported branches, which bug introduced the flaw?
Bug 539356 for 18.  Bug may have existed long before esr10.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same key patch applies as intended with fuzz.  The cleanup patches, which include risk, are not required.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely; see quote in comment 24.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Fixes potential heap buffer overflow security risk.
User impact if declined: risk
Fix Landed on Version: 18
Risk to taking this patch (and alternatives if risky): very low risk; see quote in comment 24.
String or UUID changes made by this patch:  none
Attachment #663278 - Flags: sec-approval?
Attachment #663278 - Flags: approval-mozilla-esr10?
(Assignee)

Comment 27

5 years ago
(In reply to Karl Tomlinson (:karlt) from comment #26)
> having to influence the chrome popup during an event dispatched
> because of reflow.

and that needs to be during a paint event on the chrome popup.  I'm not sure content will get to run then, so this may not be exploitable on esr10.
But the patch is safe.
(Assignee)

Updated

5 years ago
status-firefox-esr10: affected → checkin-pending
Attachment #663278 - Flags: sec-approval? → sec-approval+

Updated

5 years ago
Attachment #663278 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 28

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/53363548ad9b
status-firefox-esr10: checkin-pending → fixed
I'm confused. How do we have a bug that affects ESR10 but does not affect 17, which came after ESR10's fork?
(Assignee)

Comment 30

5 years ago
See comment 22.  The painting code has changed several times.  The bug on ESR10 is a theoretical bug.  The testcase here may not reproduce on ESR10, but there is the potential for other triggers.
Whiteboard: [asan] → [asan][adv-track-main17-][adv-track-esr17+]
Alias: CVE-2012-5829
Flags: sec-bounty?
Keywords: testcase-wanted → testcase
Keywords: verifyme
This bug qualifies for the security bug bounty.
Flags: sec-bounty? → sec-bounty+
Whiteboard: [asan][adv-track-main17-][adv-track-esr17+] → [asan][adv-main18+][adv-esr17+]
Keywords: verifyme
Group: core-security
Keywords: regression
Keywords: csectype-bounds
You need to log in before you can comment on or make changes to this bug.