Closed Bug 930381 (CVE-2013-5613) Opened 7 years ago Closed 7 years ago

heap-use-after-free in libxul.so!PresShell::DispatchSynthMouseMove

Categories

(Core :: Layout, defect)

27 Branch
x86_64
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox25 --- wontfix
firefox26 + fixed
firefox27 + fixed
firefox28 + fixed
firefox-esr17 --- wontfix
firefox-esr24 --- fixed
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed

People

(Reporter: tsmith, Assigned: smaug)

References

Details

(4 keywords, Whiteboard: [adv-main26+][adv-esr24.2+][qa-])

Attachments

(3 files)

Attached file 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
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: General → Layout
Product: Firefox → Core
Assignee: nobody → bugs
Based on code inspection, this is a regression from bug 655267
Attached patch patchSplinter Review
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.
Attachment #821958 - Flags: review?(tnikkel)
Comment on attachment 821958 [details] [diff] [review]
patch

Bug 655267 mostly just moved this code, so this could have existed before that bug.
Attachment #821958 - Flags: review?(tnikkel) → review+
Nope. The old code was keeping presshell alive via a strong pointer.
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.
Attachment #821958 - Flags: sec-approval?
Attachment #821958 - Flags: approval-mozilla-esr24?
Attachment #821958 - Flags: approval-mozilla-esr17?
Attachment #821958 - Flags: approval-mozilla-beta?
Attachment #821958 - Flags: approval-mozilla-b2g18?
Attachment #821958 - Flags: approval-mozilla-aurora?
We're shipping on Tuesday so this can't go in until 11/12 or later.
Whiteboard: [checkin after 11/12]
Comment on attachment 821958 [details] [diff] [review]
patch

sec-approval+ and approved for beta and aurora on 11/12 or later.
Attachment #821958 - Flags: approval-mozilla-esr24?
Attachment #821958 - Flags: approval-mozilla-esr24+
Attachment #821958 - Flags: approval-mozilla-esr17?
Attachment #821958 - Flags: approval-mozilla-esr17+
Attachment #821958 - Flags: approval-mozilla-beta?
Attachment #821958 - Flags: approval-mozilla-beta+
Attachment #821958 - Flags: approval-mozilla-aurora?
Attachment #821958 - Flags: approval-mozilla-aurora+
Comment on attachment 821958 [details] [diff] [review]
patch

Fixing flag I forgot to set.
Attachment #821958 - Flags: sec-approval? → sec-approval+
Blocks: 932449
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.
Attached patch additional fixSplinter Review
This should also fix the assertion you mentioned I think.
Attachment #824666 - Flags: review?(bugs)
Comment on attachment 824666 [details] [diff] [review]
additional fix

this should perhaps land separately in a non-sec bug.
Attachment #824666 - Flags: review?(bugs) → review+
Oh, I just saw your comment in the other bug.
Yeah, it's part of the same problem so I think it makes sense to land and
track the patches together here.
yup
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
Attachment #824666 - Flags: sec-approval?
Attachment #824666 - Flags: approval-mozilla-esr24?
Attachment #824666 - Flags: approval-mozilla-esr17?
Attachment #824666 - Flags: approval-mozilla-beta?
Attachment #824666 - Flags: approval-mozilla-b2g18?
Attachment #824666 - Flags: approval-mozilla-aurora?
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).
Attachment #824666 - Flags: sec-approval?
Attachment #824666 - Flags: sec-approval+
Attachment #824666 - Flags: approval-mozilla-esr24?
Attachment #824666 - Flags: approval-mozilla-esr24+
Attachment #824666 - Flags: approval-mozilla-esr17?
Attachment #824666 - Flags: approval-mozilla-beta?
Attachment #824666 - Flags: approval-mozilla-beta+
Attachment #824666 - Flags: approval-mozilla-aurora?
Attachment #824666 - Flags: approval-mozilla-aurora+
Flags: sec-bounty?
Should these patches be rolled into one commit? Suggestions on commit message/messages?
Yeah, that's probably a good idea.  I would write "Bug 930381. r=mats,smaug,tn"
Went with a message smaug gave me over IRC.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59e42825c44
Whiteboard: [checkin after 11/12]
Had to add an include for mozilla/Likely.h on b2g18.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/3d28e6cbacce
https://hg.mozilla.org/mozilla-central/rev/d59e42825c44
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Attachment #821958 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Comment on attachment 824666 [details] [diff] [review]
additional fix


a+ on the b2g18 approval for sanity, this is already landed.
Attachment #824666 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Flags: in-testsuite?
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?
Flags: needinfo?(bugs)
I don't know which other patch might have fixed those assertions, or did Mats' patch fix them perhaps.
Flags: needinfo?(bugs)
In Comment 10, Mats says his patch (which landed at the same time) fixes the assertion, so that's expected.
Keywords: verifyme
Flags: sec-bounty? → sec-bounty+
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.
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/
Flags: needinfo?(tysmith)
Keywords: verifyme
Whiteboard: [adv-main26+][adv-esr24.2+]
Whiteboard: [adv-main26+][adv-esr24.2+] → [adv-main26+][adv-esr24.2+][qa-]
Alias: CVE-2013-5613
Landed the test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3256a8c0d4dd
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.