Closed Bug 795804 (CVE-2012-4214) Opened 12 years ago Closed 12 years ago

Heap-use-after-free in nsTextEditorState::PrepareEditor

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Platform:
x86_64
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla19
Tracking Flags:
Tracking Status
firefox17 + fixed
firefox18 + fixed
firefox19 + fixed
firefox-esr10 17+ fixed

People

(Reporter: inferno, Assigned: ayg)

Details

(4 keywords, Whiteboard: [adv-track-main17+][adv-track-esr17+][asan])

Attachments

(4 files)

Testcase
868 bytes, text/html
Details
Patch
3.02 KB, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-aurora+
abillings
: sec-approval+
Details | Diff | Splinter Review
Patch backported to beta
11.45 KB, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-beta+
akeybl
: approval-mozilla-esr10+
Details | Diff | Splinter Review
Patch backported to ESR10
14.54 KB, patch
ehsan.akhgari
: review+
akeybl
: approval-mozilla-esr10+
Details | Diff | Splinter Review
Attached file Testcase — 
Reproduces on trunk. Run a few firefox instances simultaneously for reproducing reliabily.

=================================================================
==19969== ERROR: AddressSanitizer heap-use-after-free on address 0x7f310f2083b0 at pc 0x7f313ddafc7c bp 0x7fffb5d04130 sp 0x7fffb5d04128
READ of size 8 at 0x7f310f2083b0 thread T0
    #0 0x7f313ddafc7b in nsTextEditorState::PrepareEditor(nsAString_internal const*) src/content/html/content/src/nsTextEditorState.cpp:1125
    #1 0x7f313ddcd603 in PrepareEditorEvent::Run() src/content/html/content/src/nsTextEditorState.cpp:1026
    #2 0x7f313cad7c4c in nsContentUtils::RemoveScriptBlocker() src/content/base/src/nsContentUtils.cpp:5021
    #3 0x7f313b04835e in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2275
    #4 0x7f313b0320a6 in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2274
    #5 0x7f313b43fc91 in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3834
    #6 0x7f313bb77232 in nsHideViewer::Run() src/layout/generic/nsSubDocumentFrame.cpp:782
    #7 0x7f313cad7c4c in nsContentUtils::RemoveScriptBlocker() src/content/base/src/nsContentUtils.cpp:5021
    #8 0x7f313b04835e in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2275
    #9 0x7f313b0320a6 in ~nsAutoScriptBlocker src/../../../../dist/include/nsContentUtils.h:2274
    #10 0x7f313ccf2af5 in nsDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) src/content/base/src/nsDocument.cpp:6371
    #11 0x7f313e668b5c in nsHTMLDocument::AdoptNode(nsIDOMNode*, nsIDOMNode**) src/content/html/document/src/nsHTMLDocument.h:87
    #12 0x7f3141f73cd6 in nsIDOMDocument_AdoptNode(JSContext*, unsigned int, JS::Value*) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:3452
    #13 0x7f314d3dcd9f in js::CallJSNative(JSContext*, int (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/jscntxtinlines.h:370
    #14 0x7f314d37eb79 in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2460
    #15 0x7f314d2caaee in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) src/js/src/jsinterp.cpp:324
    #16 0x7f314d3ea566 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) src/js/src/jsinterp.cpp:509
    #17 0x7f314d3ec2fb in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) src/js/src/jsinterp.cpp:546
    #18 0x7f314caff289 in JS::Evaluate(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long, JS::Value*) src/js/src/jsapi.cpp:5679
    #19 0x7f313ee16dee in nsJSContext::EvaluateString(nsAString_internal const&, JSObject*, nsIPrincipal*, nsIPrincipal*, char const*, unsigned int, JSVersion, nsAString_internal*, bool*) src/dom/base/nsJSEnvironment.cpp:1506
    #20 0x7f313efcfb76 in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) src/dom/base/nsGlobalWindow.cpp:9621
    #21 0x7f313ef876c4 in nsGlobalWindow::RunTimeout(nsTimeout*) src/dom/base/nsGlobalWindow.cpp:9880
    #22 0x7f313efcda28 in nsGlobalWindow::TimerCallback(nsITimer*, void*) src/dom/base/nsGlobalWindow.cpp:10147
    #23 0x7f3146b44972 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:473
    #24 0x7f3146b45e7a in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:556
    #25 0x7f3146b09580 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:612
    #26 0x7f314679becb in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #27 0x7f31451e53b6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #28 0x7f3146dc1e11 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #29 0x7f3146dc1c46 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #30 0x7f3146dc1b2b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #31 0x7f314468cdda in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #32 0x7f31432bf9b4 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #33 0x7f3139932a4d in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3782
    #34 0x7f31399388c5 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3848
    #35 0x7f313993b774 in XRE_main src/toolkit/xre/nsAppRunner.cpp:3923
    #36 0x40d013 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #37 0x40a755 in main src/browser/app/nsBrowserApp.cpp:279
    #38 0x7f3157830c4c in ?? ??:0
