Closed Bug 994337 (CVE-2015-4503) Opened 8 years ago Closed 6 years ago

mozTCPSocket leaks client memory to server

Categories

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

28 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed
b2g18 --- ?
b2g-v1.1hd --- ?
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- ?
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.2 --- ?

People

(Reporter: dchanm+bugzilla, Assigned: jdm)

Details

(Keywords: sec-moderate, Whiteboard: [post-critsmash-triage][adv-main41-])

Attachments

(1 file, 1 obsolete file)

I noticed something weird while testing the TCPSocket implementation.

Linux bob 3.5.0-48-generic #72~precise1-Ubuntu SMP Tue Mar 11 20:09:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0

STR
1. run nc on server, depends on distro
nc -l -v 8000
or
nc -l -p 8000
2. Enable mozTCPSocket by adding the about:config value
dom.mozTCPSocket.enabled = true
3. Open up web console in chrome privileged page, e.g. about:config

var sock = navigator.mozTCPSocket.open("localhost", 8000, {binaryType: "string"})
sock.send(new Uint8Array(65535))

You may have to call send() more than once.

Result
netcat shows output beginning with
[object Uint8Array]
followed by a bunch of gibberish / strings which appear to be from the Firefox process

Expected
No output or error
Ugh, this is broken.  We stringify the object and get [object Uint8Array], but we get the length from .length, which is 64k.  And then we happily read off the end of the buffer ....

This sounds awfully familiar.
It appears the B2G multi-process code takes a different path. The bug doesn't directly affect it

Call socketbridge to perform actual send
http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocket.js#666

Check the object type and associated lengths
http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocketChild.cpp#200

Return from child before bad code at TCPSocket.js#696 is reached
http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocket.js#683


I haven't looked deeply into the C++ length checks to see if there are boundary conditions.
I tested this with the Simulator and it seems to affect Firefox OS 1.2 and everything newer.
Setting tracking flags (to "?" for untested versions).
I tested this in firefox-18.0.2, so I *guess* all b2g are affected.
I think the simulator doesn't use OOP as far as I know, so this may be a reason why the simulator is affected and not the device (if comment 2 is correct - we should test on device.)
Alias: foxbleed
Keywords: sec-high
Is sec-high over-stating the problem? This is not exposed to web content and may not affect Firefox OS. If the problem is limited to broken (not sending the data they intended) or malicious apps running in the desktop/fennec WebRT then sec-moderate may be more appropriate.
Group: core-security
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (Away 4/19-5/7) from comment #1)
> Ugh, this is broken.  We stringify the object and get [object Uint8Array],
> but we get the length from .length, which is 64k.  And then we happily read
> off the end of the buffer ....

How do we do that in a JS-implemented API? Is this bug in mozTCPSocket or some underlying glue that could affect other APIs?
(In reply to Daniel Veditz [:dveditz] from comment #7)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (Away 4/19-5/7) from
> comment #1)
> > Ugh, this is broken.  We stringify the object and get [object Uint8Array],
> > but we get the length from .length, which is 64k.  And then we happily read
> > off the end of the buffer ....
> 
> How do we do that in a JS-implemented API? Is this bug in mozTCPSocket or
> some underlying glue that could affect other APIs?

It's a bug in the implementation (I believe Kyle was talking about <http://mxr.mozilla.org/mozilla-central/source/dom/network/src/TCPSocket.js#670>).  TCPSocket currently doesn't use WebIDL and just accepts a jsval and gets things wrong.
nsIStringInputStream is a bit of a footgun when used from JS ...

But yes, this would not be an issue if TCPSocket used WebIDL.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (Away 4/19-5/7) from comment #1)
> but we get the length from .length, which is 64k.  And then we happily read off the
> end of the buffer ....
>
> This sounds awfully familiar.

The irony is that dchan was trying to use this interface to detect servers that suffer from Heartbleed.
Group: dom-core-security
Keywords: sec-highsec-moderate
Finally got a setup to test on device and couldnt reproduce on the Flame (master, build date 2014040911111125) or Peak (1.2, build date 20140410205903)
Resetting tracking flags for b2g... :)
This bug is easy to test and find. I wonder if we should CC more people to make sure this gets attention.
Any suggestions? Judging from the comments in bug 745283, it seems that UDPSocket was implemented by staring at TCPSocket. I would not be surprised if this will expose a leak just the same, but I could not make UDPSocket work in Nightly (Chrome privileges, pref enabled) yet.
FWIW. I could make UDPSocket work, it's not vulnerable.
This seems pretty serious. Adding Donovan and jdm who both have hacked on the TCPSocket API.
Josh, can you take a look please?
Flags: needinfo?(josh)
This should fix the problem. Sorry for taking so long to look into it.
Attachment #8614307 - Flags: review?(bugmail)
Assignee: nobody → josh
Status: NEW → ASSIGNED
Flags: needinfo?(josh)
Now with a test.
Attachment #8614320 - Flags: review?(bugmail)
Attachment #8614307 - Attachment is obsolete: true
Attachment #8614307 - Flags: review?(bugmail)
Comment on attachment 8614320 [details] [diff] [review]
Force sending strings over TCPSocket when strings are required

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

