Bugzilla

Comment 1

•

7 years ago

This bisects back to when we first started building Stylo by default, unfortunately. On debug builds, I see an assertion too, but I'm not sure if it's related or not.

ASSERTION: we already have a content declaration block: '!mContentDeclarationBlock', file /builds/worker/workspace/build/src/dom/svg/nsSVGElement.cpp, line 1314

Has Regression Range: --- → no

status-firefox56: --- → disabled

status-firefox-esr52: --- → unaffected

tracking-firefox57: --- → ?

tracking-firefox58: --- → ?

Summary: AddressSanitizer: heap-use-after-free [@ NonMappedAttrCount] with READ of size 8 → stylo: AddressSanitizer: heap-use-after-free [@ NonMappedAttrCount] with READ of size 8

Version: Trunk → 57 Branch

Comment 2

•

7 years ago

Note: to install the fuzzer extension (required by the testcase), you need to build the XPI locally (by cloning https://github.com/MozillaSecurity/domfuzz/tree/master/dom/extension and running "make" in that directory) and then setting these prefs in the Firefox Nightly profile that you're using for testing:
  xpinstall.signatures.required = false
  extensions.legacy.enabled = true
Then it should let you install the XPI.

Updated

•

7 years ago

Keywords: regression

Comment 3

•

7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> On debug builds, I see an assertion too, but I'm not sure if
> it's related or not.

It is related.

In my (non-ASAN-enabled) debug build, we're failing that assertion because this nsSVGElement's mContentDeclarationBlock is non-null -- and it's specifically 0xe5e5e5e5e5e5e5e5, which I think is indicative that this object is deallocated, or something like that.  So, this is the same sort of UAF.

The problem here is that the element is getting cycle collected, BUT we retain a raw pointer to it (after it's been deleted!!!) in this "mLazySVGPresElements" structure (which seems to be stylo-specific).  And then later on (after the element has been deleted), we call ResolveScheduledSVGPresAttrs, which traverses the contents of mLazySVGPresElements, including this pointer to a previously-deleted element. And we fail the assertion (because we have this bogus pointer value) and then crash from UAF.

So presumably we need to ensure that the element gets removed from mLazySVGPresElements when it's cycle-collected/deleted.

Comment 4

•

7 years ago

So, this is a regression from https://hg.mozilla.org/mozilla-central/rev/33e225044544 (Bug 1329093), which is where we added this mLazySVGPresElements stuff.  But stylo has to be enabled for this codepath to be active, hence the later de-facto regression range in comment 1.

Comment 5

•

7 years ago

(In reply to Daniel Holbert [:dholbert] from comment #3)
> So presumably we need to ensure that the element gets removed from
> mLazySVGPresElements when it's cycle-collected/deleted.

It looks like we do actually try to do this (~nsSVGElement calls OwnerDoc()->UnscheduleSVGForPresAttrEvaluation(this)).

BUT, the problem is that we're calling it on **a different document** from the document that we called ScheduleSVGForPresAttrEvaluation(this) on earlier.  So, Unschedule silently fails to remove anything, and the other document is left with a raw pointer to this element still present in its mLazySVGPresElements structure, which gets flushed later on.

I think nsSVGElement::NodeInfoChanged() needs to unregister the element from the old document, before registering it with the new document...

Comment 6

•

7 years ago

I guess nsSVGElement::NodeInfoChanged() does try to unregister from the old document, but then we change the document *again* afterwards, without calling NodeInfoChanged.

Specifically: we end up in nsNodeUtils::CloneAndAdopt, which has two relevant places where it changes the mNodeInfo (and the document):
  - Line 529, where we call NodeInfoChanged afterwards:
https://dxr.mozilla.org/mozilla-central/rev/2ed5e7fbf39e949693d8a7455d6313b14a7aeaf6/dom/base/nsNodeUtils.cpp#529-531

 - Line 618, where we DO NOT call NodeInfoChanged afterwards:
https://dxr.mozilla.org/mozilla-central/rev/2ed5e7fbf39e949693d8a7455d6313b14a7aeaf6/dom/base/nsNodeUtils.cpp#618

We end up changing the document (and unregistering/registering) via the first spot, and then changing the document again (and not doing any unregister/register dance!!!) via the second spot.  So we're left with a stale registration in the wrong document.

Assignee

Comment 7

•

7 years ago

Ugh.  I think we should be calling NodeInfoChanged in that second block!  This is basically similar to bug 1317409.

Assignee

Comment 8

•

7 years ago

Oh, and this probably affects various old branches too.  For example, nsMappedAttributeElement::NodeInfoChanged goes back to Firefox 55 and can leave stale pointers lying around if not called...

Assignee

Comment 9

•

7 years ago

Attached patch Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node — Details — Splinter Review

MozReview-Commit-ID: 71k1jv8thFA

Attachment #8917238 - Flags: review?(peterv)

Assignee

Updated

•

7 years ago

Assignee: nobody → bzbarsky

Status: NEW → ASSIGNED

Assignee

Comment 10

•

7 years ago

Comment on attachment 8917238 [details] [diff] [review]
Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  Not terribly
   easily.  This is an error codepath and for starters you have to figure out
   how to trigger that error.

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

Which older supported branches are affected by this flaw?  Probably 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?  This should be pretty simple to backport.

How likely is this patch to cause regressions; how much testing does it need?  I don't think this needs very much testing.

Attachment #8917238 - Flags: sec-approval?

Peter Van der Beken [:peterv]

Assignee

Comment 11

•

7 years ago

Note that I have _not_ had a chance to test that patch on the actual testcase yet.  I don't have an ASAN build around right this second.

Comment 12

•

7 years ago

Comment on attachment 8917238 [details] [diff] [review]
Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node

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

Hmm, I guess this was really a problem with the patch in bug 568188 (but might not have problematic at that point, depending on the NodeInfoChanged implementations).

Attachment #8917238 - Flags: review?(peterv) → review+

Julien Cristau [:jcristau] (back April 22)

Comment 13

•

7 years ago

[Tracking Requested - why for this release]: updating esr status per comments 8/10

status-firefox56: disabled → wontfix

status-firefox-esr52: unaffected → ?

tracking-firefox57: ? → +

tracking-firefox58: ? → +

tracking-firefox-esr52: --- → ?

Comment 14

•

7 years ago

Comment on attachment 8917238 [details] [diff] [review]
Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node

sec-approval+. Let's backport this to beta.
Is it not clear if ESR52 is affected?

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

Updated

•

7 years ago

Keywords: sec-critical

Comment 15

•

7 years ago

(In reply to Peter Van der Beken [:peterv] from comment #12)
> Hmm, I guess this was really a problem with the patch in bug 568188 (but
> might not have problematic at that point, depending on the NodeInfoChanged
> implementations).

Looking at other NodeInfoChanged impls, I think there's one other problematic one which does predate stylo/57:
 - nsMappedAttributeElement::NodeInfoChanged() calls...
     nsHTMLStyleSheet* sheet = OwnerDoc()->GetAttributeStyleSheet();
     mAttrsAndChildren.SetMappedAttrStyleSheet(sheet);
https://dxr.mozilla.org/mozilla-central/rev/a0488ecc201c04f2617e7b02f039344e8fbf0d9a/dom/base/nsMappedAttributeElement.cpp#42-45

That "SetMappedAttrStyleSheet(sheet)" call causes the element to store a raw pointer to the "new" document's stylesheet, via this code:
https://dxr.mozilla.org/mozilla-central/rev/a0488ecc201c04f2617e7b02f039344e8fbf0d9a/dom/base/nsMappedAttributes.cpp#254

So -- then, if we switch the element back to the original doc (without calling NodeInfoChanged) as described in comment 6, then I expect the "new" doc could be destroyed (along with its nsHTMLStyleSheet), while the nsMappedAttributeElement lives on indefinitely with a raw pointer to the now-destroyed nsHTMLStyleSheet.  So, it seems like that could trigger a UAF.

This version of the problem goes back to Firefox 55, it looks like, since that's where this nsMappedAttributeElement::NodeInfoChanged() impl was added (in bug 1352898).

(In reply to Al Billings [:abillings] from comment #14)
> Is it not clear if ESR52 is affected?

I *think* 52 is unaffected -- I'm not aware of any badness that this can cause that'd go back as far as 52.

Comment 16

•

7 years ago

Yeah, ESR52 only has two nonempty NodeInfoChanged implementations:
  https://dxr.mozilla.org/mozilla-esr52/search?q=NodeInfoChanged&redirect=true

They are:
https://dxr.mozilla.org/mozilla-esr52/rev/a0c13b815564700ddba946adf24b1462e761d5b3/dom/html/nsGenericHTMLElement.cpp#2847
https://dxr.mozilla.org/mozilla-esr52/rev/a0c13b815564700ddba946adf24b1462e761d5b3/dom/html/HTMLImageElement.cpp#715

...which don't look problematic from this bug's perspective. (We might still have incorrect behavior there, from missing a call into those functions, but they're not saving/working-with raw pointers to data that's owned by something else. So they don't seem like they could trigger a UAF.)

Assignee

Comment 17

•

7 years ago

I agree; 52 is not affected.  The two implementations there are idempotent, and we just called it and haven't had a chance to do anything since, really, so even the behavior is correct on 52.

Assignee

Comment 18

•

7 years ago

Alright, I have now tested this in an ASAN build and verified that the patch fixes the testcase.

Assignee

Comment 19

•

7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/51ff2d13e50c1e8f7731e4c7243064bb96d2e4d0

Updated

•

7 years ago

status-firefox-esr52: ? → unaffected

tracking-firefox-esr52: ? → ---

Comment 20

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/51ff2d13e50c

Status: ASSIGNED → RESOLVED

Closed: 7 years ago

status-firefox58: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla58

Comment 21

•

7 years ago

(In reply to Al Billings [:abillings] from comment #14)
> sec-approval+. Let's backport this to beta.

(IIRC, bz still needs to request beta approval separately -- tagging him for that -- but let me know if that's somehow already implicitly granted by abillings' comment here^^ .)

Flags: needinfo?(bzbarsky)

Updated

•

7 years ago

Flags: needinfo?(abillings)

Comment 22

•

7 years ago

I'll give Beta approval or Ritu can do so.

Flags: needinfo?(abillings) → needinfo?(rkothari)

Daniel Veditz [:dveditz]

Updated

•

7 years ago

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

Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)

Assignee

Comment 23

•

7 years ago

Comment on attachment 8917238 [details] [diff] [review]
Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node

Approval Request Comment
[Feature/Bug causing the regression]: Longstanding problem exacerbated by servo
[User impact if declined]: Likely-exploitable crashes
[Is this code covered by automated tests?]: No, because of the security status.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Probably good to
   double-check that the original testcase is fixed.  See comment 0 and 
   comment 2.  You probably need an ASAN build and the fuzzing extension.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: I don't think so.
[Why is the change risky/not risky?]: This is fixing a pretty clear bug in the
   adoption code, and doing that in a pretty simple way.
[String changes made/needed]: None.

Flags: needinfo?(bzbarsky)

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

Comment 24

•

7 years ago

Comment on attachment 8917238 [details] [diff] [review]
Make sure to call NodeInfoChanged whenever we change the nodeinfo on a node

Sec-crit, Beta57+

Flags: needinfo?(rkothari)

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

Comment 25

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/544de21bc060

status-firefox57: affected → fixed

Andrei Vaida [:avaida]

Updated

•

7 years ago

Flags: qe-verify+

Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com

Updated

•

7 years ago

Whiteboard: [adv-main57+]

Comment 26

•

7 years ago

I followed the steps from comment 2 and description, however the fuzzPriv addon says it is not compatible with Fx 57 and Fx 58. 
Can you tell me a workaround?

Thank you!