Closed Bug 814995 Opened 7 years ago Closed 7 years ago

Crash @xul!nsHTMLFramesetFrame::GetSizeOfChildAt on poison value


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

Not set



Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- fixed
firefox21 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 --- wontfix
b2g18 --- wontfix


(Reporter: nils, Assigned: mats)



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

Crash Data


(8 files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:20.0) Gecko/20.0 Firefox/20.0
Build ID: 20121125030729

Steps to reproduce:

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

function start() {
tmp = document.createElement('iframe');

function second() {
<body onload="start()"></body>

Stack Backtrace:

714ec333 8b34ba          mov     esi,dword ptr [edx+edi*4] ds:002b:f0090100=????????


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.

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.
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.
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:
which calls GetFramesetParent(this)

699 nsHTMLFramesetFrame* nsHTMLFramesetFrame::GetFramesetParent(nsIFrame* aChild)
700 {
701   nsHTMLFramesetFrame* parent = nullptr;
702   nsIContent* content = aChild->GetContent();
704   if (content) { 
705     nsCOMPtr<nsIContent> contentParent = content->GetParent();
707     if (contentParent && contentParent->IsHTML() &&
708         contentParent->Tag() == nsGkAtoms::frameset) {
709       nsIFrame* fptr = aChild->GetParent();
710       parent = (nsHTMLFramesetFrame*) fptr;
711     }
712   }
714   return parent;
715 }

were we cast the parent frame (nsBlockFrame) to nsHTMLFramesetFrame.

GetDesiredSize then calls GetSizeOfChild with that frame.
Attached patch wipSplinter 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.
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 2Splinter Review
Remove a redundant cast.
Make nsHTMLFramesetBorderFrame::HandleEvent use do_QueryFrame instead
of casting.
Attachment #686393 - Flags: review?(roc)
Attached patch part 3Splinter 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 4Splinter 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:
and an Opt build just to check it compiles without DEBUG:
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?

Which older supported branches are affected by this flaw?

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+
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 817404
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.
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.:

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:

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.
Resolution: FIXED → ---
Either way, this bug is fixed on trunk which is what the Resolution reflects.
Closed: 7 years ago7 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:
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)
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:
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.