r=asuth.  Analysis for my own benefit/sanity follows.

Re-statement of problem and existing code-flow as I understand it:
- nsIStringInputStream::SetData has this IDL signature:
    void setData(in string data, in long dataLen);
  which maps to this footgun call-signature which doesn't have XPConnect enforcing any consistency constraint between "data" and "dataLen:
    nsStringInputStream::SetData(const char* aData, int32_t aDataLen)
- nsIDOMTCPSocket.send has a signature of "in jsval data" to allow it to take an arraybuffer or string, with potential footgun consequences.
- Varying by process configuration:
  - multiprocess:
    - the "any" gets constrained by the implementation of TCPSocketChild::SendSend which enforces that the payload is either a string or an array buffer.  NS_ERROR_FAILURE is generated if it's not.
      - The string is serialized as a string with the length/offset ignored, so lies/screw-ups aren't possible there.
      - Arraybuffer length and offsets enforced by sane std::min() logic.
    - in the parent, arraybuffer is fine.  String is also fine because the XPIDL "string" type is TD_PSTRING/T_CHAR_STR which bottoms out in a call to JS_EncodeStringToBuffer which viciously truncates everything down to chars.
  - single process
    - the "any" gets passed through, although XPConnect should be double-wrapping an XPCWrappedNative around an XPCWrappedJS for objects.  This means length should be the native implementation (or undefined), and toString should always produce something safe/consistent unless we pierce the wrapper.
    - Without the fix, in non-"arraybuffer" mode `data` is always passed directly to setData without transformation or type verification.  The T_CHAR_STR implementation always calls ToString on what's passed-in.
    - Without the fix, in non-"arraybuffer" mode (e.g. string), any passed-in `data` which exposes a `length` (XPConnect allowing) that is inconsistent with data.toString()'s length will potentially result in reads beyond the allocated bounds of the string.
    - In the specific case of a Uint8Array (which has a native "length" implementation) and a toString() that doesn't match up, the reported badness happens.

The fix:
- In non-"arraybuffer" mode the code explicitly coerces `data` to a string and takes its length.  This ensures consistency.  Malicious/incorrect `data` payloads will tend to result in things like "[xpconnect yay just what you wanted]" being sent instead of other stuff.

What about jerky values and array buffers?:
- nsIArrayBufferInputStream's setData implementation performs type-checking and enforces bounds-checking on byteOffset/byteLength which means that we're not going to read beyond the end of an array.  (However, the lack of enforcement of byteLength and byteOffset in the JS logic means that the speculative newBufferedAmount value in the multiprocess child can temporarily result in an overestimation.  However, this will correct itself within a few messages from the parent to the child.)
Attachment #8614320 - Flags: review?(bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/3d1610589d6a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Group: core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main41+]
Alias: foxbleed → CVE-2015-4503
Flags: sec-bounty?
(This was reported when David Chan was working for Mozilla.)
Flags: sec-bounty?
Alias: CVE-2015-4503
Whiteboard: [post-critsmash-triage][adv-main41+] → [post-critsmash-triage][adv-main41-]
Restoring the CVE because it's published in an advisory. Assuming it was removed by accident.
Alias: CVE-2015-4503
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.