Closed Bug 1304201 Opened 3 years ago Closed 3 years ago

Out-of-bounds access in nsWebBrowser::RemoveWebBrowserListener()

Categories

(Core Graveyard :: Embedding: APIs, defect)

48 Branch
defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: q1, Assigned: myk)

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [post-critsmash-triage][adv-main52+])

Attachments

(1 file)

nsWebBrowser::RemoveWebBrowserListener() (embedding\browser\nsWebBrowser.cpp) reads (and, if the element read contains exactly the right garbage) removes an element one beyond the end of its |mListenerArray| array,

This bug could cause a jump to an incorrect address, because removing the element invokes its type's destructor, which (because the type eventually contains a |nsIWeakReference|), eventually calls the virtual function |nsISupports::Release()|. Since the element is beyond the array's end, it almost certainly contains an incorrect virtual function table pointer, which the generated code will use to call what should be the implementation of nsISupports::Release(), but which instead almost certainly will be code in some other function.

Also removing an element beyond the array's end will corrupt the array and memory far beyond its end (see nsTArray_base::ShiftData(), which gets invoked as part of the removal).


The bug is in lines 248-49, which use |count| instead of |count-1|:


230: NS_IMETHODIMP
231: nsWebBrowser::RemoveWebBrowserListener(nsIWeakReference* aListener,
232:                                        const nsIID& aIID)
233: {
234:   NS_ENSURE_ARG_POINTER(aListener);
235: 
236:   nsresult rv = NS_OK;
237:   if (!mWebProgress) {
238:     // if there's no-one to register the listener w/, and we don't have a queue
239:     // going, the the called is calling Remove before an Add which doesn't make
240:     // sense.
241:     if (!mListenerArray) {
242:       return NS_ERROR_FAILURE;
243:     }
244: 
245:     // iterate the array and remove the queued listener
246:     int32_t count = mListenerArray->Length();
247:     while (count > 0) {
248:       if (mListenerArray->ElementAt(count).Equals(aListener, aIID)) {
249:         mListenerArray->RemoveElementAt(count);
250:         break;
251:       }
252:       count--;
253:     }

The bug is still present in trunk.
Flags: sec-bounty?
Group: core-security → dom-core-security
I'd ask smaug or bz to review this obvious fix, but both are busy at the moment, according to their Bugzilla status.  dveditz: perhaps you can do so?
Assignee: nobody → myk
Status: NEW → ASSIGNED
Attachment #8795561 - Flags: review?(dveditz)
Comment on attachment 8795561 [details] [diff] [review]
remove correct WebBrowserListener

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

I think I can review this as an XPCOM API usage fix.

FWIW, on trunk (and maybe aurora), we now crash on these sorts of out-of-bounds accesses.
Attachment #8795561 - Flags: review?(dveditz) → review+
(In reply to Nathan Froyd [:froydnj] from comment #2)
> FWIW, on trunk (and maybe aurora), we now crash on these sorts of
> out-of-bounds accesses.

That is a very good idea.
I'd land this myself, but I'm unsure of the current policy about landing security fixes.  Specifically: when to land them in a cycle (if it matters) and how to describe them (to avoid tipping off black hats).  If someone points me at the appropriate document, I'm still happy to land it.  In the meantime, I've set checkin-needed in the hopes that someone who is privy to the policy will come along and do so for me.
Keywords: checkin-needed
(In reply to Myk Melez [:myk] [@mykmelez] from comment #4)
> I'd land this myself, but I'm unsure of the current policy about landing
> security fixes.  Specifically: when to land them in a cycle (if it matters)
> and how to describe them (to avoid tipping off black hats).  If someone
> points me at the appropriate document, I'm still happy to land it.  In the
> meantime, I've set checkin-needed in the hopes that someone who is privy to
> the policy will come along and do so for me.

Hi Myk, i guess https://wiki.mozilla.org/Security/Bug_Approval_Process is the doc you want. since this is sec-moderate i will land this patch
https://hg.mozilla.org/mozilla-central/rev/8e77884e0828
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Flags: sec-bounty? → sec-bounty+
Group: dom-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52+]
Group: core-security-release
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.