Closed Bug 1087633 (CVE-2014-1590) Opened 5 years ago Closed 5 years ago

XMLHttpRequest.prototype.send crashes when given a crafted object

Categories

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

29 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 + verified
firefox35 + verified
firefox36 + verified
firefox-esr31 34+ verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: joev, Assigned: smaug)

References

Details

(Keywords: sec-moderate, Whiteboard: [adv-main34+][adv-esr31.3+] thread checks seem to protect from the worst)

Attachments

(5 files, 1 obsolete file)

Attached image screenshot of call stack (obsolete) —
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.
Attachment #8509812 - Attachment is obsolete: true
Component: Untriaged → DOM
Product: Firefox → Core
We seem to pass js as nsIInputStream to necko.
It is not clear to me why we let this behavior.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Does anyone recall why send(nsIInputStream); isn't chrome only?
Flags: needinfo?(peterv)
Flags: needinfo?(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...
Flags: needinfo?(bzbarsky)
Attached patch v1Splinter Review
This is what I had as the first idea.
Attachment #8512888 - Flags: review?(bzbarsky)
...which is apparently the same what you proposed.
Comment on attachment 8512888 [details] [diff] [review]
v1

r=me
Attachment #8512888 - Flags: review?(bzbarsky) → review+
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
Flags: needinfo?(peterv)
Attachment #8512888 - Flags: sec-approval?
Attachment #8512888 - Flags: approval-mozilla-esr31?
Attachment #8512888 - Flags: approval-mozilla-beta?
Attachment #8512888 - Flags: approval-mozilla-aurora?
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?
Assignee: nobody → bugs
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Dan - Olli has said that he considers this sec-critical (comment 11). Do you agree? How do you want to handle landing?
Flags: needinfo?(dveditz)
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.
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
Does XHR access the stream off main thread in every case, even in Sync XHR?
Sync XHR is actually just async XHR + spinning event loop. So no difference there.
I tried this in a Worker script as well, but the type confusion does not seem possible in my testing.
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.
Flags: needinfo?(dveditz)
Whiteboard: thread checks seem to protect from the worst
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.
Attachment #8512888 - Flags: sec-approval?
Attachment #8512888 - Flags: sec-approval+
Attachment #8512888 - Flags: approval-mozilla-aurora?
Attachment #8512888 - Flags: approval-mozilla-aurora+
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?
Flags: needinfo?(bugs)
Attachment #8512888 - Flags: approval-mozilla-esr31?
Attachment #8512888 - Flags: approval-mozilla-esr31+
Attachment #8512888 - Flags: approval-mozilla-beta?
Attachment #8512888 - Flags: approval-mozilla-beta+
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.
Flags: needinfo?(bugs)
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.
Attached file stack3.txt
Stack trace from previously-mentioned crash
(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 ?
(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.
https://hg.mozilla.org/mozilla-central/rev/dad61f719326
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(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 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
Flags: needinfo?(bugs)
Attachment #8515667 - Flags: approval-mozilla-b2g32?
Attachment #8515667 - Flags: approval-mozilla-b2g30?
Attachment #8515667 - Flags: approval-mozilla-b2g32?
Attachment #8515667 - Flags: approval-mozilla-b2g32+
Attachment #8515667 - Flags: approval-mozilla-b2g30?
Attachment #8515667 - Flags: approval-mozilla-b2g30+
Depends on: 1096263
Depends on: 1096262
Whiteboard: thread checks seem to protect from the worst → [adv-main34+][adv-esr31.3+] thread checks seem to protect from the worst
Alias: CVE-2014-1590
Confirmed crash on Fx33, and verified fixed on Fx34, Fx35 and Fx36, 2014-11-19. 
Also confirmed fixed on RC2 of Fx31.3.0esr.
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.