Closed Bug 1454126 Opened 3 years ago Closed 3 years ago

crash at null in [@ nsMappedAttributes::GetAttr]


(Core :: DOM: Editor, defect, P1)




Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 + fixed
firefox61 + fixed


(Reporter: tsmith, Assigned: masayuki)


(Blocks 1 open bug)


(5 keywords, Whiteboard: [adv-main60+])


(3 files, 4 obsolete files)

Attached file testcase.html
==83696==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f2bf39babcf bp 0x7fff3b755170 sp 0x7fff3b755170 T0)
==83696==The signal is caused by a READ memory access.
==83696==Hint: address points to the zero page.
    #0 0x7f2bf39babce in nsMappedAttributes::GetAttr(nsAtom*) const src/dom/base/nsMappedAttributes.cpp:183:28
    #1 0x7f2bf835819b in AttrValueIs src/obj-firefox/dist/include/mozilla/dom/Element.h:2008:46
    #2 0x7f2bf835819b in IsMozEditorBogusNode src/obj-firefox/dist/include/mozilla/EditorBase.h:1073
    #3 0x7f2bf835819b in mozilla::TextEditRules::CreateBogusNodeIfNeeded(mozilla::dom::Selection*) src/editor/libeditor/TextEditRules.cpp:1435
    #4 0x7f2bf8357ab1 in mozilla::TextEditRules::Init(mozilla::TextEditor*) src/editor/libeditor/TextEditRules.cpp:143:17
    #5 0x7f2bf836686a in mozilla::TextEditor::EndEditorInit() src/editor/libeditor/TextEditor.cpp:208:17
    #6 0x7f2bf8367958 in ~AutoEditInitRulesTrigger src/editor/libeditor/TextEditUtils.cpp:93:28
    #7 0x7f2bf8367958 in mozilla::TextEditor::Init(nsIDocument&, mozilla::dom::Element*, nsISelectionController*, unsigned int, nsTSubstring<char16_t> const&) src/editor/libeditor/TextEditor.cpp:142
    #8 0x7f2bf6849ab0 in nsTextEditorState::PrepareEditor(nsTSubstring<char16_t> const*) src/dom/html/nsTextEditorState.cpp:1414:25
    #9 0x7f2bf8b81cef in nsTextControlFrame::EnsureEditorInitialized() src/layout/forms/nsTextControlFrame.cpp:317:28
    #10 0x7f2bf66ba683 in mozilla::dom::HTMLInputElement::GetEventTargetParent(mozilla::EventChainPreVisitor&) src/dom/html/HTMLInputElement.cpp:3429:25
    #11 0x7f2bf6436cd8 in mozilla::EventTargetChainItem::GetEventTargetParent(mozilla::EventChainPreVisitor&) src/dom/events/EventDispatcher.cpp:425:12
    #12 0x7f2bf643a9af in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) src/dom/events/EventDispatcher.cpp:894:19
    #13 0x7f2bf89b9645 in mozilla::ScrollFrameHelper::FireScrollEvent() src/layout/generic/nsGfxScrollFrame.cpp:4972:5
    #14 0x7f2bf89b9034 in mozilla::ScrollFrameHelper::ScrollEvent::Run() src/layout/generic/nsGfxScrollFrame.cpp:4925:14
    #15 0x7f2bf85ae73b in nsRefreshDriver::DispatchScrollEvents() src/layout/base/nsRefreshDriver.cpp:1289:12
    #16 0x7f2bf85b1242 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1893:7
    #17 0x7f2bf85c1310 in TickDriver src/layout/base/nsRefreshDriver.cpp:337:13
    #18 0x7f2bf85c1310 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:307
    #19 0x7f2bf85c0ec4 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:328:5
    #20 0x7f2bf85c3c4e in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:770:5
    #21 0x7f2bf85c3c4e in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:683
    #22 0x7f2bf85becf9 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:529:20
    #23 0x7f2bf0777c89 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1096:14
    #24 0x7f2bf07936c0 in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
    #25 0x7f2bf165c74a in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
    #26 0x7f2bf15b1289 in RunInternal src/ipc/chromium/src/base/
    #27 0x7f2bf15b1289 in RunHandler src/ipc/chromium/src/base/
    #28 0x7f2bf15b1289 in MessageLoop::Run() src/ipc/chromium/src/base/
    #29 0x7f2bf805fd3a in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
    #30 0x7f2bfc0ef06b in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
    #31 0x7f2bfc2f7c1c in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4834:22
    #32 0x7f2bfc2fad5c in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4979:8
    #33 0x7f2bfc2fc224 in XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:5071:21
    #34 0x4f168b in do_main src/browser/app/nsBrowserApp.cpp:231:22
    #35 0x4f168b in main src/browser/app/nsBrowserApp.cpp:304
    #36 0x7f2c0fe9482f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #37 0x420f48 in _start (firefox+0x420f48)
