930381 - (CVE-2013-5613) heap-use-after-free in libxul.so!PresShell::DispatchSynthMouseMove

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: 3D8EA595-001412.html

Found by the BlackBerry Security Automated Analysis Team's fuzzing framework ALF.

==21242==ERROR: AddressSanitizer: heap-use-after-free on address 0x61700022a21c at pc 0x7f0fe52bd9bc bp 0x7fff20ff6650 sp 0x7fff20ff6648
READ of size 4 at 0x61700022a21c thread T0
    #0 0x7f0fe52bd9bb (libxul.so!PresShell::DispatchSynthMouseMove(mozilla::WidgetGUIEvent*, bool)+0x1db)
        Line 75 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/RestyleManager.h"
    #1 0x7f0fe52cc0c4 (libxul.so!PresShell::ProcessSynthMouseMoveEvent(bool)+0xde4)
        Line 5256 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp"
    #2 0x7f0fe52f0547 (libxul.so!nsRefreshDriver::Tick(long, mozilla::TimeStamp)+0xbb7)
        Line 1074 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #3 0x7f0fe52f64e0 (libxul.so!mozilla::RefreshDriverTimer::Tick()+0x1f0)
        Line 168 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #4 0x7f0fe8de4c31 (libxul.so!nsTimerImpl::Fire()+0x6d1)
        Line 546 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #5 0x7f0fe8de52d6 (libxul.so!nsTimerEvent::Run()+0x66)
        Line 630 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #6 0x7f0fe8ddc019 (libxul.so!nsThread::ProcessNextEvent(bool, bool*)+0xaa9)
        Line 622 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp"
    #7 0x7f0fe8d08371 (libxul.so!NS_ProcessNextEvent(nsIThread*, bool)+0xb1)
        Line 251 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp"
    #8 0x7f0fe7955091 (libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+0x311)
        Line 85 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp"
    #9 0x7f0fe8ef7653 (libxul.so!MessageLoop::Run()+0x1c3)
        Line 220 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc"
    #10 0x7f0fe7733cac (libxul.so!nsBaseAppShell::Run()+0x5c)
        Line 161 of "/builds/slave/m-in-l64-asan-0000000000000000/build/widget/xpwidgets/nsBaseAppShell.cpp"
    #11 0x7f0fe7135d9e (libxul.so!nsAppStartup::Run()+0xbe)
        Line 268 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/components/startup/nsAppStartup.cpp"
    #12 0x7f0fe46bf1c5 (libxul.so!XREMain::XRE_mainRun()+0x1e05)
        Line 3886 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #13 0x7f0fe46c00fa (libxul.so!XREMain::XRE_main(int, char**, nsXREAppData const*)+0x4fa)
        Line 3954 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #14 0x7f0fe46c102b (libxul.so!XRE_main+0x3ab)
        Line 4156 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #15 0x459d1d (firefox!main+0x94d)
        Line 275 of "/builds/slave/m-in-l64-asan-0000000000000000/build/browser/app/nsBrowserApp.cpp"
    #16 0x7f0ff3d5876c (libc.so.6!__libc_start_main+0xec)
        Line 226 of "libc-start.c"
    #17 0x45929c (firefox!_start+0x28)
0x61700022a21c is located 28 bytes inside of 760-byte region [0x61700022a200,0x61700022a4f8)
freed by thread T0 here:
    #0 0x4461a5 (firefox!free+0x55)
        Line 64 of "/builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc"
    #1 0x7f0fe529f118 (libxul.so!mozilla::RestyleManager::Release()+0x138)
        Line 225 of "../../dist/include/mozilla/mozalloc.h"
