frame-poisoned crash in nsHTMLInputElement

RESOLVED FIXED in Firefox 17

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: abbGZcvu_bugzilla.mozilla.org, Assigned: smaug)

Tracking

(Blocks 1 bug, 4 keywords)

16 Branch
mozilla19
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox16 wontfix, firefox17 verified, firefox18 verified, firefox19 fixed, firefox-esr10 wontfix)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

7 years ago
Posted file repro.zip
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.4 (KHTML, like Gecko) Chrome/22.0.1229.94 Safari/537.4

Steps to reproduce:

The below files trigger a use-after-free in Firefox 16.0.1, causing it to crash when tries to accesses data through a poisoned frame. frame.html can trigger the issue by itself, and repro.html is not required but does make it easier to trigger the access violation.

--- repro.html ---
<html>
  <head>
    <script>
      function refresh() {
        setTimeout("location.reload()", 1000);
      }
    </script>
  </head>
  <body>
    <iframe src="frame.html" onload="refresh()" onerror="refresh()"></iframe>
  </body>
</html>

--- frame.html ---
<html>
  <head id="head">
    <input id="input" class="x"/>
    <script type="text/javascript">
      function go() {
        var oHead = document.getElementById("head");
        var oInput = document.getElementById("input");
        document.designMode="on";
        document.documentElement.offsetTop;
        document.addEventListener("DOMAttrModified", function (){
          oHead.outerHTML = "";
        }, true);
        document.body.className = "x";
        setInterval(function(){
          console.log(oInput.selectionDirection);
        }, 10);
      }
    </script>
    <style type="text/css">
      body {
        display:table-row;
      }
      .x {
        position:absolute;
      }
    </style>
  </head>
  <body onload="go()">
  </body>
</html>



Actual results:

Attempt to read from unallocated arbitrary memory (@0xF0DE7FFF) in xul.dll!nsCOMPtr_base::assign_from_qi


Expected results:

No crash
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Assignee

Comment 1

7 years ago
nsTextControlFrame::EnsureEditorInitialized() is unsafe, and it is expected to return
error value in case something odd happens.
Assignee: nobody → bugs
Assignee

Comment 2

7 years ago
Posted patch patch (obsolete) — Splinter Review
The change to nsTextEditorState.cpp ensures that PreDestroy will be
called and we don't get warnings about it being missing all the time.

Change to nsTextEditRules::Init just make assertion to warning.
It really should be just warning since CreateBogusNodeIfNeeded is null safe.

The changes to nsTextControlFrame.cpp are the real crash fixes.
Attachment #672718 - Flags: review?(ehsan)
Assignee

Comment 3

7 years ago
Posted patch -w (obsolete) — Splinter Review
Assignee

Updated

7 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Assignee

Comment 4

7 years ago
Comment on attachment 672718 [details] [diff] [review]
patch

No, need to do something better, since looks like there is still
one ###!!! ASSERTION: Why PreDestroy hasn't been called?: '!mDocWeak || mDidPreDestroy', file /home/smaug/mozilla/hg/mozilla/editor/libeditor/base/nsEditor.cpp, line 162
during shutdown and it means there is a runtime leak.
Attachment #672718 - Flags: review?(ehsan)
Assignee

Comment 5

7 years ago
Posted patch patchSplinter Review
This is a bit safer.
I do still see the assertion once when CC runs after tab close, but the editor 
doesn't stay in the CC log, so at least we don't increase CC times.

The assertion problem could be fixed in a followup, since we should
keep the fix here as simple and safe (for branches) as possible, IMO.
Attachment #672718 - Attachment is obsolete: true
Attachment #672743 - Flags: review?(ehsan)
Assignee

Comment 6

7 years ago
Posted patch -wSplinter Review
Attachment #672719 - Attachment is obsolete: true

Updated

7 years ago
Attachment #672743 - Flags: review?(ehsan) → review+
Assignee

Comment 7

7 years ago
Comment on attachment 672743 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Not too hard. NS_ENSURE_STATE(weakFrame.IsAlive()); is quite clear indicator where the problem is.

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?
as far as I know, all