0x7f310f2083b0 is located 48 bytes inside of 120-byte region [0x7f310f208380,0x7f310f2083f8)
freed by thread T0 here:
    #0 0x4c4af0 in free ??:0
    #1 0x7f31546bc586 in moz_free src/memory/mozalloc/mozalloc.cpp:51
    #2 0x7f313e05e4a6 in operator delete(void*) src/../../../../dist/include/mozilla/mozalloc.h:224
    #3 0x7f313e06b9b9 in nsHTMLInputElement::HandleTypeChange(unsigned char) src/content/html/content/src/nsHTMLInputElement.cpp:2621
    #4 0x7f313e09a2f4 in nsHTMLInputElement::ParseAttribute(int, nsIAtom*, nsAString_internal const&, nsAttrValue&) src/content/html/content/src/nsHTMLInputElement.cpp:2751
    #5 0x7f313cece403 in nsGenericElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/base/src/nsGenericElement.cpp:1952
    #6 0x7f313dd28b8d in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, bool) src/content/html/content/src/nsGenericHTMLElement.cpp:1994
    #7 0x7f313dd0c5a3 in nsGenericHTMLElement::SetAttr(int, nsIAtom*, nsAString_internal const&, bool) src/content/html/document/src/../../content/src/nsGenericHTMLElement.h:175
    #8 0x7f313dd0e4b7 in nsGenericHTMLElement::SetAttrHelper(nsIAtom*, nsAString_internal const&) src/content/html/content/src/nsGenericHTMLElement.cpp:2863
    #9 0x7f313e0772d0 in nsHTMLInputElement::SetType(nsAString_internal const&) src/content/html/content/src/nsHTMLInputElement.cpp:906
    #10 0x7f313e07733e in non-virtual thunk to nsHTMLInputElement::SetType(nsAString_internal const&) src/gfx/cairo/cairo/src/cairo-surface-subsurface.c:0
    #11 0x7f3142620241 in nsIDOMHTMLInputElement_SetType(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) src/objdir-ff-asan-sym/js/xpconnect/src/dom_quickstubs.cpp:15518
    #12 0x7f314d6bf785 in js::CallJSPropertyOpSetter(JSContext*, int (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>), JS::Handle<JSObject*>, JS::Handle<long>, int, JS::MutableHandle<JS::Value>) src/js/src/jscntxtinlines.h:456
    #13 0x7f314d6f6056 in js::baseops::SetPropertyHelper(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, unsigned int, JS::MutableHandle<JS::Value>, int) src/js/src/jsobj.cpp:4592
    #14 0x7f314d423bf6 in js::SetPropertyOperation(JSContext*, unsigned char*, JS::Handle<JS::Value>, JS::Handle<JS::Value>) src/js/src/jsinterpinlines.h:355
    #15 0x7f314d36ed8e in js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) src/js/src/jsinterp.cpp:2370
    #16 0x7f314d2caaee in js::RunScript(JSContext*, JS::Handle<JSScript*>, js::StackFrame*) src/js/src/jsinterp.cpp:324
    #17 0x7f314d3dd590 in js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) src/js/src/jsinterp.cpp:378
    #18 0x7f314cc39f44 in js::Invoke(JSContext*, js::InvokeArgsGuard&, js::MaybeConstruct) src/js/src/jsinterp.h:109
    #19 0x7f314d3e29e0 in js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value*, JS::Value*) src/js/src/jsinterp.cpp:411
    #20 0x7f314cb0c356 in JS_CallFunctionValue src/js/src/jsapi.cpp:5860
    #21 0x7f313ee25356 in nsJSContext::CallEventHandler(nsISupports*, JSObject*, JSObject*, nsIArray*, nsIVariant**) src/dom/base/nsJSEnvironment.cpp:1907
    #22 0x7f313f68fc68 in nsJSEventListener::HandleEvent(nsIDOMEvent*) src/dom/src/events/nsJSEventListener.cpp:212
    #23 0x7f313da2fecb in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:845
    #24 0x7f313da31648 in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.cpp:918
    #25 0x7f313dbc773f in nsEventListenerManager::HandleEvent(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) src/content/events/src/nsEventListenerManager.h:143
    #26 0x7f313dbb72de in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:182
    #27 0x7f313dbb5215 in nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) src/content/events/src/nsEventDispatcher.cpp:313
    #28 0x7f313dbbb569 in nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) src/content/events/src/nsEventDispatcher.cpp:629
    #29 0x7f313b255602 in DocumentViewerImpl::LoadComplete(tag_nsresult) src/layout/base/nsDocumentViewer.cpp:1024
