Last Comment Bug 787704 - (CVE-2012-3990) use-after-free in nsIContent::GetNameSpaceID
(CVE-2012-3990)
: use-after-free in nsIContent::GetNameSpaceID
Status: RESOLVED FIXED
[asan][advisory-tracking+]
: crash, csectype-uaf, sec-critical, testcase
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: Menus (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla18
Assigned To: Olli Pettay [:smaug]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-09-01 11:59 PDT by miaubiz
Modified: 2014-07-24 13:44 PDT (History)
9 users (show)
dveditz: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
+
fixed
16+
fixed


Attachments
repro case (1.20 KB, text/html)
2012-09-01 11:59 PDT, miaubiz
no flags Details
asan log linux nightly (6.29 KB, text/plain)
2012-09-01 11:59 PDT, miaubiz
no flags Details
asan log linux aurora (6.29 KB, text/plain)
2012-09-01 11:59 PDT, miaubiz
no flags Details
patch (1.96 KB, patch)
2012-09-01 16:22 PDT, Olli Pettay [:smaug]
masayuki: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
for branches with nsnull (1.96 KB, patch)
2012-09-03 00:22 PDT, Olli Pettay [:smaug]
akeybl: approval‑mozilla‑beta+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description miaubiz 2012-09-01 11:59:04 PDT
Created attachment 657590 [details]
repro case

when I load:

<html>
  <head>
    <style>
      .c0 { content:'A' } 
    </style>
    <script>
      onload = function() {
        document.body.appendChild(document.createElement('td'))
        el0=document.createElement('div')
        document.body.appendChild(el0)
        el1=document.createElement('form')
        document.body.appendChild(el1)
        el2=document.createElement('input')
        el2.setAttribute('type', 'submit')
        el1.appendChild(el2)
        el3=document.createElement('input')
        el3.setAttribute('required', 'A')
        el1.appendChild(el3)  
        el4=document.createElement('input')
        el4.type='file'
        el1.appendChild(el4)
        setTimeout(function() {
          el4.focus()
          setTimeout(function() {
            el4.click()
            el2.setAttribute('required', 'A')
            el0.setAttribute('class', 'c0')
            setTimeout(function() {
              document.designMode='on'
              document.execCommand('italic')
              document.designMode='off'
              setTimeout(function() {
                el2.click()
              }, 100)
            }, 100)
          }, 100)
        }, 100)
      }
    </script>
  </head>
  <body>
  </body>
</html>

I get a crash like so:

==24180== ERROR: AddressSanitizer heap-use-after-free on address 0x7fffbe11ac98 at pc 0x7fffeeb97d74 bp 0x7ffffffedd70 sp 0x7ffffffedd68
READ of size 8 at 0x7fffbe11ac98 thread T0
    #0 0x7fffeeb97d74 in nsIContent::GetNameSpaceID() const /builds/slave/try-lnx64/build/../../../dist/include/nsINodeInfo.h:143
    #1 0x7fffee6f9a37 in nsCOMPtr<nsIDOMEventTarget>::operator=(nsCOMPtr<nsIDOMEventTarget> const&) /builds/slave/try-lnx64/build/../../../../dist/include/nsCOMPtr.h:614
    #2 0x7fffee6fe5fb in nsXULPopupManager::ShowPopupCallback(nsIContent*, nsMenuPopupFrame*, bool, bool) /builds/slave/try-lnx64/build/layout/xul/base/src/nsXULPopupManager.cpp:716
    #3 0x7fffee6fc7a2 in nsXULPopupManager::FirePopupShowingEvent(nsIContent*, bool, bool) /builds/slave/try-lnx64/build/layout/xul/base/src/nsXULPopupManager.cpp:1181
    #4 0x7fffee6fcf36 in nsXULPopupManager::ShowPopup(nsIContent*, nsIContent*, nsAString_internal const&, int, int, bool, bool, bool, nsIDOMEvent*) /builds/slave/try-lnx64/build/layout/xul/base/src/nsXULPopupManager.cpp:568
    #5 0x7fffee691040 in non-virtual thunk to nsPopupBoxObject::OpenPopup(nsIDOMElement*, nsAString_internal const&, int, int, bool, bool, nsIDOMEvent*) /builds/slave/try-lnx64/build/../../../../dist/include/nsCOMPtr.h:407
0x7fffbe11ac98 is located 24 bytes inside of 368-byte region [0x7fffbe11ac80,0x7fffbe11adf0)
freed by thread T0 here:
    #0 0x42ae21 in free ??:0
    #1 0x7fffee92a335 in nsNodeUtils::LastRelease(nsINode*) /builds/slave/try-lnx64/build/content/base/src/nsNodeUtils.cpp:261
    #2 0x7fffee9ff963 in mozilla::dom::FragmentOrElement::Release() /builds/slave/try-lnx64/build/content/base/src/FragmentOrElement.cpp:1853
    #3 0x7fffef02ebc4 in nsGlobalWindow::SetFocusedNode(nsIContent*, unsigned int, bool) /builds/slave/try-lnx64/build/dom/base/nsGlobalWindow.cpp:7765
    #4 0x7fffeefc93be in nsFocusManager::SetFocusInner(nsIContent*, int, bool, bool) 

aurora and nightly affected atleast.
Comment 1 miaubiz 2012-09-01 11:59:28 PDT
Created attachment 657591 [details]
asan log linux nightly
Comment 2 miaubiz 2012-09-01 11:59:47 PDT
Created attachment 657592 [details]
asan log linux aurora
Comment 3 Olli Pettay [:smaug] 2012-09-01 14:52:45 PDT
We end up having sContent pointing to a deleted object.
sPresContext looks scary too, and sTextStateObserver.

Masayuki, could you look at this? If not, I will.
Comment 4 Olli Pettay [:smaug] 2012-09-01 14:55:29 PDT
Hmm, I guess sPresContext is ok and sTextStateObserver is actually refcounted.
Comment 5 Olli Pettay [:smaug] 2012-09-01 15:02:58 PDT
Actually, I could take this.
Comment 6 Olli Pettay [:smaug] 2012-09-01 15:15:58 PDT
Or no :) My initial approach doesn't seem to work.
Comment 7 Olli Pettay [:smaug] 2012-09-01 16:22:39 PDT
Created attachment 657613 [details] [diff] [review]
patch
Comment 8 Olli Pettay [:smaug] 2012-09-01 16:24:24 PDT
Comment on attachment 657613 [details] [diff] [review]
patch

