Closed Bug 840480 Opened 8 years ago Closed 8 years ago
use-after-poison in ns
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
8 years ago
Component: General → Layout
Product: Firefox → Core
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.
Also, what if the destroyed frame is an ancestor of a tracked frame? Can that not occur?
We'd destroy the tracked frame before the ancestor in that case, wouldn't we?
Ah, we notify all destroyed frames of course, silly me :-)
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?
Attachment #713250 - Flags: review?(roc) → review+
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.
Attachment #713250 - Flags: sec-approval?
Attachment #713250 - Flags: sec-approval? → sec-approval+
Do I need separate approval for aurora?
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
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.
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.
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/83db4b776451 I assume we'll land the test for this at some point?
Comment on attachment 713250 [details] [diff] [review] Keep a permanent overflow tracker [Triage Comment] Approving for uplift to branches
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.
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: *** [/home/ioanabudnar/llvm/projects/build/lib/Support/Release/ConstantRange.o] Error 1 make: 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.
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.
Thanks for the info, Mats :) Verified as fixed on Firefox 22 for now (20130304030933).
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
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
Attachment #712871 - Attachment mime type: text/plain → text/html
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
Landed the crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/72bd51589d1c
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.