previously allocated by thread T0 here:
    #0 0x4c4bb0 in __interceptor_malloc ??:0
    #1 0x7f31546bc6da in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:57
    #2 0x7f313e05d2ca in operator new(unsigned long) src/../../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7f313e05c040 in NS_NewHTMLInputElement(already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/content/src/nsHTMLInputElement.cpp:537
    #4 0x7f313e5e54af in CreateHTMLElement(unsigned int, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/document/src/nsHTMLContentSink.cpp:497
    #5 0x7f313e5e5d0a in NS_NewHTMLElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/html/document/src/nsHTMLContentSink.cpp:480
    #6 0x7f313cfa66b1 in NS_NewElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/base/src/nsNameSpaceManager.cpp:201
    #7 0x7f314017055a in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) src/parser/html/nsHtml5TreeOperation.cpp:340
    #8 0x7f314018fb75 in nsHtml5TreeOpExecutor::RunFlushLoop() src/parser/html/nsHtml5TreeOpExecutor.cpp:564
    #9 0x7f31401cc259 in nsHtml5ExecutorFlusher::Run() src/parser/html/nsHtml5StreamParser.cpp:127
    #10 0x7f3146b09580 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:612
    #11 0x7f314679becb in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:220
    #12 0x7f31451e53b6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #13 0x7f3146dc1e11 in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:208
    #14 0x7f3146dc1c46 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:201
    #15 0x7f3146dc1b2b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:175
    #16 0x7f314468cdda in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #17 0x7f31432bf9b4 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #18 0x7f3139932a4d in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3782
    #19 0x7f31399388c5 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3848
    #20 0x7f313993b774 in XRE_main src/toolkit/xre/nsAppRunner.cpp:3923
    #21 0x40d013 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #22 0x40a755 in main src/browser/app/nsBrowserApp.cpp:279
Shadow byte and word:
  0x1fe621e41076: fd
  0x1fe621e41070: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fe621e41050: 00 00 00 00 00 00 00 00
  0x1fe621e41058: fb fb fb fb fb fb fb fb
  0x1fe621e41060: fa fa fa fa fa fa fa fa
  0x1fe621e41068: fa fa fa fa fa fa fa fa
=>0x1fe621e41070: fd fd fd fd fd fd fd fd
  0x1fe621e41078: fd fd fd fd fd fd fd fd
  0x1fe621e41080: fa fa fa fa fa fa fa fa
  0x1fe621e41088: fa fa fa fa fa fa fa fa
  0x1fe621e41090: 00 00 00 fb fb fb fb fb
Stats: 254M malloced (296M for red zones) by 516614 calls
Stats: 42M realloced by 24355 calls
Stats: 221M freed by 290089 calls
Stats: 87M really freed by 207706 calls
Stats: 484M (123993 full pages) mmaped in 121 calls
  mmaps   by size class: 8:311277; 9:32764; 10:8190; 11:14329; 12:2048; 13:1536; 14:1280; 15:384; 16:1024; 17:1248; 18:144; 19:40; 20:28;
  mallocs by size class: 8:448277; 9:33777; 10:8842; 11:17027; 12:2365; 13:1758; 14:1483; 15:342; 16:1199; 17:1317; 18:159; 19:42; 20:26;
  frees   by size class: 8:238380; 9:24927; 10:5659; 11:13922; 12:1543; 13:1467; 14:1292; 15:301; 16:1126; 17:1300; 18:110; 19:39; 20:23;
  rfrees  by size class: 8:184498; 9:8844; 10:2132; 11:9156; 12:663; 13:494; 14:464; 15:181; 16:934; 17:311; 18:24; 19:4; 20:1;
