Closed Bug 1394522 Opened 3 years ago Closed 3 years ago

Null check ref pointers when iterating over intersection observers

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- disabled
firefox55 --- wontfix
firefox56 + fixed
firefox57 + fixed

People

(Reporter: tschneider, Assigned: tschneider)

References

Details

(Keywords: csectype-uaf, sec-high, Whiteboard: [adv-main56+][post-critsmash-triage])

Crash Data

Attachments

(1 file, 1 obsolete file)

When iterating over the array of intersection observers stored in nsDocument, ref pointers should be null checked to avoid accessing freed memory.
Crash Signature: [@ mozilla::dom::DOMIntersectionObserver::Update ] [@ mozilla::dom::DOMIntersectionObserver::Notify ]
Blocks: 1393610
Assignee: nobody → tschneider
Group: layout-core-security, core-security
(In reply to Tobias Schneider [:tobytailor] from comment #0)
> When iterating over the array of intersection observers stored in
> nsDocument, ref pointers should be null checked to avoid accessing freed
> memory.

This is clearly a UAF bug. Marking as such.
Crash Signature: [@ mozilla::dom::DOMIntersectionObserver::Update ] [@ mozilla::dom::DOMIntersectionObserver::Notify ] → [@ mozilla::dom::DOMIntersectionObserver::Update ] [@ mozilla::dom::DOMIntersectionObserver::Notify ] [@ nsTArray_Impl<T>::RemoveElementsAt | nsDocument::NotifyIntersectionObservers ] [@ nsDocument::ScheduleIntersectionObserverNotification ] [@ nsTArr…
How does a null check solve a UAF? do you dereference some other pointer assuming the first pointer is valid? Otherwise it sounds like you'd just get null dereferences.
Track 56+/57+ as sec-high.
As discussed on IRC, always taking a copy of mIntersectionObservers before iterating.
Attachment #8903460 - Attachment is obsolete: true
Attachment #8904322 - Flags: review?(bugs)
So you found some case where Update may cause JS to run (or flush layout or something)?
I initially thought so after we talked about it. But we actually use only use GetPrimaryFrame() without passing FlushType::Layout as an argument. That should not trigger any flush of layout. Or am I wrong. If we don't flush, I assume we don't have to copy the mIntersectionObserver before iterating. It still seems that we need the null checks in place, since current crash reports try to access invalid pointers there. I wasn't able to reproduce this with a test case tho. But we should have this checks anyway, preliminary.
Null check doesn't protect against invalid pointers. It protects only against null.
Comment on attachment 8904322 [details] [diff] [review]
Null check ref pointers when iterating over intersection observers

So I don't know how this would fix this bug.
But didn't figure out what causes this bug when I was looking at the code.

However, I think we should anyhow do this, at least the sTArray<RefPtr<DOMIntersectionObserver>> part, since it is so easy to change Update to do something which may trigger a layout flush or so.
Attachment #8904322 - Flags: review?(bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/870c7725fccc

Please nominate this for beta approval when you get a chance. Also, this should have had sec-approval before landing since it's a sec-high that affects more than just trunk.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(tschneider)
Resolution: --- → FIXED
Group: layout-core-security, core-security → core-security-release
Comment on attachment 8904322 [details] [diff] [review]
Null check ref pointers when iterating over intersection observers

Approval Request Comment
[Feature/Bug causing the regression]: 1243846
[User impact if declined]: Crash risk. (see crash signatures).
[Has the fix been verified in Nightly?]: Yes
[Is the change risky?]: No
Flags: needinfo?(tschneider)
Attachment #8904322 - Flags: approval-mozilla-beta?
Comment on attachment 8904322 [details] [diff] [review]
Null check ref pointers when iterating over intersection observers

Fix a sec-high. Beta56+.
Attachment #8904322 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
diff --git a/dom/base/nsDocument.cpp b/dom/base/nsDocument.cpp
--- a/dom/base/nsDocument.cpp
+++ b/dom/base/nsDocument.cpp
@@ -13118,9 +13118,15 @@ nsDocument::UpdateIntersectionObservatio
       time = perf->Now();
     }
   }
+  nsTArray<RefPtr<DOMIntersectionObserver>> observers(mIntersectionObservers.Count());
   for (auto iter = mIntersectionObservers.Iter(); !iter.Done(); iter.Next()) {
     DOMIntersectionObserver* observer = iter.Get()->GetKey();

I think it's the line above that may be your bug ^. Shouldn't it be: 

     RefPtr<DOMIntersectionObserver> observer = iter.Get()->GetKey();

...so that pointers will actually become null-checkable if Update() mutates the observer list?
...or better yet, lose the temp var and write it as:
observers.AppendElement(iter.Get()->GetKey());
(In reply to Jet Villegas (:jet) from comment #15)
>      DOMIntersectionObserver* observer = iter.Get()->GetKey();
> 
> I think it's the line above that may be your bug ^. Shouldn't it be: 
> 
>      RefPtr<DOMIntersectionObserver> observer = iter.Get()->GetKey();

I don't think this would make a difference.  The nsTArray will create its own RefPtr entries, regardless of whether we pass it a temporary raw ptr vs a temporary RefPtr.  If we change the temporary to be a RefPtr, then that just means we'll incur more refcount churn, but it won't change any outcome here.

(The one-liner form in comment 16 is a nice for brevity, though, IMO.)

> ...so that pointers will actually become null-checkable if Update() mutates
> the observer list?

I don't think the suggestions here help with that yet, but I do agree that the pointers don't seem to be usefully null-checkable at this point.  If Update() mutates the observer list, the local "observers" array still remains untouched (and keeps everything it points to alive)...

So the "if (observer)" null checks seem useless right now, as long as we're expecting GetKey() to have returned something non-null back when we called it...  The most useful thing here seems to be the separate local copy of the array, IIUC, not the null-checks.
In other words, I don't think it's possible for the "if (observer)" null-checks to fail.

Or, if it is possible, then that suggests that |iter.Get()->GetKey()| gave us a null value back in the first loop -- in which case, we should just filter out null values at that point instead of inserting them into |observers| only to skip over them later?
See also comment 6-9.
nsTObserverArray seams to be a safe option here for arrays that might get modified during iteration.
Should we make mIntersectionObserver a nsTObserverArray instead of a nsTHashtable? Or as an alternate, would it make sense to keep ref pointers instead of raw pointers in mIntersectionObserver?
Digging further into this, nsTObserverArray doesn't seam to be a good fit (looks like it can only be used for nsIDocumentObserver). I think making mIntersectionObservers an nsTArray<RefPtr<DOMIntersectionObserver>> is the safest option here.
Created Bug 1398437 with a patch.
Whiteboard: [adv-main56+]
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
Target Milestone: --- → mozilla57
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.