previously allocated by thread T0 here:
    #0 0x4462e5 (firefox!malloc+0x55)
        Line 74 of "/builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc"
    #1 0x7f0feddfe5c8 (libmozalloc.so!moz_xmalloc+0x8)
        Line 54 of "/builds/slave/m-in-l64-asan-0000000000000000/build/memory/mozalloc/mozalloc.cpp"
    #2 0x7f0fe5230421 (libxul.so!nsDocumentViewer::InitInternal(nsIWidget*, nsISupports*, nsIntRect const&, bool, bool, bool)+0x581)
        Line 824 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsDocumentViewer.cpp"
    #3 0x7f0fe522fe90 (libxul.so!nsDocumentViewer::Init(nsIWidget*, nsIntRect const&)+0x20)
        Line 642 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsDocumentViewer.cpp"
    #4 0x7f0fe929f537 (libxul.so!nsDocShell::Embed(nsIContentViewer*, char const*, nsISupports*)+0xe7)
        Line 6397 of "/builds/slave/m-in-l64-asan-0000000000000000/build/docshell/base/nsDocShell.cpp"
    #5 0x7f0fe92b14f4 (libxul.so!nsDocShell::CreateContentViewer(char const*, nsIRequest*, nsIStreamListener**)+0x1084)
        Line 8173 of "/builds/slave/m-in-l64-asan-0000000000000000/build/docshell/base/nsDocShell.cpp"
    #6 0x7f0fe9254ad4 (libxul.so!nsDSURIContentListener::DoContent(char const*, bool, nsIRequest*, nsIStreamListener**, bool*)+0x304)
        Line 122 of "/builds/slave/m-in-l64-asan-0000000000000000/build/docshell/base/nsDSURIContentListener.cpp"
    #7 0x7f0fe92f698f (libxul.so!nsDocumentOpenInfo::TryContentListener(nsIURIContentListener*, nsIChannel*)+0x6ef)
        Line 680 of "/builds/slave/m-in-l64-asan-0000000000000000/build/uriloader/base/nsURILoader.cpp"
    #8 0x7f0fe92f433c (libxul.so!nsDocumentOpenInfo::DispatchContent(nsIRequest*, nsISupports*)+0x67c)
        Line 382 of "/builds/slave/m-in-l64-asan-0000000000000000/build/uriloader/base/nsURILoader.cpp"
    #9 0x7f0fe92f3aaf (libxul.so!nsDocumentOpenInfo::OnStartRequest(nsIRequest*, nsISupports*)+0x32f)
        Line 258 of "/builds/slave/m-in-l64-asan-0000000000000000/build/uriloader/base/nsURILoader.cpp"
    #10 0x7f0fe4964bc2 (libxul.so!nsBaseChannel::OnStartRequest(nsIRequest*, nsISupports*)+0x1e2)
        Line 718 of "/builds/slave/m-in-l64-asan-0000000000000000/build/netwerk/base/src/nsBaseChannel.cpp"
Shadow bytes around the buggy address:
  0x0c2e8003d3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e8003d400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e8003d410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e8003d420: 00 fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2e8003d430: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c2e8003d440: fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2e8003d450: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2e8003d460: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2e8003d470: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2e8003d480: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c2e8003d490: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap right redzone:    fb
  Freed heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==21242==ABORTING

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Based on code inspection, this is a regression from bug 655267

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Note, even with the patch there will be assertion
"###!!! ASSERTION: Must have view manager: '!isSafeToFlush || mViewManager'
but that is actually somewhat bogus, since we have if check right after that to check that
it is safe and we have viewmanager.

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 821958 [details] [diff] [review]
patch

Bug 655267 mostly just moved this code, so this could have existed before that bug.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Nope. The old code was keeping presshell alive via a strong pointer.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 821958 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't think too easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch is rather obvious.

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?
(this is 2+ years old regression)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch should apply everywhere, with some --fuzz

How likely is this patch to cause regressions; how much testing does it need?
Should be super safe.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

We're shipping on Tuesday so this can't go in until 11/12 or later.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 821958 [details] [diff] [review]
patch

sec-approval+ and approved for beta and aurora on 11/12 or later.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 821958 [details] [diff] [review]
patch

Fixing flag I forgot to set.

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 821958 [details] [diff] [review]
patch

This isn't sufficient to fix the problem.

We need to keep the shell alive for sure but DispatchSynthMouseMove
also have a local raw pointer 'restyleManager':
http://hg.mozilla.org/mozilla-central/annotate/829d7bef8b0a/layout/base/nsPresShell.cpp#l3405
so if the shell is Destroy'ed (but not deleted), it will do
SetShell(null) on its prescontext which in turn disconnects
from its mRestyleManager and if that's the last ref it will
be deleted and 'restyleManager' points to deallocated memory.