Flags: in-testsuite?
This may be related to bug 1454139 or bug 1454133 so marking as s-s.
Blocks: 1454133
Group: dom-core-security
See Also: → 1454139
No longer blocks: 1454133
See Also: → 1454133
Masayuki, this looks editor related. Could you take a look? Thanks.
Flags: needinfo?(masayuki)
I investigated this yesterday but I still don't understand what happens. I cannot access the bugs mentioned in comment 3. If they could be good hint, could you cc me them too?
Flags: needinfo?(masayuki) → needinfo?(continuation)
Okay, got it. insertHorizontalRule command inserted <hr> element into the <input> element such DOM tree isn't expected by some places.
Assignee: nobody → masayuki
Flags: needinfo?(continuation)
Attached patch Patch for m-c (obsolete) — Splinter Review
See the commit message for the detail. This is caused by creating unexpected anonymous subtree in <input> element.
Attachment #8968456 - Flags: review?(m_kato)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #3)
> I investigated this yesterday but I still don't understand what happens. I
> cannot access the bugs mentioned in comment 3.

I've CC'ed you on those bugs in case they would be useful.
Comment on attachment 8968456 [details] [diff] [review]
Patch for m-c

Review of attachment 8968456 [details] [diff] [review]:

Since this bug is marked as core-security, I think you shouldn't land test case with fix according to

::: editor/libeditor/EditorDOMPoint.h
@@ +556,5 @@
> +    }
> +    if (!IsInNativeAnonymousSubtree()) {
> +      return EditorRawDOMPoint(*this);
> +    }
> +    nsINode* parent;

nsIContent* parent?
Attachment #8968456 - Flags: review?(m_kato) → review+
(In reply to Makoto Kato [:m_kato] from comment #8)
> ::: editor/libeditor/EditorDOMPoint.h
> @@ +556,5 @@
> > +    }
> > +    if (!IsInNativeAnonymousSubtree()) {
> > +      return EditorRawDOMPoint(*this);
> > +    }
> > +    nsINode* parent;
> nsIContent* parent?

Ah, no, parent can be a document node which is not derived from nsIContent. Therefore, mParent is nsINode. However, I used nsINode::GetParent() instead of nsINode::GetParentNode().  I'll change the method. (Although mParent shouldn't become document node in most cases.)
Attached patch The patch for m-c (r=m_kato) (obsolete) — Splinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It's easy since the patch includes the testcase to reproduce this crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

I don't think so. This looks like just a crash bug fix if there is no link to this closed bug report.

Which older supported branches are affected by this flaw?

Not sure but ESR 52 doesn't support <detail> and the testcase won't work as expected.

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

Not sure. Possible to change native anonymous subtree in <input>/<textarea> with execCommand() must be long standing bug. However, the crash itself must be newer bug since a lot of safer code have been replaced with faster code with Quantum Flow.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I created same patch for Beta branch too.

How likely is this patch to cause regressions; how much testing does it need?