If not all supported branches, which bug introduced the flaw?
NA

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch seems to apply cleanly to branches, except ESR10 needs some minor tweaking.

How likely is this patch to cause regressions; how much testing does it need?
This should be quite safe. Effectively this is NS_ENSURE_STATE(weakFrame.IsAlive()); and NS_ENSURE_STATE(root);
PreDestroyer is there just to ensure that we PreDestroy editor (and not assert so much and not leak too much)
Attachment #672743 - Flags: sec-approval?
Attachment #672707 - Attachment mime type: application/octet-stream → application/java-archive
Is it the frame that's used after being freed (which is usually what the poisoned address indicates) or an editor that's used after it was freed? The point of frame poisoning was to turn what could have been an exploitable UAF into a benign "suicide". If we got the frame pointer from a dead editor then that's probably exploitable in some way.
Comment on attachment 672743 [details] [diff] [review]
patch

sec-approval+
Attachment #672743 - Flags: sec-approval? → sec-approval+
Assignee

Comment 10

7 years ago
The crash is about accessing a member variable from a dead nsIFrame and using that.
Assignee

Comment 11

7 years ago
Comment on attachment 672743 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): old regression
User impact if declined: Crashes
Testing completed (on m-c, etc.): about to land m-c 
Risk to taking this patch (and alternatives if risky): Shouldn't be risky
String or UUID changes made by this patch: NA
Attachment #672743 - Flags: approval-mozilla-release?
Attachment #672743 - Flags: approval-mozilla-beta?
Attachment #672743 - Flags: approval-mozilla-aurora?
Assignee

Comment 12

7 years ago
https://hg.mozilla.org/mozilla-central/rev/92e4438ecf5e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment on attachment 672743 [details] [diff] [review]
patch

This was wontfixed for 16 so I'm only approving for branch uplift to Beta/Aurora.
Attachment #672743 - Flags: approval-mozilla-beta?
Attachment #672743 - Flags: approval-mozilla-beta+
Attachment #672743 - Flags: approval-mozilla-aurora?
Attachment #672743 - Flags: approval-mozilla-aurora+
(In reply to Olli Pettay [:smaug] from comment #10)
> The crash is about accessing a member variable from a dead nsIFrame and
> using that.

That doesn't sound exploitable, that's the case frame-poisoning was designed to catch.
Whiteboard: frame-poisoning
Skylined:
> The below files trigger a use-after-free

What tool is reporting that? At the moment it looks like straight-up frame-poisoning, but is it possible the entire frame arena has been freed? That would tip back into "probably exploitable".
Reporter

Comment 17

7 years ago
Are you calling me a tool? :)

I made that conclusion after looking at the code: from what I understand, the poison value should only appear in an access violation if there is a call to free the frame followed by continued use. Even though the memory is not released when free is called, it is technically still a use-after-free. Also, the mitigation does not prevent triggering the vulnerability, it only makes exploitation harder (if not impossible). Hence, I assumed you would want to treat this as a security issue, rather than a stability issues.

As far as I can tell, the mitigation does indeed appear to prevent exploitation, but I am not familiar enough with Firefox internals to determine if there is no way around it. I can only speculate that the frame may have some member properties that could be manipulated to allow manipulation of further data beyond what is considered secure. 

I will defer to you judgment in this case. If you are confident that this is not exploitable, then I would appreciate it you could remove the view restrictions on this bug, so I can blog about this mitigation.
Assignee

Updated

7 years ago
Attachment #672743 - Flags: approval-mozilla-release?

Comment 18

7 years ago
Dan is correct, this is a safe and non-exploitable crash since the frame has been poisoned properly:

