Closed
Bug 814995
Opened 12 years ago
Closed 11 years ago
Crash @xul!nsHTMLFramesetFrame::GetSizeOfChildAt on poison value
Categories
(Core :: Layout: Block and Inline, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: nils, Assigned: MatsPalmgren_bugz)
References
Details
(5 keywords, Whiteboard: [adv-main20-])
Crash Data
Attachments
(8 files)
489 bytes,
text/html
|
Details | |
1.44 KB,
text/html
|
Details | |
935 bytes,
patch
|
Details | Diff | Splinter Review | |
3.03 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
1.95 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
7.64 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
9.71 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
19.68 KB,
patch
|
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20.0 Firefox/20.0 Build ID: 20121125030729 Steps to reproduce: Description: The attached testcase crashes the browser with two frameset appended to a document. Crashes on a poison value, which indicates a use-after-free. Tested and Affected Versions: Firefox 20.0a1 Testcase: <html> <script> function start() { tmp = document.createElement('iframe'); document.documentElement.appendChild(tmp); window.setTimeout('second()',100); } function second() { tmp.contentDocument.removeChild(tmp.contentDocument.childNodes[0]); o988=document.createElement('frameset'); o1051=document.createElement('frameset'); tmp.contentDocument.appendChild(o1051); tmp.contentDocument.documentElement.appendChild(o988); } </script> <body onload="start()"></body> </html> Stack Backtrace: Windows: xul!nsHTMLFramesetFrame::GetSizeOfChildAt+0x35: 714ec333 8b34ba mov esi,dword ptr [edx+edi*4] ds:002b:f0090100=???????? xul!nsHTMLFramesetFrame::GetSizeOfChildAt+0x35 xul!nsHTMLFramesetFrame::GetSizeOfChild+0x36 xul!nsHTMLFramesetFrame::GetDesiredSize+0x5c xul!nsHTMLFramesetFrame::Reflow+0x47 xul!nsBlockReflowContext::ReflowBlock+0xd1 xul!nsBlockFrame::ReflowBlockFrame+0x39a xul!nsBlockFrame::ReflowLine+0x50 xul!nsBlockFrame::ReflowDirtyLines+0x254 xul!nsBlockFrame::Reflow+0x1b2 xul!nsContainerFrame::ReflowChild+0x5d xul!nsCanvasFrame::Reflow+0x138 xul!nsHTMLReflowState::Init+0x29f xul!nsHTMLScrollFrame::ReflowScrolledFrame+0x17b xul!nsHTMLScrollFrame::ReflowContents+0x87 xul!nsHTMLScrollFrame::Reflow+0x13a xul!nsContainerFrame::ReflowChild+0x5d xul!ViewportFrame::Reflow+0x1ee reported by nils of vulndev ltd. Actual results: Firefox crashed. Expected results: Firefox shouldn't have crashed.
Updated•12 years ago
|
Attachment #684995 -
Attachment mime type: text/plain → text/html
Comment 1•12 years ago
|
||
Not a recent regression, Firefox 17 is affected too. bp-ff4c3ef9-f0fb-4a20-aeba-1f88f2121126 Not quite the same: in Fx17 I get a null deref rather than a poisoned frame but at least the stack is similar enough to say it's the same thing.
Status: UNCONFIRMED → NEW
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox20:
--- → affected
Ever confirmed: true
Version: 20 Branch → Trunk
Comment 2•12 years ago
|
||
This does not affect the ESR-10 branch, however, so it is a regression somewhere.
status-firefox-esr10:
--- → unaffected
Keywords: regression
Comment 3•12 years ago
|
||
The report in comment #1 has a different signature than what we'd get out of the stack in comment #0, so adding both here.
Crash Signature: [@ nsHTMLFramesetFrame::GetDesiredSize ]
[@ nsHTMLFramesetFrame::GetSizeOfChildAt]
Updated•12 years ago
|
Component: Untriaged → Layout
Product: Firefox → Core
Summary: Crash @xul!nsHTMLFramesetFrame::GetSizeOfChildAt → Crash @xul!nsHTMLFramesetFrame::GetSizeOfChildAt on poison value
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → matspal
Severity: normal → critical
Component: Layout → Layout: Block and Inline
Keywords: reproducible
Assignee | ||
Comment 4•12 years ago
|
||
This is the frame tree when we Reflow 0x7fffb6b5c090 (blue). Reflow calls GetDesiredSize: http://hg.mozilla.org/mozilla-central/annotate/c63d5cff18ba/layout/generic/nsFrameSetFrame.cpp#l934 which calls GetFramesetParent(this) http://hg.mozilla.org/mozilla-central/annotate/c63d5cff18ba/layout/generic/nsFrameSetFrame.cpp#l678 699 nsHTMLFramesetFrame* nsHTMLFramesetFrame::GetFramesetParent(nsIFrame* aChild) 700 { 701 nsHTMLFramesetFrame* parent = nullptr; 702 nsIContent* content = aChild->GetContent(); 703 704 if (content) { 705 nsCOMPtr<nsIContent> contentParent = content->GetParent(); 706 707 if (contentParent && contentParent->IsHTML() && 708 contentParent->Tag() == nsGkAtoms::frameset) { 709 nsIFrame* fptr = aChild->GetParent(); 710 parent = (nsHTMLFramesetFrame*) fptr; 711 } 712 } 713 714 return parent; 715 } were we cast the parent frame (nsBlockFrame) to nsHTMLFramesetFrame. GetDesiredSize then calls GetSizeOfChild with that frame. http://hg.mozilla.org/mozilla-central/annotate/c63d5cff18ba/layout/generic/nsFrameSetFrame.cpp#l736
Assignee | ||
Comment 5•12 years ago
|
||
This fixes it, but...
Assignee | ||
Comment 6•12 years ago
|
||
... I don't really see the need for the content checks that GetFramesetParent does. Why should we question what the frame constructor did here? GetDesiredSize() only cares that it's a nsHTMLFramesetFrame anyway. https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=0b35d8177fba
Attachment #686364 -
Flags: review?(roc)
Assignee | ||
Comment 7•12 years ago
|
||
Comment on attachment 686364 [details] [diff] [review] crash fix, part 1 There's a few more casts in this file that looks suspicious. I'll fix those too.
Attachment #686364 -
Flags: review?(roc)
Assignee | ||
Comment 8•12 years ago
|
||
Remove a redundant cast. Make nsHTMLFramesetBorderFrame::HandleEvent use do_QueryFrame instead of casting.
Attachment #686393 -
Flags: review?(roc)
Assignee | ||
Comment 9•12 years ago
|
||
Remove the 4th arg for nsHTMLFramesetFrame::CanChildResize which is a bool to indicate whether to cast the frame it finds to a frameset or not. Make it use do_QueryFrame instead. The rest is just removing code as a consequence of that.
Attachment #686396 -
Flags: review?(roc)
Attachment #686393 -
Flags: review?(roc) → review+
Attachment #686396 -
Flags: review?(roc) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #686364 -
Attachment description: fix → crash fix, part 1
Attachment #686364 -
Flags: review?(roc)
Attachment #686364 -
Flags: review?(roc) → review+
Assignee | ||
Comment 10•12 years ago
|
||
Remove the remaining use of the mChildTypes array (which contains a number saying which type of child frame is at that index) because it's fragile and not really needed. What the heck is RecalculateBorderResize() doing anyway? it allocates a new 'childTypes' array, iterates the children setting the type for each, and then ... nothing! was it supposed to swap it with mChildTypes? Anyway, it's bogus since it assigns the frame type values based on the content's nodeinfo. All parts: https://hg.mozilla.org/try/rev/0ea4fddab577 and an Opt build just to check it compiles without DEBUG: https://tbpl.mozilla.org/?tree=Try&rev=3b7d5349cef5
Attachment #686410 -
Flags: review?(roc)
Comment on attachment 686410 [details] [diff] [review] part 4 Review of attachment 686410 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this file could use a lot of love :-)
Attachment #686410 -
Flags: review?(roc) → review+
Assignee | ||
Comment 12•12 years ago
|
||
[Security approval request comment] How easily can the security issue be deduced from the patch? I guess one can deduce it has something to do with <frame> or <frameset>. It's probably quite hard to figure out the real problem. 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? all If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? same patch should work with minor tweaks How likely is this patch to cause regressions; how much testing does it need? low risk
Attachment #686685 -
Flags: sec-approval?
Updated•12 years ago
|
Attachment #686685 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 13•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc20e4d6b9ee
Flags: in-testsuite?
Comment 14•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cc20e4d6b9ee
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 15•11 years ago
|
||
I'm assuming this is an sg:high/crit. If so, please nominate for uplift to Aurora/Beta/ESR10/ESR17. Thanks!
Comment 16•11 years ago
|
||
I think layout crashes where we hit poison values generally mean we're successfully defending ourselves from attack, so this isn't high or critical, but I could be wrong.
Comment 17•11 years ago
|
||
If this isn't sec high/critical then no need to land on esr17 either, marking wontfix.
status-firefox-esr17:
--- → wontfix
Reporter | ||
Comment 18•11 years ago
|
||
I don't think the poison value mitigation is effective in this case, before reproducing I see a range of different crashes, including access to other invalid memory regions. E.g.: xul!nsHTMLFramesetFrame::GetSizeOfChildAt+0x43: 717623fa 8b0c81 mov ecx,dword ptr [ecx+eax*4] ds:002b:4fd01424=???????? 0:000:x86> cdb: Reading initial command 'kp 16;q' ChildEBP RetAddr 0056bca0 717b1061 xul!nsHTMLFramesetFrame::GetSizeOfChildAt(int aIndexInParent = 0n1, struct nsSize * aSize = 0x0056bce4, struct nsIntPoint * aCellIndex = 0x0056bcbc)+0x43 0056bcc4 719a1bf4 xul!nsHTMLFramesetFrame::GetSizeOfChild(class nsIFrame * aChild = 0x07b17f38, struct nsSize * aSize = 0x0056bce4)+0x36 0056bcec 71b25d0d xul!nsHTMLFramesetFrame::GetDesiredSize(class nsPresContext * aPresContext = 0x06b17800, struct nsHTMLReflowState * aReflowState = 0x0056bea0, struct nsHTMLReflowMetrics * aDesiredSize = 0x0056c03c)+0x5c 0056bd90 7149cb2f xul!nsHTMLFramesetFrame::Reflow(class nsPresContext * aPresContext = 0x06b17800, struct nsHTMLReflowMetrics * aDesiredSize = 0x0056c03c, struct nsHTMLReflowState * aReflowState = 0x0056bea0, unsigned int * aStatus = 0x0056bdfc)+0x47 0056bdb4 71465582 xul!nsBlockReflowContext::ReflowBlock(struct nsRect * aSpace = 0x0435afe0, bool aApplyTopMargin = false, struct nsCollapsingMargin * aPrevMargin = 0x4fd01424, int aClearance = 0n0, bool aIsAdjacentWithTop = false, class nsLineBox * aLine = 0x0056bce4, struct nsHTMLReflowState * aFrameRS = 0x00000000, unsigned int * aFrameReflowStatus = 0x00000000, class nsBlockReflowState * aState = 0x00000000)+0x19f 0056c094 7146f64d xul!nsBlockFrame::ReflowBlockFrame(class nsBlockReflowState * aState = 0x00000000, class nsLineList_iterator aLine = class nsLineList_iterator, bool * aKeepReflowGoing = 0x4fd01424)+0x4d2 0056c140 71423a8e xul!nsBlockFrame::ReflowLine(class nsBlockReflowState * aState = 0x0056bce4, class nsLineList_iterator aLine = class nsLineList_iterator, bool * aKeepReflowGoing = <Memory access error>)+0x15d 0056c1f4 71450ca6 xul!nsBlockFrame::ReflowDirtyLines(class nsBlockReflowState * aState = 0x07b18ae0)+0x1be 0056c3c4 714a47d0 xul!nsBlockFrame::Reflow(class nsPresContext * aPresContext = 0x06b17800, struct nsHTMLReflowMetrics * aMetrics = 0x0056c548, struct nsHTMLReflowState * aReflowState = 0x0056c480, unsigned int * aStatus = 0x0056c63c)+0x1c6
Reporter | ||
Comment 19•11 years ago
|
||
The index of the child can also be changed, which might allow the pointer to be outside the "unmappable" memory region: xul!nsHTMLFramesetFrame::GetSizeOfChildAt+0x35: 716f23ec 8b34ba mov esi,dword ptr [edx+edi*4] ds:002b:f0090148=???????? 0:000:x86> cdb: Reading initial command 'kp 16;q' ChildEBP RetAddr 003c87d0 71741061 xul!nsHTMLFramesetFrame::GetSizeOfChildAt(int aIndexInParent = 0n18, struct nsSize * aSize = 0x003c8814, struct nsIntPoint * aCellIndex = 0x003c87ec)+0x35 003c87f4 71931bf4 xul!nsHTMLFramesetFrame::GetSizeOfChild(class nsIFrame * aChild = 0x0dce09c8, struct nsSize * aSize = 0x003c8814)+0x36 003c881c 71ab5d0d xul!nsHTMLFramesetFrame::GetDesiredSize(class nsPresContext * aPresContext = 0x09e79800, struct nsHTMLReflowState * aReflowState = 0x003c89d0, struct nsHTMLReflowMetrics * aDesiredSize = 0x003c8b6c)+0x5c 003c88c0 7142cb2f xul!nsHTMLFramesetFrame::Reflow(class nsPresContext * aPresContext = 0x09e79800, struct nsHTMLReflowMetrics * aDesiredSize = 0x003c8b6c, struct nsHTMLReflowState * aReflowState = 0x003c89d0, unsigned int * aStatus = 0x003c892c)+0x47 003c88e4 713f5582 xul!nsBlockReflowContext::ReflowBlock(struct nsRect * aSpace = 0x0c903880, bool aApplyTopMargin = false, struct nsCollapsingMargin * aPrevMargin = 0x09bf7c40, int aClearance = 0n0, bool aIsAdjacentWithTop = false, class nsLineBox * aLine = 0xf0090100, struct nsHTMLReflowState * aFrameRS = 0x00000000, unsigned int * aFrameReflowStatus = 0x00000000, class nsBlockReflowState * aState = 0x00000000)+0x19f
Updated•11 years ago
|
Updated•11 years ago
|
Reporter | ||
Comment 20•11 years ago
|
||
Daniel , please see my previous comments. I don't think this one should be rated as a DoS as it crashes on non-poison values as well. I will try to minimise such a testcase.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•11 years ago
|
||
Either way, this bug is fixed on trunk which is what the Resolution reflects.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Keywords: csec-dos → csec-framepoisoning
Updated•11 years ago
|
Whiteboard: [adv-main20-]
Comment 22•11 years ago
|
||
Mats: is this truly a framepoisoned crash? the mChildTypes array that you killed wasn't a pointer to frames... if that got out of sync would it potentially lead to casting pointers to a wrong-type-of-frame? If so you could end up pointing at memory zapped with framepoisoned values, but grab values that overlap another object since you've got a wrong idea of the frame object's size. As you say in comment 21 it doesn't affect the resolution of this bug, but it most definitely affects several other important things: - whether it's eligible for a bug bounty - whether we back-port the fix to ESR-17 and b2g18 - whether we release an advisory for it
Flags: needinfo?(matspal)
Whiteboard: [adv-main20-] → [adv-main20?]
Assignee | ||
Comment 23•11 years ago
|
||
Part 1 is the fix for the reported crash. That crash is explained in comment 4 and is unrelated to the mChildTypes array fix in part 4. Let's look at the details of the crash a bit further: http://hg.mozilla.org/mozilla-central/annotate/c63d5cff18ba/layout/generic/nsFrameSetFrame.cpp#l718 We call GetSizeOfChild with a frame that is really a nsBlockFrame that we have casted to nsHTMLFramesetFrame. GetSizeOfChild is non-virtual so that's not exploitable per se. The mFrames loop at line 742 is going to work correctly since both nsHTMLFramesetFrame/nsBlockFrame inherits nsContainerFrame directly, so should have that at the same offset. Indeed, the loop starts and we crash in GetSizeOfChildAt called from line 746. GetSizeOfChildAt is also non-virtual and we call it (again) with the nsBlockFrame 'this'. Now things go wrong - we read members mNumCols, mNumRows and mColSizes[], mRowSizes[]. Those members don't exist on a nsBlockFrame so the read values are random or will SEGV. We don't assign any members or call any virtual methods though, and the out params aSize, aCellIndex are variables on the stack. aCellIndex is ignored by GetSizeOfChildAt. aSize influences nsHTMLFramesetFrame::Reflow in determining the size of the layout box. None of the above seems exploitable to me. I guess theoretically you could read the memory values off the size of the box, maybe. I'd say sec-low at worst. I'm guessing the source of the poison value is that mColSizes/mRowSizes is beyond the end of a nsBlockFrame and were read from another (free'd) object (containing poison) in the pres shell arena. Part 2/3/4 is additional cleanup that has nothing to do with the reported crash(es). I haven't analyzed whether these errors can be exploited.
Flags: needinfo?(matspal)
Comment 24•11 years ago
|
||
Thanks for the analysis, Mats
Comment 25•11 years ago
|
||
Setting flags as we won't be doing a security advisory for this.
Whiteboard: [adv-main20?] → [adv-main20-]
Assignee | ||
Comment 26•10 years ago
|
||
Landed the crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/4fcf84dda2c9
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•