840480 - use-after-poison in nsIFrame::Properties()

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

ASAN-report from m-c opt-build:

==17323== ERROR: AddressSanitizer use-after-poison on address 0x7f69dc3d35e0 at pc 0x7f69fa02f7a9 bp 0x7fff499743f0 sp 0x7fff499743e8
READ of size 8 at 0x7f69dc3d35e0 thread T0
    #0 0x7f69fa02f7a8 in nsIFrame::Properties() const /home/attekett/firefox/src/layout/base/../generic/nsIFrame.h:746
    #1 0x7f69fa02dad4 in mozilla::css::RestyleTracker::DoProcessRestyles() /home/attekett/firefox/src/layout/base/RestyleTracker.cpp:248
    #2 0x7f69fa06a44d in mozilla::css::RestyleTracker::ProcessRestyles() /home/attekett/firefox/src/layout/base/RestyleTracker.h:201
    #3 0x7f69fa190e33 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/attekett/firefox/src/layout/base/nsRefreshDriver.cpp:898
    #4 0x7f69fa193b7e in mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, long, mozilla::TimeStamp) /home/attekett/firefox/src/layout/base/nsRefreshDriver.cpp:164
    #5 0x7f69fcdd9b23 in nsTimerImpl::Fire() /home/attekett/firefox/src/xpcom/threads/nsTimerImpl.cpp:482
    #6 0x7f69fcdd9fda in nsTimerEvent::Run() /home/attekett/firefox/src/xpcom/threads/nsTimerImpl.cpp:565
    #7 0x7f69fcd1d32b in NS_ProcessNextEvent_P(nsIThread*, bool) /home/attekett/firefox/src/objdir-ff-asan/xpcom/build/nsThreadUtils.cpp:238
    #8 0x7f69fc53eb9c in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/attekett/firefox/src/ipc/glue/MessagePump.cpp:82
    #9 0x7f69fce54743 in MessageLoop::RunInternal() /home/attekett/firefox/src/ipc/chromium/src/base/message_loop.cc:215
    #10 0x7f69fc28feb3 in nsBaseAppShell::Run() /home/attekett/firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #11 0x7f69f9981133 in XREMain::XRE_main(int, char**, nsXREAppData const*) /home/attekett/firefox/src/toolkit/xre/nsAppRunner.cpp:3899
    #12 0x7f69f9981ce7 in XRE_main /home/attekett/firefox/src/toolkit/xre/nsAppRunner.cpp:4102
    #13 0x409de7 in do_main(int, char**, nsIFile*) /home/attekett/firefox/src/browser/app/nsBrowserApp.cpp:186
    #14 0x7f6a03d3276c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
0x7f69dc3d35e0 is located 1376 bytes inside of 8192-byte region [0x7f69dc3d3080,0x7f69dc3d5080)
allocated by thread T0 here:
    #0 0x4306e0 in __interceptor_malloc ??:0
    #1 0x7f6a0243aaa6 in PL_ArenaAllocate /home/attekett/firefox/src/nsprpub/lib/ds/plarena.c:200
Shadow byte and word:
  0x1fed3b87a6bc: f7
  0x1fed3b87a6b8: f7 f7 f7 f7 f7 f7 f7 f7
More shadow bytes:
  0x1fed3b87a698: 00 00 00 f7 f7 f7 f7 f7
  0x1fed3b87a6a0: f7 f7 f7 f7 f7 f7 f7 f7
  0x1fed3b87a6a8: f7 f7 00 00 00 00 00 00
  0x1fed3b87a6b0: 00 00 00 00 00 00 00 00
=>0x1fed3b87a6b8: f7 f7 f7 f7 f7 f7 f7 f7
  0x1fed3b87a6c0: f7 f7 f7 f7 f7 f7 f7 00
  0x1fed3b87a6c8: 00 00 00 00 00 00 00 00
  0x1fed3b87a6d0: 00 00 00 00 00 00 00 00
  0x1fed3b87a6d8: 00 00 00 00 00 00 00 00
Stats: 255M malloced (282M for red zones) by 463360 calls
Stats: 45M realloced by 25252 calls
Stats: 216M freed by 328205 calls
Stats: 84M really freed by 219021 calls
Stats: 468M (119891 full pages) mmaped in 117 calls
  mmaps   by size class: 8:327660; 9:32764; 10:8190; 11:16376; 12:3072; 13:1536; 14:1280; 15:384; 16:1088; 17:1280; 18:48; 19:40; 20:16;
  mallocs by size class: 8:390261; 9:34683; 10:9332; 11:19546; 12:2663; 13:1826; 14:1685; 15:455; 16:1417; 17:1369; 18:65; 19:42; 20:16;
  frees   by size class: 8:274244; 9:24057; 10:5540; 11:17099; 12:1649; 13:1335; 14:1482; 15:316; 16:1026; 17:1349; 18:55; 19:39; 20:14;
  rfrees  by size class: 8:193620; 9:8728; 10:2413; 11:11191; 12:728; 13:553; 14:521; 15:178; 16:787; 17:272; 18:25; 19:4; 20:1;
