1087633 - (CVE-2014-1590) XMLHttpRequest.prototype.send crashes when given a crafted object

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[joev]

Attachment: screenshot of call stack

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.104 Safari/537.36

Steps to reproduce:

Crashes safely on an out-of-thread access:

xhr = new XMLHttpRequest();
xhr.open('POST', '/');
xhr.send({available: function(){}});

Crashes safely on null ptr deref:

q=0;
u = new Proxy({}, {get:function(){
  if(q++>0){xhr.abort()}
  return function(){};
}});
xhr = new XMLHttpRequest();
xhr.open('POST', '/k');
xhr.send(u);


I didn't explore any further. Looks like a type confusion issue. I poked around a bit to look for exploitability, but nothing obvious jumped out at me.



Expected results:

Sorry for the terrible huge crash screenshot! I forgot to dump the stack to text.

Comment 1

[joev]

Attachment: Null pointer deref stacktrace

Comment 2

[joev]

Attachment: Off main thread crash stacktrace

Comment 3

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

We seem to pass js as nsIInputStream to necko.
It is not clear to me why we let this behavior.

Comment 4

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

Does anyone recall why send(nsIInputStream); isn't chrome only?

Comment 5

[Boris Zbarsky [:bzbarsky]]

We don't have a way to make only some overloads chrome-only in the IDL.  And it looks like the implementation doesn't do any security checks.

Given that first stack, which shows the stream being read on a background thread, we want to disallow all JS-implemented streams here, even when called from chrome.  And content can't get its hands on an actual stream object anyway.  So I think the right thing to do here is to QI the stream to nsIXPCWrappedJS and if that succeeds fail out.

That said, this won't help cases when the stream passed in is a wrapper around some JS-implemented stream (from chrome only, obviously)....

We could also consider marking the stream interface builtinclass, though this might break some extensions that use other APIs involving streams.  But chances are, those extensions are already doing weird things when trying to _use_ such streams, if they end up going through the stream transport service...

Comment 6

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

Attachment: v1

This is what I had as the first idea.

Comment 7

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

...which is apparently the same what you proposed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8512888 [details] [diff] [review]
v1

r=me

Comment 9

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

Comment on attachment 8512888 [details] [diff] [review]
v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It is somewhat obvious that we're filtering out script implemented InputStreams, and if one tries passing
script implemented stream, bad things happen.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The commit message will be effectively the same what the code does
"Bug 1087633, Filter out XPConnect wrapped input streams, r=bz"

Which older supported branches are affected by this flaw?
all

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch should apply everywhere. (possibly with some fuzz)

How likely is this patch to cause regressions; how much testing does it need?
It should be pretty safe since script implemented streams would have crashed pretty reliably.

[String/UUID change made/needed]:
NA

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Given the request for uplift to ESR31, I take it this goes back at least as far as 31. Do you have a sec rating for this bug?

Comment 11

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

Unless proved otherwise, I'd treat this as sec-critical.
Addref/release of wrappedjs crash safely in case of wrong thread, but I'm not at all sure
about the safety of other cases.

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

Note that I'm assuming the B2G is impacted.

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Dan - Olli has said that he considers this sec-critical (comment 11). Do you agree? How do you want to handle landing?

Comment 14

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

We have quite some
if (!MOZ_LIKELY(NS_IsMainThread()))
  MOZ_CRASH();

checks in WrappedJS, but it is hard to prove we aren't missing any case.

bholley might be less paranoid.

Comment 15

[Bobby Holley (:bholley)]

I'm pretty sure that we'll crash safely if C++ code tries to invoke any methods on an XPCWrappedJS-ed XPCOM object off-main thread. I don't know enough about what's going on to say what that means.

Comment 16

[Daniel Veditz [:dveditz]]

Does XHR access the stream off main thread in every case, even in Sync XHR?

Comment 17

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

Sync XHR is actually just async XHR + spinning event loop. So no difference there.

Comment 18

[joev]

