Closed
Bug 1394522
Opened 7 years ago
Closed 7 years ago
Null check ref pointers when iterating over intersection observers
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla57
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)
1.19 KB,
patch
|
smaug
:
review+
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
When iterating over the array of intersection observers stored in nsDocument, ref pointers should be null checked to avoid accessing freed memory.
Assignee | ||
Updated•7 years ago
|
Crash Signature: [@ mozilla::dom::DOMIntersectionObserver::Update ]
[@ mozilla::dom::DOMIntersectionObserver::Notify ]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tschneider
Updated•7 years ago
|
Group: layout-core-security, core-security
Comment 1•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
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…
Updated•7 years ago
|
Keywords: csectype-uaf,
sec-high
Comment 2•7 years ago
|
||
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.
Assignee | ||
Comment 3•7 years ago
|
||
Updated•7 years ago
|
status-firefox55:
--- → affected
status-firefox56:
--- → affected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → disabled
tracking-firefox55:
--- → ?
tracking-firefox56:
--- → ?
tracking-firefox57:
--- → ?
Assignee | ||
Comment 5•7 years ago
|
||
As discussed on IRC, always taking a copy of mIntersectionObservers before iterating.
Attachment #8903460 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8904322 -
Flags: review?(bugs)
Comment 6•7 years ago
|
||
So you found some case where Update may cause JS to run (or flush layout or something)?
Assignee | ||
Comment 7•7 years ago
|
||
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.
Comment 8•7 years ago
|
||
Null check doesn't protect against invalid pointers. It protects only against null.
Comment 9•7 years ago
|
||
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+
Assignee | ||
Comment 10•7 years ago
|
||
Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1577997d8571b4b967e650cddafbcb5cb6e7588
Comment 11•7 years ago
|
||
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: 7 years ago
tracking-firefox55:
? → ---
Flags: needinfo?(tschneider)
Resolution: --- → FIXED
Updated•7 years ago
|
Group: layout-core-security, core-security → core-security-release
Assignee | ||
Comment 12•7 years ago
|
||
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 13•7 years ago
|
||
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+
Comment 14•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/a36d089f670f
Comment 15•7 years ago
|
||
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?
Comment 16•7 years ago
|
||
...or better yet, lose the temp var and write it as: observers.AppendElement(iter.Get()->GetKey());
Comment 17•7 years ago
|
||
(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.
Comment 18•7 years ago
|
||
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?
Comment 19•7 years ago
|
||
See also comment 6-9.
Assignee | ||
Comment 20•7 years ago
|
||
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?
Assignee | ||
Comment 21•7 years ago
|
||
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.
Assignee | ||
Comment 22•7 years ago
|
||
Created Bug 1398437 with a patch.
Updated•7 years ago
|
Whiteboard: [adv-main56+]
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main56+] → [adv-main56+][post-critsmash-triage]
Updated•6 years ago
|
Target Milestone: --- → mozilla57
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•