Stats: malloc large: 1492 small slow: 2567
==17323== ABORTING

Comment 1

[Mats Palmgren (inactive)]

We have a frame in the local OverflowChangedTracker here:
http://mxr.mozilla.org/mozilla-central/source/layout/base/RestyleTracker.cpp#147

the style changes destroys it, but nsCSSFrameConstructor::NotifyDestroyingFrame
has mOverflowChangedTracker == NULL so the frame is not removed.

I think frame poisoning makes it non-exploitable though.

Comment 2

[Mats Palmgren (inactive)]

Also, what if the destroyed frame is an ancestor of a tracked frame?
Can that not occur?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We'd destroy the tracked frame before the ancestor in that case, wouldn't we?

Comment 4

[Mats Palmgren (inactive)]

Ah, we notify all destroyed frames of course, silly me :-)

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Keep a permanent overflow tracker

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 713250 [details] [diff] [review]
Keep a permanent overflow tracker

Review of attachment 713250 [details] [diff] [review]:
-----------------------------------------------------------------

Add an assertion that when the tracker is destroyed, it has been flushed?

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 713250 [details] [diff] [review]
Keep a permanent overflow tracker

[Security approval request comment]
How easily could an exploit be constructed based on the patch? I believe that poisoning prevents exploiting.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No

Which older supported branches are affected by this flaw? 20 and newer.

If not all supported branches, which bug introduced the flaw? Bug 815666

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? They should be identical.

How likely is this patch to cause regressions; how much testing does it need? It shouldn't, but more testing time would be beneficial.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Do I need separate approval for aurora?

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 713250 [details] [diff] [review]
Keep a permanent overflow tracker

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 815666
User impact if declined: Crash on some web pages.
Testing completed (on m-c, etc.): Hasn't landed on m-c yet as I want to land both at the same time.
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Merge day is tomorrow and this hasn't been landed to trunk yet - please try to land today if possible, otherwise it will need an approval-beta nom as well.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Sorry, I'm confused. Do you want me to land it to m-c only before approving? I thought we were supposed to land on all branches at once for security bugs.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Matt Woodrow (:mattwoodrow) from comment #11)
> Sorry, I'm confused. Do you want me to land it to m-c only before approving?
> I thought we were supposed to land on all branches at once for security bugs.

We don't generally approve for uplift until patch(es) have landed successfully (without backout needed) to trunk.

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Also, since this is sec-other and not critical there's no rush to uplift - we can get it into an early FF20 beta if need be.

Comment 14

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/83db4b776451

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/83db4b776451

I assume we'll land the test for this at some point?

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 713250 [details] [diff] [review]
Keep a permanent overflow tracker

[Triage Comment]
Approving for uplift to branches

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d492919c068b

This doesn't apply to beta. From what I can tell, the block of code being changed in nsTransitionManager::UpdateAllThrottledStyles doesn't exist there.

Comment 18

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e28f8d101957

Comment 19

[Ioana (away)]

I've been trying to build and ASAN build to verify this bug, but I can't get past an error at the last step here https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer#LLVM.2FClang:
/bin/rm: cannot remove `/home/ioanabudnar/llvm/projects/build/lib/Support/Release/ConstantRange.d.tmp': No such file or directory
make[1]: *** [/home/ioanabudnar/llvm/projects/build/lib/Support/Release/ConstantRange.o] Error 1
make[1]: Leaving directory `/home/ioanabudnar/llvm/projects/build/lib/Support'
make: *** [all] Error 1

Can someone offer some guidance about what to do next? Or direct me to a build, if you prefer that.

FYI, the builds here https://people.mozilla.com/~choller/firefox/asan/ all say they are Firefox 19, so they're no good for this.

Comment 20

[Mats Palmgren (inactive)]

FYI, mozilla-central ASan builds for Linux64 are now built daily:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/

The documentation on devmo is a bit old - recent Clang versions supports ASan
by default so there's no need to build your own.  If your system have an older
Clang you can download 3.2 from http://llvm.org/releases/download.html
I'm pretty sure Aurora and Beta can be built using Clang 3.2.

Comment 21

[Ioana (away)]

Thanks for the info, Mats :)

Verified as fixed on Firefox 22 for now (20130304030933).

Comment 22

[Ioana (away)]

Also verified as fixed on Firefox 20 beta (built from the 03/05 mozilla-beta repo):
Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20130305 Firefox/20.0

Comment 23

[Ioana (away)]

Verified as fixed on Firefox 21 Aurora (built from the 03/05 mozilla-aurora repo):
Mozilla/5.0 (X11; Linux x86_64; rv:21.0) Gecko/20130305 Firefox/21.0

Comment 24

[Daniel Veditz [:dveditz]]

ASAN 'use-after-poison' detections are usually serious security bugs, and are separate from our own "framepoisoning" mitigation (which is present in release builds).

In this case, however, as Mats notes in comment 1 when ASAN doesn't stop execution this particular bug does then get safely caught by our own kind of non-exploitable framepoisoning.
bp-256f42df-38a1-4992-8bbd-41d362130528

Comment 25

[Mats Palmgren (inactive)]

Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72bd51589d1c

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/72bd51589d1c