(gdb) bt 20
#0  0x00000001038a5170 in nsQueryInterface::operator() (this=0x7fff5fbf8b40, aIID=@0x1052003f8, answer=0x7fff5fbf8b30) at nsCOMPtr.cpp:14
#1  0x00000001019fa133 in nsCOMPtr<nsITextControlElement>::assign_from_qi (this=0x7fff5fbf8c70, qi={mRawPtr = 0x7ffffffff0dea7ff}, aIID=@0x1052003f8) at nsCOMPtr.h:1144
#2  0x00000001019fa0ee in nsCOMPtr<nsITextControlElement>::nsCOMPtr (this=0x7fff5fbf8c70, qi={mRawPtr = 0x7ffffffff0dea7ff}) at nsCOMPtr.h:554
#3  0x00000001019f7b2d in nsCOMPtr<nsITextControlElement>::nsCOMPtr (this=0x7fff5fbf8c70, qi={mRawPtr = 0x7ffffffff0dea7ff}) at nsCOMPtr.h:555
#4  0x00000001019f5d90 in nsTextControlFrame::GetSelectionRange (this=0x118a8fb00, aSelectionStart=0x0, aSelectionEnd=0x0, aDirection=0x7fff5fbf8d24) at /Users/ehsanakhgari/moz/aurora/layout/forms/nsTextControlFrame.cpp:1068
#5  0x00000001019f640f in non-virtual thunk to nsTextControlFrame::GetSelectionRange(int*, int*, nsITextControlFrame::SelectionDirection*) () at /Users/ehsanakhgari/moz/aurora/layout/forms/nsTextControlFrame.cpp:1105
#6  0x000000010214e3e4 in nsHTMLInputElement::GetSelectionDirection (this=0x100538200, aDirection=@0x7fff5fbf8df8) at /Users/ehsanakhgari/moz/aurora/content/html/content/src/nsHTMLInputElement.cpp:3095
#7  0x000000010214e56f in non-virtual thunk to nsHTMLInputElement::GetSelectionDirection(nsAString_internal&) () at /Users/ehsanakhgari/moz/aurora/content/html/content/src/nsHTMLInputElement.cpp:3083
#8  0x0000000102d1da65 in nsIDOMHTMLInputElement_GetSelectionDirection (cx=0x10dabdbc0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf95c8}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf8f48}, vp_={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbf9090}) at dom_quickstubs.cpp:15967
#9  0x0000000104915d77 in js::CallJSPropertyOp (cx=0x10dabdbc0, op=0x102d1d990 <nsIDOMHTMLInputElement_GetSelectionDirection(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)>, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf95c8}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf8f48}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbf9090}) at jscntxtinlines.h:437
#10 0x000000010491a8d4 in js::Shape::get (this=0x10e73a5d8, cx=0x10dabdbc0, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf95c8}, obj=0x10e7234f0, pobj=0x10e723490, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbf9090}) at jsscopeinlines.h:303
#11 0x00000001049076c8 in js_NativeGetInline (cx=0x10dabdbc0, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf95c8}, obj=0x10e7234f0, pobj=0x10e723490, shape=0x10e73a5d8, getHow=1, vp=0x7fff5fbfb4e8) at /Users/ehsanakhgari/moz/aurora/js/src/jsobj.cpp:4228
#12 0x00000001049086d2 in js_GetPropertyHelperInline (cx=0x10dabdbc0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf95c8}, receiver={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf95c8}, id_={asBits = 4830267008}, getHow=1, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfb4e8}) at /Users/ehsanakhgari/moz/aurora/js/src/jsobj.cpp:4382
#13 0x0000000104907d28 in js::GetPropertyHelper (cx=0x10dabdbc0, obj={<js::HandleBase<JSObject *>> = {<No data fields>}, ptr = 0x7fff5fbf95c8}, id={<js::HandleBase<jsid>> = {<No data fields>}, ptr = 0x7fff5fbf9558}, getHow=1, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfb4e8}) at /Users/ehsanakhgari/moz/aurora/js/src/jsobj.cpp:4391
#14 0x00000001048ce503 in js::GetPropertyOperation (cx=0x10dabdbc0, script=0x10e726310, pc=0x116e2a62e "5", lval={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfbcd0}, vp={<js::MutableHandleBase<JS::Value>> = {<js::MutableValueOperations<JS::MutableHandle<JS::Value> >> = {<js::ValueOperations<JS::MutableHandle<JS::Value> >> = {<No data fields>}, <No data fields>}, <No data fields>}, ptr = 0x7fff5fbfb4e8}) at jsinterpinlines.h:283
#15 0x00000001048b578c in js::Interpret (cx=0x10dabdbc0, entryFrame=0x10db83030, interpMode=js::JSINTERP_NORMAL) at /Users/ehsanakhgari/moz/aurora/js/src/jsinterp.cpp:2340
#16 0x00000001048a9a85 in js::RunScript (cx=0x10dabdbc0, script={<js::HandleBase<JSScript *>> = {<No data fields>}, ptr = 0x10e73d7a8}, fp=0x10db83030) at /Users/ehsanakhgari/moz/aurora/js/src/jsinterp.cpp:324
#17 0x00000001048c3177 in js::InvokeKernel (cx=0x10dabdbc0, args={<JS::CallReceiver> = {usedRval_ = false, argv_ = 0x10db83030}, argc_ = 0}, construct=js::NO_CONSTRUCT) at /Users/ehsanakhgari/moz/aurora/js/src/jsinterp.cpp:378
#18 0x00000001049122c3 in js::Invoke (cx=0x10dabdbc0, args=@0x7fff5fbfc0c0, construct=js::NO_CONSTRUCT) at jsinterp.h:109
#19 0x00000001048c37b8 in js::Invoke (cx=0x10dabdbc0, thisv=@0x7fff5fbfc180, fval=@0x7fff5fbfc1e8, argc=0, argv=0x0, rval=0x7fff5fbfc3c8) at /Users/ehsanakhgari/moz/aurora/js/src/jsinterp.cpp:411
(More stack frames follow...)
(gdb) p mRawPtr
$1 = (Cannot access memory at address 0x7ffffffff0dea7ff
(gdb) p/x ARENA_POISON
$2 = 0x7ffffffff0dea7ff
(In reply to SkyLined from comment #17)
> Are you calling me a tool? :)

No, sorry if my comment was taken that way. Many people have been testing Firefox using ASan builds and I mistakenly assumed you were as well. If ASan was reporting our frame arena being freed either that would be a bug in ASan or I was completely misreading the error log and the code near where it crashed (which is quite likely, I have to look at a lot of bugs quickly).

> I made that conclusion after looking at the code: from what I understand,
> the poison value should only appear in an access violation if there is a
> call to free the frame followed by continued use. Even though the memory is
> not released when free is called, it is technically still a use-after-free.

yes and yes

> Also, the mitigation does not prevent triggering the vulnerability, it only
> makes exploitation harder (if not impossible).

yes; we hope closer to the impossible end, but yes.

> Hence, I assumed you would want to treat this as a security issue, rather
> than a stability issues.

We do, and kudos to Olli for jumping right on it. For purposes of the security bug bounty, however, severity matters: I needed to make sure I understood the bug correctly.

> As far as I can tell, the mitigation does indeed appear to prevent
> exploitation, but I am not familiar enough with Firefox internals to
> determine if there is no way around it. I can only speculate that the frame
> may have some member properties that could be manipulated to allow
> manipulation of further data beyond what is considered secure.

They shouldn't, they should be completely stomped. Here's an overview of the approach

http://robert.ocallahan.org/2010/10/mitigating-dangling-pointer-bugs-using_15.html

By-passing frame-poisoning would most definitely be worth a bug bounty, possibly several if you then submit lots of formerly "safe" crashes before we're able to patch up the poisoning mitigation.

> If you are confident that this is not exploitable, then I would
> appreciate it you could remove the view restrictions on this bug,
> so I can blog about this mitigation.

Fair enough.
Group: core-security
Summary: Use-after-free in nsHTMLInputElement → frame-poisoned crash in nsHTMLInputElement
Whiteboard: frame-poisoning

Updated

7 years ago
Target Milestone: --- → mozilla19
This doesn't qualify for a bounty because it is sec-low rated.
Verified as fixed on Firefox 17 RC - there is no crash when loading the test case from the Description.

Checked also the Socorro reports and I couldn't find any crashes after the fix landed.

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121116115405
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0

Verified as fixed on Firefox 18 beta 3 - there is no crash when loading the test case from the description. Checked also the Socorro reports and I couldn't find any crashes after the fix landed.

Setting tracking flag for Firefox 18 to Verified.
QA Contact: simona.marcu
You need to log in before you can comment on or make changes to this bug.