The testcase in bug 932449 demonstrates this.

Comment 10

[Mats Palmgren (inactive)]

Attachment: additional fix

This should also fix the assertion you mentioned I think.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 824666 [details] [diff] [review]
additional fix

this should perhaps land separately in a non-sec bug.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Oh, I just saw your comment in the other bug.

Comment 13

[Mats Palmgren (inactive)]

Yeah, it's part of the same problem so I think it makes sense to land and
track the patches together here.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

yup

Comment 15

[Mats Palmgren (inactive)]

Comment on attachment 824666 [details] [diff] [review]
additional fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I don't think too easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch is rather obvious.

Which older supported branches are affected by this flaw?
all

If not all supported branches, which bug introduced the flaw?
(this is 2+ years old regression)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch should apply everywhere, with some --fuzz

How likely is this patch to cause regressions; how much testing does it need?
Should be super safe.

(I just copy-pasted Olli's answers ;-) )


[Approval Request Comment]
Bug caused by (feature/regressing bug #): don't know
User impact if declined: sec-critical crash
Testing completed: none
Risk to taking this patch (and alternatives if risky): zero risk
String or UUID changes made by this patch: none

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 824666 [details] [diff] [review]
additional fix

sec-approval+ and other approvals to match the other patch's rating. This should go in on 11/12 with the first.

We don't need ESR17 anymore as I wasn't thinking when I approved it earlier because we don't support it anymore (the release yesterday was the last).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Should these patches be rolled into one commit? Suggestions on commit message/messages?

Comment 18

[Mats Palmgren (inactive)]

Yeah, that's probably a good idea.  I would write "Bug 930381. r=mats,smaug,tn"

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Went with a message smaug gave me over IRC.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59e42825c44

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8c34ae74b10
https://hg.mozilla.org/releases/mozilla-beta/rev/889dd3ef9c6c
https://hg.mozilla.org/releases/mozilla-esr24/rev/9d9222ba5892
https://hg.mozilla.org/releases/mozilla-b2g18/rev/11723a2e1981

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Had to add an include for mozilla/Likely.h on b2g18.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3d28e6cbacce

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/889dd3ef9c6c

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/11723a2e1981
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/3d28e6cbacce

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d59e42825c44

Comment 24

[bhavana bajaj [:bajaj]]

Comment on attachment 824666 [details] [diff] [review]
additional fix


a+ on the b2g18 approval for sanity, this is already landed.

Comment 25

[Ioana (away)]

On Firefox 25 ASAN I get no errors and multiple entries for this assertion:
###!!! ASSERTION: Must have view manager: '!isSafeToFlush || mViewManager', file ../../../layout/base/nsPresShell.cpp, line 3792

The assertion is gone on:
ASAN builds:
Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (20131113065829)
Mozilla/5.0 (X11; Linux x86_64; rv:27.0) Gecko/20100101 Firefox/27.0 (20131114004000)
Mozilla/5.0 (X11; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (20131113030205)

The lack of errors and comment 2 confuse me a bit. Is this a different bug solved by the fix here? If so, have you got any more details on how I could reproduce the bug here?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

I don't know which other patch might have fixed those assertions, or did Mats' patch fix them perhaps.

Comment 27

[Andrew McCreight [:mccr8]]

In Comment 10, Mats says his patch (which landed at the same time) fixes the assertion, so that's expected.

Comment 29

[Matt Wobensmith [:mwobensmith][:matt:]]

Using an ASan FF27 build from 2013-10-25, I can't repro the crash.

Tyson, would you mind verifying in an affected branch that the patch has addressed the problem? Thank you.

Comment 30

[Ioana (away)]

Removing the keyword since QA can't reproduce this locally...

Tyson, if you are interested, you can find the builds for verification here:
ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

Comment 31

[Tyson Smith [:tsmith]]

Verified with build from 03-Dec-2013:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-asan/1386111423/

Comment 33

[Mats Palmgren (inactive)]

Landed the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3256a8c0d4dd

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3256a8c0d4dd