426143 - nsAbRDFDataSource::NotifyObservers passes stack local variables to ASYNC proxies violating the API

Status: RESOLVED INVALID

(MailNews Core :: Address Book, defect)

Description

[timeless]

please read bug 382631 comment 10 and the linked document.

nsAbRDFDataSource::NotifyObservers is broken it's passing a pointer to a stack local variable and assuming something good will happen. This is of course wrong. Sadly it won't be protected by bug 382631's fix because the variable is marked as a voidPtr.

note: i very very very rarely check this checkbox, if you see this bug and you see me checking it, please assume I had a reason to do so.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

I'm assuming you checked the security-sensitive checkbox because the theory is that this could be an exploitable crash, correct?  

When would this code be running on something other than the UI thread -- during import?

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

Timeless confirms that this may be exploitable.  It should block 3.0a1, and probably 2.0.0.14 as well.

Comment 3

[Daniel Veditz [:dveditz]]

Have enough flags?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Andrew, is this something you'd have bandwidth to work on?  It's one of the last small number of 3.0a1 blockers.  If so, please go ahead and take...

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

So, I'm not sure I see the problem.  NotifyObservers does create an nsAbRDFNotification struct on the stack and does pass a void pointer to said structure.  However, it passes the pointer to a call to either nsCOMArray::EnumerateForwards (trunk) or nsSupportsArray::EnumerateForwards (1.8 branch) with a helper function (changeEnumFunc, assertEnumFunc, or unassertEnumFunc).  The helper functions are the ones that actually call the proxy objects, and they 'unbox' the contents of the (void pointer passed) nsAbRDFNotification struct, passing only nsIRDFblah pointers.

I don't see any magic in the call to EnumerateForwards that could cause a problem, and it looks like all the callers of NotifyObservers are actually initializing things properly.  Am I missing something?

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

It looks to me as though you're right about the unboxing itself.  There's still a possible cause for concern here about object lifetimes, however: the unboxed nsIRDF* stuff originates as raw COM pointer arguments to nsAbRDFDataSource::NotifyObservers() on whatever non-UI thread we happen to be running on.  This means that if NotifyObservers returns quickly, the {change,assert,unassert} functions may be operating on (and calling through) non-owning references.  This works as long as nothing has happened to cause the refcount of those objects to hit zero and be deallocated before the main thread is done with them.  Thoughts?

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

I think we're good; it looks like the proxy object takes care to reference count the arguments.  (Which makes sense.)

nsProxyObjectManager::GetProxyForObject (used by nsAbRDFDataSource::CreateProxyObservers via CreateProxyObserver) creates and returns an nsProxyObject.  nsProxyObject::QueryInterface creates an nsProxyEventObject (via LockedFind).
nsProxyEventObject::CallMethod creates an nsProxyObjectCallInfo object.  The nsProxyObjectCallInfo constructor issues a call to RefCountInInterfacePointers, which will AddRef/Release each parameter if:
 * GetType().IsInterfacePointer() is true
 * IsIn() is true
 * it has an nsISupports interface
It will also copy/free strings in the NS_PROXY_ASYNC case, but we don't care about that.

The nsIRDFObserver defines all of the parameters to be 'in'.

Comment 8

[David Ascher (:davida)]

timeless, dveditz: andrew and dmose think this isn't a bug.  Your take based on analysis above?

Comment 9

[timeless]

sorry. in reading it, for some reason i decided that the mProxyObservers was the proxy and not the things it held.