IIRC static nsCOMPtrs should be avoided, so I used manual refcounting here.
Also, since sContent is released when prescontext goes away, there
shouldn't be leaks.
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-09-01 21:35:54 PDT
Comment on attachment 657613 [details] [diff] [review]
patch

This patch looks okay for me.

However, I don't understand why does the testcase in comment 0 cause such case. Looks like el4.focus() caused releasing the content, why?
Comment 10 Olli Pettay [:smaug] 2012-09-03 00:15:55 PDT
It goes something like: Focus would be in native anon button of input type file when designMode = on reframes the element.
Comment 11 Olli Pettay [:smaug] 2012-09-03 00:22:15 PDT
Created attachment 657793 [details] [diff] [review]
for branches with nsnull
Comment 12 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-09-03 01:26:43 PDT
(In reply to Olli Pettay [:smaug] from comment #10)
> It goes something like: Focus would be in native anon button of input type
> file when designMode = on reframes the element.

When it's reframed, all of the anon contents will be recreated?
Comment 13 Olli Pettay [:smaug] 2012-09-03 03:18:32 PDT
Yes.
Comment 14 Masayuki Nakano [:masayuki] (Mozilla Japan) 2012-09-03 06:47:58 PDT
Thanks, I understand.
Comment 16 Andrew McCreight [:mccr8] 2012-09-13 13:51:20 PDT
Olli, is this ready to be landed?
Comment 17 Olli Pettay [:smaug] 2012-09-13 17:00:47 PDT
(In reply to Andrew McCreight [:mccr8] from comment #16)
> Olli, is this ready to be landed?

Yes, but wasn't sure whether I should land it immediately.
Comment 18 Olli Pettay [:smaug] 2012-09-13 17:01:00 PDT
I'll try to remember to land it tomorrow.
Comment 19 Alex Keybl [:akeybl] 2012-09-19 17:57:30 PDT
(In reply to Olli Pettay [:smaug] from comment #18)
> I'll try to remember to land it tomorrow.

Now's the time. Please land and nominate for uplift.
Comment 20 Olli Pettay [:smaug] 2012-09-20 02:18:07 PDT
Comment on attachment 657793 [details] [diff] [review]
for branches with nsnull

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Security sensitive crash
Testing completed (on m-c, etc.): tested locally. Will land m-c real soon
Risk to taking this patch (and alternatives if risky): May keep an element
longer than before, so that could increase memory usage temporarily.
String or UUID changes made by this patch: NA
Comment 21 Olli Pettay [:smaug] 2012-09-20 02:18:29 PDT
Comment on attachment 657613 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Security sensitive crash
Testing completed (on m-c, etc.): tested locally. Will land m-c real soon
Risk to taking this patch (and alternatives if risky): May keep an element
longer than before, so that could increase memory usage temporarily.
String or UUID changes made by this patch: NA
Comment 22 Olli Pettay [:smaug] 2012-09-20 11:03:42 PDT
https://hg.mozilla.org/mozilla-central/rev/f8d30ea0974c
Comment 23 Alex Keybl [:akeybl] 2012-09-20 15:46:54 PDT
Comment on attachment 657613 [details] [diff] [review]
patch

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.
Comment 24 Alex Keybl [:akeybl] 2012-09-21 15:53:29 PDT
We'll approve on Monday if there haven't been any Nightly/Aurora regressions (please land on Aurora asap).
Comment 25 Andrew McCreight [:mccr8] 2012-09-22 10:21:38 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/33c5c2cccda3
Comment 26 Olli Pettay [:smaug] 2012-09-22 10:50:20 PDT
Thanks for landing to Aurora.
(I was going to do it later today.)
Comment 27 Alex Keybl [:akeybl] 2012-09-24 15:31:50 PDT
Comment on attachment 657793 [details] [diff] [review]
for branches with nsnull

[Triage Comment]
Approving for Beta (please land no later than tomorrow morning PT) and ESR10.
Comment 29 Tracy Walker [:tracy] 2014-01-10 10:39:28 PST
mass remove verifyme requests greater than 4 months old

Note You need to log in before you can comment on or make changes to this bug.