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

Status: RESOLVED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[q1]

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.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: remove correct WebBrowserListener

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?

Comment 2

[Nathan Froyd [:froydnj]]

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.

Comment 3

[q1]

(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.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 5

[Carsten Book [:Tomcat]]

(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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e77884e08283e644582bfdbbe45e5338034374f

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8e77884e0828