Closed Bug 814995 Opened 13 years ago Closed 13 years ago

Crash @xul!nsHTMLFramesetFrame::GetSizeOfChildAt on poison value

Categories

(Core :: Layout: Block and Inline, defect)

Product:
Core
Component:
Layout: Block and Inline
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Tracking Flags:
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- fixed
firefox21 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: nils, Assigned: MatsPalmgren_bugz)

References

Details

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

Crash Data

Attachments

(8 files)

frames.html (crashes firefox)
489 bytes, text/html
Details
frame tree
1.44 KB, text/html
Details
wip
935 bytes, patch
Details | Diff | Splinter Review
crash fix, part 1
3.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
part 2
1.95 KB, patch
roc
: review+
Details | Diff | Splinter Review
part 3
7.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
part 4
9.71 KB, patch
roc
: review+
Details | Diff | Splinter Review
All four parts folded together
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.
Attachment #684995 - Attachment mime type: text/plain → text/html
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
Keywords: crash, testcase
Version: 20 Branch → Trunk
This does not affect the ESR-10 branch, however, so it is a regression somewhere.
status-firefox-esr10: --- → unaffected
Keywords: regression
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]
Component: Untriaged → Layout
Product: Firefox → Core
Summary: Crash @xul!nsHTMLFramesetFrame::GetSizeOfChildAt → Crash @xul!nsHTMLFramesetFrame::GetSizeOfChildAt on poison value
Assignee: nobody → matspal
Severity: normal → critical
Component: Layout → Layout: Block and Inline
Keywords: reproducible
Attached file frame tree —
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
Attached patch wip — — Splinter Review
This fixes it, but...
... 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)
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)
Attached patch part 2 — — Splinter Review
Remove a redundant cast. Make nsHTMLFramesetBorderFrame::HandleEvent use do_QueryFrame instead of casting.
Attachment #686393 - Flags: review?(roc)
Attached patch part 3 — — Splinter Review
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 #686364 - Attachment description: fix → crash fix, part 1
Attachment #686364 - Flags: review?(roc)
Attached patch part 4 — — Splinter Review
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+
[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?
Attachment #686685 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/cc20e4d6b9ee
Status: NEW → RESOLVED
Closed: 13 years ago
status-firefox20: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 817404
Depends on: 817734
No longer depends on: 817734
I'm assuming this is an sg:high/crit. If so, please nominate for uplift to Aurora/Beta/ESR10/ESR17. Thanks!
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.
If this isn't sec high/critical then no need to land on esr17 either, marking wontfix.
status-firefox-esr17: --- → wontfix
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
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
Blocks: 810303
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 → ---
Either way, this bug is fixed on trunk which is what the Resolution reflects.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [adv-main20-]
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?]
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)
Keywords: csec-framepoisoning, sec-other → sec-low
Thanks for the analysis, Mats
Setting flags as we won't be doing a security advisory for this.
Whiteboard: [adv-main20?] → [adv-main20-]
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.

Attachment

General

Creator:
Created:
Updated:
Size: