Closed
Bug 802985
Opened 12 years ago
Closed 12 years ago
frame-poisoned crash in nsHTMLInputElement
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: abbGZcvu_bugzilla.mozilla.org, Assigned: smaug)
References
(Blocks 1 open bug)
Details
(4 keywords)
Attachments
(3 files, 2 obsolete files)
|
(deleted),
application/java-archive
|
Details | |
|
(deleted),
patch
|
ehsan.akhgari
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
|
(deleted),
patch
|
Details | Diff | Splinter Review |
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
|
||
Updated•12 years ago
|
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
|
Assignee | |
Comment 1•12 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•12 years ago
|
||
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•12 years ago
|
||
|
|
Assignee | |
Updated•12 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
|
|
Assignee | |
Comment 4•12 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•12 years ago
|
||
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•12 years ago
|
||
Attachment #672719 -
Attachment is obsolete: true
|
||
Updated•12 years ago
|
Attachment #672743 -
Flags: review?(ehsan) → review+
|
|
Assignee | |
Comment 7•12 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?
|
||
Updated•12 years ago
|
status-firefox-esr10:
--- → affected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox-esr17:
--- → affected
Keywords: sec-critical
|
||
Updated•12 years ago
|
Attachment #672707 -
Attachment mime type: application/octet-stream → application/java-archive
|
|
||
Comment 8•12 years ago
|
||
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.
status-firefox-esr17:
affected → ---
|
|
||
Comment 9•12 years ago
|
||
Comment on attachment 672743 [details] [diff] [review]
patch
sec-approval+
Attachment #672743 -
Flags: sec-approval? → sec-approval+
|
|
Assignee | |
Comment 10•12 years ago
|
||
The crash is about accessing a member variable from a dead nsIFrame and using that.
|
|
Assignee | |
Comment 11•12 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•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
|
||
Comment 13•12 years ago
|
||
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+
|
|
||
Comment 14•12 years ago
|
||
(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.
Keywords: csec-uaf,
sec-critical
Whiteboard: frame-poisoning
|
|
||
Comment 15•12 years ago
|
||
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 16•12 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•12 years ago
|
Attachment #672743 -
Flags: approval-mozilla-release?
|
|
||
Comment 17•12 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
|
|
||
Comment 18•12 years ago
|
||
(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
|
|
Assignee | |
Comment 19•12 years ago
|
||
|
|
||
Updated•12 years ago
|
|
||
Updated•12 years ago
|
Blocks: PoisonFrameCrash
|
||
Updated•12 years ago
|
Target Milestone: --- → mozilla19
|
||
Updated•12 years ago
|
Attachment #673706 -
Attachment description: Bounty Nomination → Bounty Non-qual
|
|
||
Comment 20•12 years ago
|
||
This doesn't qualify for a bounty because it is sec-low rated.
|
||
Comment 21•12 years ago
|
||
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
|
|
||
Comment 22•12 years ago
|
||
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.
Description
•