I tried this in a Worker script as well, but the type confusion does not seem possible in my testing.

Comment 19

[Daniel Veditz [:dveditz]]

Is it fair to lower the severity to sec-moderate at this point? We don't know of a way to get the bogus object used on the content thread and the off-main-thread crashes look relatively benign. Obviously we should still fix this but we don't need to be so jumpy about backports.

ESR would still be nice though -- it's a small safe patch and plugs up an easy DoS.

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 8512888 [details] [diff] [review]
v1

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

sec-approval and a=dveditz for aurora. I'll leave the beta r? for lmandel. I wouldn't mind landing this on beta given the patch but it doesn't seem urgent to do so.

I'd like to get it fixed on the corresponding ESR though, however far back we backport it, just in case our analysis is overly-optimistic.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8512888 [details] [diff] [review]
v1

I think this is good to take on Beta and ESR. This patch still needs to land on m-c. Olli, can you get this landed today?

Comment 22

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

Landing...

for branches one may need to rename dom/base/nsXMLHttpRequest.h to
content/base/src/nsXMLHttpRequest.h
With that, the patch seems to apply just fine.

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cce9c56538f

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for Static Analysis bustage. Those builds are non-unified in case that matters.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6eea06e3b908

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3481070&repo=mozilla-inbound

Comment 25

[joev]

Hrm. Here is another interesting case from this bug (the URLs are irrelevant):

q=0;
xhr = new XMLHttpRequest();
xhr.open('POST', 'https://google.com/');
u = new Proxy(xhr, {get:function(m,n){ alert(n);
  if(q++>=0){xhr.abort(); xhr.open('GET', 'http://localhost:9990/');xhr.send();}
  return function(){};
}});
xhr.send(u);

Throws the error:

Thread 1: EXC_BAD_ACCESS (code=2 address=0x7fff5f3fffc8)

Sounds like memory corruption. I'll upload the stacktrace in a moment.

Comment 26

[joev]

Attachment: stack3.txt

Stack trace from previously-mentioned crash

Comment 27

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> Backed out for Static Analysis bustage. Those builds are non-unified in case
> that matters.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/6eea06e3b908
> 
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3481070&repo=mozilla-inbound

Hmm, what include am I missing. nsIInputStream.h ?

Comment 28

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

Attachment: +#include "nsIInputStream.h"

Comment 29

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

(In reply to joev from comment #26)
> Created attachment 8515241 [details]
> stack3.txt
> 
> Stack trace from previously-mentioned crash
As far as I see, this is stack overflow. Locally the stack gets huge before crashing.

Comment 30

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

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

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dad61f719326

Comment 32

[joev]

(In reply to Olli Pettay [:smaug] from comment #29)
> (In reply to joev from comment #26)
> > Created attachment 8515241 [details]
> > stack3.txt
> > 
> > Stack trace from previously-mentioned crash
> As far as I see, this is stack overflow. Locally the stack gets huge before
> crashing.

I realized that a few moments after posting it, derp, the stack size is >48k... I needed some sleep :) Anyways thanks for the quick response here guys.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1a0fd34cd02d
https://hg.mozilla.org/releases/mozilla-beta/rev/72938afdf993
https://hg.mozilla.org/releases/mozilla-esr31/rev/d38075a0a0f6

Please request b2g30 and b2g32 approval on this when you get a chance.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/72938afdf993

Comment 35

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

Comment on attachment 8515667 [details] [diff] [review]
+#include "nsIInputStream.h"

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Ancient
User impact if declined: Easy to crash the browser
Testing completed: landed to m-c yesterday
Risk to taking this patch (and alternatives if risky): Should be very safe 
String or UUID changes made by this patch:NA

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/43ae7b0d038a
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/43ae7b0d038a
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/fe5faf249b9c

Comment 37

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on Fx33, and verified fixed on Fx34, Fx35 and Fx36, 2014-11-19. 
Also confirmed fixed on RC2 of Fx31.3.0esr.