1111971 - Use After Free in WebSocketChannel::BeginOpen()

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Looben Yang]

Attachment: wsserver_uaf_BeginOpen.js

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

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

[Tracking Requested - why for this release]:
WebSocket on workers is a new feature.

Baku, should we disable WebSockets on workers on beta at least?

Comment 2

[Andrea Marchesini [:baku]]

Yes. Let's disable them on beta. Separate bug for it.

Comment 3

[Andrew McCreight [:mccr8]]

The beta disable bug is bug 1112054.

Comment 4

[Andrew McCreight [:mccr8]]

I'm going to assume that you are the one to fix this, Andrea.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 7

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 10

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 12

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

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

Comment 13

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Forgot to run qref.

Comment 14

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fce8528ac1e2

Comment 16

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=5263744&repo=mozilla-inbound

Comment 17

[Andrea Marchesini [:baku]]

Attachment: id.patch

Interdiff.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 20

[Andrea Marchesini [:baku]]

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

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(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() ?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 23

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 25

[Andrea Marchesini [:baku]]

Attachment: ws2.patch

Right, sorry for this. This patch passes the tests.

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8547528 [details] [diff] [review]
ws2.patch

Comment 12. Thanks.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8547528 [details] [diff] [review]
ws2.patch

Approve all things!

You're good to go.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Andrea Marchesini [:baku]]

Attachment: fix.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/66150e000a62

Comment 31

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

https://hg.mozilla.org/mozilla-central/rev/8c49a59be57a
https://hg.mozilla.org/mozilla-central/rev/66150e000a62

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6e59be636f80
https://hg.mozilla.org/releases/mozilla-beta/rev/19e248751a1c

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6e59be636f80