Use After Free in WebSocketChannel::BeginOpen()

RESOLVED FIXED in Firefox 36

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: loobenyang, Assigned: baku)

Tracking

({csectype-uaf, regression, sec-critical})

37 Branch
mozilla38
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox34 unaffected, firefox35+ disabled, firefox36+ fixed, firefox37+ fixed, firefox38 fixed, firefox-esr31 unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.2-])

Attachments

(4 attachments, 7 obsolete attachments)

Steps to reproduce:

Run web socket server wsserver_uaf_BeginOpen.js  with Node.js module websocket, enter http://localhost:12345/ in Firefox browser. It can hit a non relevant null pointer reference, in this case just restart it.

Firefox Version: 37.0a1 (2014-12-16)
Operating System: Windows 7 64bit


Test case:
On server side, it accepts web socket at port 12345 with protocol "wsm1-protocol", and then send close frame in 50ms;
On client side, it initiates web socket to port 12345 with protocol "wsm1-protocol" in 2 web workers.

Client and server side code have been combined in a single node.js source file wsserver_uaf_BeginOpen.js.


Actual results:

Firefox crashes in WebSocketChannel::BeginOpen(), the 5A5A5A5A pattern in crash address indicates a Use After Free:

First-chance exception at 0x02522151 (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x5A5A5A5E.
Unhandled exception at 0x02522151 (xul.dll) in firefox.exe: 0xC0000005: Access violation reading location 0x5A5A5A5E.


Registers:

EAX = 5A5A5A5A EBX = 00000000 ECX = 1A924000 EDX = 00000000 ESI = 193F8840 EDI = 1A924000 EIP = 02522151 ESP = 1B0BF650 EBP = 00000000 EFL = 00010206 

0x5a5a5a5e = 00000000 


Backtrace:

>	xul.dll!nsRunnableMethodImpl<void (__thiscall nsProcess::*)(void),void,1>::nsRunnableMethodImpl<void (__thiscall nsProcess::*)(void),void,1>(nsProcess * aObj, void (void) * aMethod) Line 382	C++
 	xul.dll!NS_NewRunnableMethod<nsAttributeTextNode *,void (__thiscall nsAttributeTextNode::*)(void)>(nsAttributeTextNode * aPtr, void (void) * aMethod) Line 411	C++
 	xul.dll!mozilla::net::WebSocketChannel::BeginOpen() Line 1186	C++
 	xul.dll!mozilla::net::FailDelayManager::DelayOrBegin(mozilla::net::WebSocketChannel * ws) Line 286	C++
 	xul.dll!mozilla::net::nsWSAdmissionManager::ConnectNext(nsCString & hostName) Line 495	C++
 	xul.dll!mozilla::net::nsWSAdmissionManager::OnStopSession(mozilla::net::WebSocketChannel * aChannel, tag_nsresult aReason) Line 429	C++
 	xul.dll!mozilla::net::CallOnStop::Run() Line 604	C++
 	xul.dll!mozilla::dom::`anonymous namespace'::WorkerRunnableDispatcher::WorkerRun(JSContext * aCx, mozilla::dom::workers::WorkerPrivate * aWorkerPrivate) Line 2557	C++
 	xul.dll!mozilla::dom::workers::WorkerRunnable::Run() Line 326	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 830	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 265	C++
 	xul.dll!mozilla::dom::workers::WorkerPrivate::DoRunLoop(JSContext * aCx) Line 4248	C++
 	xul.dll!`anonymous namespace'::WorkerThreadPrimaryRunnable::Run() Line 2615	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 830	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 265	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 339	C++
 	xul.dll!MessageLoop::RunHandler() Line 227	C++
 	xul.dll!MessageLoop::Run() Line 201	C++
 	xul.dll!nsThread::ThreadFunc(void * aArg) Line 359	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 419	C
 	nss3.dll!pr_root(void * arg) Line 90	C
 	msvcr110.dll!_callthreadstartex() Line 354	C
 	msvcr110.dll!_threadstartex(void * ptd) Line 332	C
 	kernel32.dll!@BaseThreadInitThunk@12()	Unknown
 	ntdll.dll!___RtlUserThreadStart@8()	Unknown
 	ntdll.dll!__RtlUserThreadStart@8()	Unknown
[Tracking Requested - why for this release]:
WebSocket on workers is a new feature.

Baku, should we disable WebSockets on workers on beta at least?
Yes. Let's disable them on beta. Separate bug for it.
Blocks: 504553
The beta disable bug is bug 1112054.
I'm going to assume that you are the one to fix this, Andrea.
Assignee: nobody → amarchesini
Posted patch ws2.patch (obsolete) — Splinter Review
Attachment #8537421 - Flags: review?(bugs)
Comment on attachment 8537421 [details] [diff] [review]
ws2.patch

>+  // Let's keep the object alive because the webSocket can be CCed in the
>+  // onopen callback.
>+  nsRefPtr<WebSocket> webSocket = mWebSocket;
>+
>   // Call 'onopen'
>-  rv = mWebSocket->CreateAndDispatchSimpleEvent(NS_LITERAL_STRING("open"));
>+  rv = webSocket->CreateAndDispatchSimpleEvent(NS_LITERAL_STRING("open"));
>   if (NS_FAILED(rv)) {
>     NS_WARNING("Failed to dispatch the open event");
>   }
> 
>-  mWebSocket->UpdateMustKeepAlive();
>-
>+  webSocket->UpdateMustKeepAlive();

Do we need this part for the main thread stuff too?
And on which branches if yes?

>   NS_IMETHOD Run()
>   {
>     MOZ_ASSERT(mChannel->IsOnTargetThread());
> 
>-    nsWSAdmissionManager::OnStopSession(mChannel, mReason);
>-
>     if (mChannel->mListener) {
>       mChannel->mListener->OnStop(mChannel->mContext, mReason);
>       mChannel->mListener = nullptr;
>       mChannel->mContext = nullptr;
>     }
It is not clear what if anything releases mListener and mContext.
We don't want leaks. So either setting those member variables null here is not needed, or we should set them null also elsewhere
(in case dispatching the runnable fails)
Attachment #8537421 - Flags: review?(bugs) → review-
Flags: sec-bounty?
Posted patch ws2.patch (obsolete) — Splinter Review
This patch does 2 things:

1. keep the websocket alive when a event is dispatched.
2. Stop the session also when CallOnStop is not called. Calling StopSession before OnStop() can change the order of some operation, so we have to check if mListener exists before using it in other runnables.
Attachment #8537421 - Attachment is obsolete: true
Attachment #8540699 - Flags: review?(bugs)
Comment on attachment 8540699 [details] [diff] [review]
ws2.patch

>   NS_IMETHOD Run()
>   {
>     MOZ_ASSERT(mChannel->IsOnTargetThread());
> 
>-    if (mLen < 0)
>-      mChannel->mListener->OnMessageAvailable(mChannel->mContext, mData);
>-    else
>-      mChannel->mListener->OnBinaryMessageAvailable(mChannel->mContext, mData);
>+    if (mChannel->mListener) {
>+      if (mLen < 0)
>+        mChannel->mListener->OnMessageAvailable(mChannel->mContext, mData);
>+      else
>+        mChannel->mListener->OnBinaryMessageAvailable(mChannel->mContext, mData);
>+    }

Would be nice if this code was using {} with if and else.

>-  CallOnStop(WebSocketChannel *aChannel,
>-             nsresult          aReason)
>+  CallOnStop(WebSocketChannel     *aChannel,
>+             nsresult             aReason)
Why so much space. I'd just remove extra spaces before aReadon, not add more after WebSocketChannel
(and * goes with the type, not with the param name.)


>     : mChannel(aChannel),
>-      mReason(aReason) {}
>+      mReason(aReason)
>+  {
>+      mListener.swap(mChannel->mListener);
>+      mContext.swap(mChannel->mContext);
>+  }
This is a bit scary. We pass aChannel, and then the ctor of CallOnStop changes the lifetime management of
channel->mListener and channel->mContext. So please document this.


And nothing guarantees on which thread mListener or mContext is now released.
This need to be clarified. Why it is safe to release the member variables in any thread?
Because of this one, r-.


>+
>+    nsWSAdmissionManager::OnStopSession(this, reason);
>+
>     mTargetThread->Dispatch(new CallOnStop(this, reason),
>                             NS_DISPATCH_NORMAL);
Hmm, is it actually guaranteed that Dispatch never throws here? We don't want leaks.
Perhaps store the CallOnStop in a nsRefPtr<nsRunnable> or some such and pass that to Dispatch.
(I know, not new code in this patch.)
Attachment #8540699 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] (away-ish 12/28-1/2) from comment #8)
> 
> And nothing guarantees on which thread mListener or mContext is now released.
> This need to be clarified. Why it is safe to release the member variables in
> any thread?
> Because of this one, r-.
> 

And if it is always safe to release them, document it clearly, and ask re-review.
Posted patch ws2.patch (obsolete) — Splinter Review
Actually it turned out that the existing code is wrong. mListener and mContext must be released on the main-thread.

Then... this is not a real problem because mListener is WebSocketImpl that is thread-safe and mContext is null.
Attachment #8540699 - Attachment is obsolete: true
Attachment #8542109 - Flags: review?(bugs)
Comment on attachment 8542109 [details] [diff] [review]
ws2.patch

>+namespace {
No anonymous namespaces. Uses static methods.

>+
>+void
>+ReleaseListenerAndContext(nsCOMPtr<nsIWebSocketListener>& aListener,
>+                          nsCOMPtr<nsISupports>& aContext)
>+{
>+  nsCOMPtr<nsIThread> mainThread;
>+  NS_GetMainThread(getter_AddRefs(mainThread));
>+
>+  if (aListener) {
>+    nsIWebSocketListener *forgettableListener;
* goes with the type.

>+    aListener.forget(&forgettableListener);
>+    NS_ProxyRelease(mainThread, forgettableListener, false);
...but you should be able to pass aListener to NS_ProxyRelease.
There is NS_ProxyRelease(nsIEventTarget* aTarget, nsCOMPtr<T>& aDoomed, ...);

>+  }
>+
>+  if (aContext) {
>+    nsISupports *forgettableContext;
>+    aContext.forget(&forgettableContext);
>+    NS_ProxyRelease(mainThread, forgettableContext, false);
ditto
Attachment #8542109 - Flags: review?(bugs) → review+
Posted patch ws2.patch (obsolete) — Splinter Review
Approval Request Comment
[Feature/regressing bug #]: bug 504553
[User impact if declined]: a crash
[Describe test coverage new/current, TBPL]: hard to test.
[Risks and why]: This patch is not complex, I don't see risks to accept it.
[String/UUID change made/needed]: none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It's easy to reproduce, just using the test script attached to the bug.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes. The issue is that  we had a wrong management of the life-time of aListener and aContext in WebSocketChannel, plus, there was a wrong ordering of messages in WebSocket when running in workers.

Which older supported branches are affected by this flaw?

aurora. In beta we have webSocket disabled by pref.

If not all supported branches, which bug introduced the flaw?

504553

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should apply to aurora. In case, it's easy to provide a porting.

How likely is this patch to cause regressions; how much testing does it need?

It's hard to cause regressions, but WebSockets in workers are showing us too many issues that maybe there is still something to fix...
Attachment #8542109 - Attachment is obsolete: true
Attachment #8545850 - Flags: sec-approval?
Attachment #8545850 - Flags: approval-mozilla-aurora?
Posted patch ws2.patch (obsolete) — Splinter Review
Forgot to run qref.
Attachment #8545850 - Attachment is obsolete: true
Attachment #8545850 - Flags: sec-approval?
Attachment #8545850 - Flags: approval-mozilla-aurora?
Attachment #8545855 - Flags: sec-approval?
Attachment #8545855 - Flags: approval-mozilla-aurora?
Comment on attachment 8545855 [details] [diff] [review]
ws2.patch

sec-approval+ for trunk. I've approved for Aurora as well but please wait until after it lands cleanly on trunk before taking it to Aurora.
Attachment #8545855 - Flags: sec-approval?
Attachment #8545855 - Flags: sec-approval+
Attachment #8545855 - Flags: approval-mozilla-aurora?
Attachment #8545855 - Flags: approval-mozilla-aurora+
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5263744&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
Posted patch id.patchSplinter Review
Interdiff.
Flags: needinfo?(amarchesini)
Attachment #8546742 - Flags: review?(bugs)
Posted patch ws2.patch (obsolete) — Splinter Review
Attachment #8545855 - Attachment is obsolete: true
Attachment #8546744 - Flags: review?(bugs)
So we're now back in comment 8
"
And nothing guarantees on which thread mListener or mContext is now released.
This need to be clarified. Why it is safe to release the member variables in any thread?
Because of this one, r-."

Then you say
"Actually it turned out that the existing code is wrong. mListener and mContext must be released on the main-thread.

Then... this is not a real problem because mListener is WebSocketImpl that is thread-safe and mContext is null."

which doesn't really help. "must be released on the main-thread" ... "not a real problem"?
So, like what?
The old code does use NS_ProxyRelease

Could we leave the code to WebSocketChannel::~WebSocketChannel
and add a helper class which CallOnStop, CallOnServerClose and CallAcknowledge all inherit.
That helper class would have 
nsCOMPtr<nsIWebSocketListener>    mListener;
nsCOMPtr<nsISupports>             mContext; 
as member variables.
It would also have a method ProxyRelease() (which would do a proxy release for mListener and mContext)
and dtor. Its dtor would call ProxyRelease().
At the end of CallOnStop's, CallOnServerClose's and CallAcknowledge's Run() ProxyRelease would also be called.

Wouldn't that guarantee the right release thread in all the cases?
Attachment #8546744 - Flags: review?(bugs) → review-
Attachment #8546742 - Flags: review?(bugs)
> And nothing guarantees on which thread mListener or mContext is now released.
> This need to be clarified. Why it is safe to release the member variables in
> any thread?

My approach is to assume that mListener and mContext are thread-safe. This is actually true in reality because WebSocketImpl is thread-safe and mContext is null.

> Could we leave the code to WebSocketChannel::~WebSocketChannel
> and add a helper class which CallOnStop, CallOnServerClose and
> CallAcknowledge all inherit.
> That helper class would have 
> nsCOMPtr<nsIWebSocketListener>    mListener;
> nsCOMPtr<nsISupports>             mContext; 
> as member variables.

Sure, but AddRef() if mListener and mContext are not thread-safe must run on the main-thread. Right?
So also this part will fail.
(In reply to Andrea Marchesini (:baku) from comment #20)
> > And nothing guarantees on which thread mListener or mContext is now released.
> > This need to be clarified. Why it is safe to release the member variables in
> > any thread?
> 
> My approach is to assume that mListener and mContext are thread-safe. This
> is actually true in reality because WebSocketImpl is thread-safe and
> mContext is null.
Why do we need mContext then if it is null?
And earlier you said "mListener and mContext must be released on the main-thread."
I'm getting confused here.

> > Could we leave the code to WebSocketChannel::~WebSocketChannel
> > and add a helper class which CallOnStop, CallOnServerClose and
> > CallAcknowledge all inherit.
> > That helper class would have 
> > nsCOMPtr<nsIWebSocketListener>    mListener;
> > nsCOMPtr<nsISupports>             mContext; 
> > as member variables.
> 
> Sure, but AddRef() if mListener and mContext are not thread-safe must run on
> the main-thread. Right?
> So also this part will fail.
Ah, indeed.
So don't take strong ref in runnables but add a method which releases safely in the main thread and call that
in runnables? something like mChannel->ReleaseListenerAndContextInMainThread() ?
One approach could be also that listener and context are kept alive by a separate threadsafe refcnted
object, and channel and relevant runnables keep a reference to that object.
Then when that object is about to die, it proxies releasing of listener and context in the main thread.
Posted patch ws2.patch (obsolete) — Splinter Review
Attachment #8546744 - Attachment is obsolete: true
Attachment #8547509 - Flags: review?(bugs)
Comment on attachment 8547509 [details] [diff] [review]
ws2.patch

>+BaseWebSocketChannel::ListenerAndContextContainer::ListenerAndContextContainer(
>+                                               nsIWebSocketListener* aListener,
>+                                               nsISupports* aContext)
>+  : mListener(aListener)
>+  , mContext(aContext)
>+{
>+  MOZ_ASSERT(NS_IsMainThread());
>+  MOZ_ASSERT(mListener && mContext);
Based on earlier comments this looks wrong.
You said mContext can be null.
(but it would be great if we can guarantee here that mListener isn't null)
Posted patch ws2.patchSplinter Review
Right, sorry for this. This patch passes the tests.
Attachment #8547509 - Attachment is obsolete: true
Attachment #8547509 - Flags: review?(bugs)
Attachment #8547528 - Flags: review?(bugs)
Attachment #8547528 - Flags: review?(bugs) → review+
Comment on attachment 8547528 [details] [diff] [review]
ws2.patch

Comment 12. Thanks.
Attachment #8547528 - Flags: sec-approval?
Attachment #8547528 - Flags: approval-mozilla-aurora?
Comment on attachment 8547528 [details] [diff] [review]
ws2.patch

This is very unlikely to make it to Aurora36 before today's uplift. Requesting beta approval for it as well under that assumption.
Attachment #8547528 - Flags: approval-mozilla-beta?
Comment on attachment 8547528 [details] [diff] [review]
ws2.patch

Approve all things!

You're good to go.
Attachment #8547528 - Flags: sec-approval?
Attachment #8547528 - Flags: sec-approval+
Attachment #8547528 - Flags: approval-mozilla-beta?
Attachment #8547528 - Flags: approval-mozilla-beta+
Attachment #8547528 - Flags: approval-mozilla-aurora?
Attachment #8547528 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/8c49a59be57a
https://hg.mozilla.org/mozilla-central/rev/66150e000a62
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Flags: sec-bounty? → sec-bounty+
Whiteboard: [b2g-adv-main2.2-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.