This patch just avoids to insert unexpected DOM nodes into <input>/<textarea>'s anonymous subtree.  And such behavior has never been expected.  Therefore, this patch must be safe.  And also this patch includes the testcase of comment 0.
Attachment #8968456 - Attachment is obsolete: true
Attachment #8968770 - Flags: sec-approval?
Attachment #8968770 - Flags: review+
Attached patch Patch for Beta (obsolete) — Splinter Review
Attachment #8968457 - Attachment is obsolete: true
Attachment #8968771 - Flags: review+
Duplicate of this bug: 1454139
Severity: normal → critical
Priority: -- → P1
Actually, Dan looked at this and said it looks like a null deref and not a serious security bug. This isn't a sec-high.

I'll clear sec-approval but we REALLY shouldn't check in tests with any kind of security issue. If this was a sec-high or sec-critical, I'd deny the sec-approval and ask for a patch with no test.
Keywords: sec-highsec-low
Attachment #8968770 - Flags: sec-approval?
Hold on this is a dup of bug 1454139 which is not a null deref.
Making this a sec-high. 

This needs a patch which DOES NOT include a test. We're not allowed to zero day ourselves with a checkin (that's part of why sec-approval) exits.

The test can land 3-4 weeks after we ship a release with the fix and not before then. 

Masayuki, please update with a new patch and set sec-approval? on it.
Flags: needinfo?(masayuki)
Keywords: sec-lowsec-high
(In reply to Al Billings [:abillings] from comment #15)
> The test can land 3-4 weeks after we ship a release with the fix and not
> before then. 

Set the in-testsuite flag to '?' when you land the patch without tests, then later when the test lands set it to in-testsuite+
[Security approval request comment]
See comment 10. But this does NOT include the test.
Attachment #8968770 - Attachment is obsolete: true
Flags: needinfo?(masayuki)
Attachment #8969163 - Flags: sec-approval?
Attachment #8969163 - Flags: review+
Attached patch Patch for BetaSplinter Review
Attachment #8968771 - Attachment is obsolete: true
Attachment #8969164 - Flags: review+
Comment on attachment 8969163 [details] [diff] [review]
Patch for m-c (r=m_kato)

sec-approval+ for trunk. Given we have a week of betas left, I'll let release management make the decision about taking it on Beta.
Attachment #8969163 - Flags: sec-approval? → sec-approval+
Hi Masayuki, can you please nominate the patch for beta60 uplift? Thanks!
Flags: needinfo?(masayuki)
Bug 1454126 - HTMLEditor should adjust selection outside native anonymous subtree when it inserts something at selection r=m_kato
Comment on attachment 8969164 [details] [diff] [review]
Patch for Beta

Approval Request Comment
[Feature/Bug causing the regression]:
Not sure. This root cause must be a long standing bug. However, the crash must be caused by Quantum Flow since a lot places started to assume native anonymous subtree in <input>/<textarea> are always expected structure. Therefore, the crash is not reproducible with ESR 52.

[User impact if declined]
Might get odd HTML tree (i.e., |foo.innerHTML = bar.innerHTML;| doesn't create same DOM tree since <input> cannot have any children at parsing HTML document). And of course, may meet the crash.

[Is this code covered by automated tests?]:
Not yet. We should land it after release.

[Has the fix been verified in Nightly?]:
Not yet, but I confirmed it with local build.

[Needs manual test from QE? If yes, steps to reproduce]:
Yes, load attachment 8967914 [details] and check if it won't cause crash.

[List of other uplifts needed for the feature/fix]:

[Is the change risky?]:

[Why is the change risky/not risky?]:
This adds an additional check to HTMLEditor::GetBetterInsertionPointFor(). The additional new check is, if it's not in native anonymous subtree.  If it's in, it returns host element of the native anonymous subtree.

[String changes made/needed]:
Flags: needinfo?(masayuki)
Attachment #8969164 - Flags: approval-mozilla-beta?
Closed: 3 years ago
Resolution: --- → FIXED
Comment on attachment 8969164 [details] [diff] [review]
Patch for Beta

Approved for today's 60.0b15 build.
Attachment #8969164 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [adv-main60+]
Duplicate of this bug: 1454133
Group: dom-core-security → core-security-release
Duplicate of this bug: 1452704
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.