Stats: malloc large: 1544 small slow: 2450
==19969== ABORTING
Component: General → Editor
Product: Firefox → Core
Aryeh, can you please take a stab at this?  Thanks!
Assignee: nobody → ayg
Can't reproduce on linux. I didn't use ASAN build, but added some manual checks.
Anyhow, nsTextEditorState &mState; is a guaranteed security problem in a runnable.
(In reply to Olli Pettay [:smaug] from comment #3)
> Can't reproduce on linux. I didn't use ASAN build, but added some manual
> checks.
> Anyhow, nsTextEditorState &mState; is a guaranteed security problem in a
> runnable.

ASAN build is broken again :(, so can't test on trunk. ccing Christian. I tested using 
20121005051921
http://hg.mozilla.org/mozilla-central/rev/3b458f4e0f42
and it reproduces nicely with multiple simultaneous firefox instances (like 10) started at once with the testcase path on command line.
I was able to compile ASAN trunk by disabling all warnings. Then i checked the testcase and it still reliably crashes for me with multiple firefox instances started simultaneously.
Keywords: csec-uaf
I can't reproduce a crash on current trunk with a bunch of refreshing (Linux 32-bit).  But it looks pretty simple -- nsTextEditorState is not reference-counted, and it gets deleted while PrepareEditorEvent holds a reference.  I guess the right way to do this is make it reference-counted.  Ehsan, should I do this similarly to bug 771994?  I can't currently test that it fixes the problem, because I can't reproduce with a local build.
This reproduces a day ago easily with a python script like this and using a nightly build from https://people.mozilla.com/~choller/firefox/asan/

import os
import time

thread_num = 10

for i in range(0, thread_num):
  profile_dir = '/tmp/firefox_profile_%d' % i
  os.system('mkdir %s' % profile_dir)
  os.system('touch %s/prefs.js')
  cmd = '/path_to_firefox/firefox-bin -no-remote -profile "%s" "test.html" 2>&1 | tee /tmp/%d.log &' % (profile_dir, i)
  os.system(cmd)
  time.sleep(0.3)
(In reply to :Aryeh Gregor from comment #6)
> I can't reproduce a crash on current trunk with a bunch of refreshing (Linux
> 32-bit).  But it looks pretty simple -- nsTextEditorState is not
> reference-counted, and it gets deleted while PrepareEditorEvent holds a
> reference.  I guess the right way to do this is make it reference-counted. 
> Ehsan, should I do this similarly to bug 771994?  I can't currently test
> that it fixes the problem, because I can't reproduce with a local build.

So we don't really need to make nsTextEditorState refcounted, since it's not really supposed to be owned by multiple things.  What we should do here is to use a WeakPtr to nsTextEditorState in PrepareEditorEvent and just check to make sure it's alive before using it (since the stuff that PrepareEditorEvent does are not important if something has destroyed the nsTextEditorState object.
(You can use this weakptr implementation: http://mxr.mozilla.org/mozilla-central/source/mfbt/WeakPtr.h.  We can backport it to branches.)
Attached patch Patch — — Splinter Review 
Compiles locally.  I can't verify it fixes the issue because I couldn't reproduce it on my own compiled build -- I didn't try the ASAN builds because it wouldn't help me verify that I fixed it.  But in theory it should.

Would you like a separate patch for beta that backports WeakPtr?  It was only added on Thursday, but that means it should already be in Aurora.
Attachment #669620 - Flags: review?(ehsan)
Comment on attachment 669620 [details] [diff] [review]
Patch

Review of attachment 669620 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, 100% sure this fixes it.  Thanks!
Attachment #669620 - Flags: review?(ehsan) → review+
(In reply to :Aryeh Gregor from comment #10)
> Would you like a separate patch for beta that backports WeakPtr?  It was
> only added on Thursday, but that means it should already be in Aurora.

That would be great, yes.
Comment on attachment 669620 [details] [diff] [review]
Patch

[Security approval request comment]
How easily can the security issue be deduced from the patch? It's perfectly evident that this fix prevents against a pointer to a deleted object to be used.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  It shouldn't be immediately obvious on how to trigger this security bug.  The patch does not accompany tests.

Which older supported branches are affected by this flaw? All of them.

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? The backport should be easy to create.  We should just import the WeakPtr implementation.

How likely is this patch to cause regressions; how much testing does it need? It is very unlikely to cause regressions.  The fix is very small and well understood.
Attachment #669620 - Flags: sec-approval?
This backports WeakPtr.h, and NullPtr.h as well because WeakPtr.h depends on it.  Fortunately, this was easy because of how self-contained mfbt is.  It compiles locally, but I didn't push to try.  Again, I didn't test it because I couldn't reproduce locally anyway.
Attachment #669874 - Flags: review?(ehsan)
Attachment #669874 - Flags: review?(ehsan) → review+
Should we request branch approval before or after getting a sec-approval+?
Presumably this affects ESR-10 as well? Hard to test or verify because we've never gotten ASan to build on that branch.
status-firefox-esr10: --- → affected
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox17: --- → +
tracking-firefox18: --- → +
tracking-firefox19: --- → +
Ehsan, you should ask sec-approval first since we should land on trunk before going elsewhere.
Attachment #669620 - Flags: sec-approval? → sec-approval+
(In reply to Daniel Veditz [:dveditz] from comment #16)
> Presumably this affects ESR-10 as well? Hard to test or verify because we've
> never gotten ASan to build on that branch.

The buggy code exists there so I have all reasons to believe that ESR is affected as well.
Comment on attachment 669620 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 534785 (the first time this code got into the tree)
User impact if declined: sec-critical
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): minimal
String or UUID changes made by this patch: none
Attachment #669620 - Flags: approval-mozilla-aurora?
Attachment #669874 - Flags: approval-mozilla-esr10?
Attachment #669874 - Flags: approval-mozilla-beta?
https://hg.mozilla.org/mozilla-central/rev/ea3394d0280c
Status: NEW → RESOLVED
Closed: 12 years ago
status-firefox19: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 669620 [details] [diff] [review]
Patch

Low risk sg:crit fix, approving for all branches.
Attachment #669620 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #669874 - Flags: approval-mozilla-esr10?
Attachment #669874 - Flags: approval-mozilla-esr10+
Attachment #669874 - Flags: approval-mozilla-beta?
Attachment #669874 - Flags: approval-mozilla-beta+
Backed out of ESR for build failures: https://tbpl.mozilla.org/?tree=Mozilla-Esr10&rev=7797a9612159

https://hg.mozilla.org/releases/mozilla-esr10/rev/d61843854f33

Aryeh, can you please look into why the build failed?  My suspicion is that the mfbt setup in ESR is very different, and we need to expose the added headers some other way.  Waldo might know how.
status-firefox-esr10: fixed → affected
Um, how did the patch even apply to ESR10?  There's no file mfbt/exported_headers.mk, so it should have failed.  mfbt/Makefile.in was created only in bug 711775, last December.  Before that, it looks like js/src/Makefile.in is what you need to change.  I also had to backport TypeTraits.h for ESR10, and remove the #include "mozilla/Assertions.h" (since MOZ_ASSERT was in mozilla/Util.h at that point).  I also opted to remove a use of MOZ_STATIC_ASSERT instead of backporting the function.  After that it still didn't compile, but Ms2ger was kind enough to suggest adding "using namespace mozilla;", and then it compiled.  I didn't test beyond building the tree.
Attachment #671190 - Flags: review?(ehsan)
Comment on attachment 671190 [details] [diff] [review]
Patch backported to ESR10

Review of attachment 671190 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!  ESR is showing that it's dated... :-)
Attachment #671190 - Flags: review?(ehsan)
Attachment #671190 - Flags: review+
Attachment #671190 - Flags: approval-mozilla-esr10?
Keywords: testcase, verifyme
Attachment #671190 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
The esr10 mfbt-addition details look fine in the newer patch...modulo mfbt/tests likely not existing there, and the test file itself likely not compiling (due to MOZ_ASSERT use) there.  I'd have omitted the test file, but whatever.  It's certainly not worth backporting the mfbt test hookup stuff to esr10.  :-)
Whiteboard: [adv-track-main17+]
Whiteboard: [adv-track-main17+] → [adv-track-main17+][adv-track-esr17+]
Alias: CVE-2012-4214
Flags: sec-bounty+
Whiteboard: [adv-track-main17+][adv-track-esr17+] → [adv-track-main17+][adv-track-esr17+][asan]
Group: core-security
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: