Closed Bug 840480 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla22
Tracking Status
firefox19 --- unaffected
firefox20 + verified
firefox21 + verified
firefox22 --- verified
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: attekett, Assigned: mattwoodrow)

References

Details

(5 keywords, Whiteboard: [asan][adv-main20+])

Attachments

(2 files)

Attached file 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
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.
Assignee: nobody → matt.woodrow
Blocks: 815666
Severity: normal → critical
OS: Linux → All
Hardware: x86_64 → All
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 :-)
Attachment #713250 - Flags: review?(roc)
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?
Whiteboard: [asan]
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
Attachment #713250 - Flags: approval-mozilla-aurora?
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?
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Blocks: 827150
Comment on attachment 713250 [details] [diff] [review]
Keep a permanent overflow tracker

[Triage Comment]
Approving for uplift to branches
Attachment #713250 - Flags: approval-mozilla-beta+
Attachment #713250 - Flags: approval-mozilla-aurora?
Attachment #713250 - Flags: approval-mozilla-aurora+
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[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.
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).
Status: RESOLVED → VERIFIED
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
Whiteboard: [asan] → [asan][adv-main20+]
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
Flags: sec-bounty-
Group: core-security
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.