994337 - (CVE-2015-4503) mozTCPSocket leaks client memory to server

Status: RESOLVED FIXED

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

Description

[David Chan [:dchan]]

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

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 2

[David Chan [:dchan]]

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.

Comment 3

[Frederik Braun [:freddy]]

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

Comment 4

[Frederik Braun [:freddy]]

I tested this in firefox-18.0.2, so I *guess* all b2g are affected.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

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

Comment 6

[Daniel Veditz [:dveditz]]

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.

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Kyle Huey [:khuey] ([e-mail]) (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?

Comment 8

[(no longer active)]

(In reply to Daniel Veditz [:dveditz] from comment #7)
> (In reply to Kyle Huey [:khuey] ([e-mail]) (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.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

nsIStringInputStream is a bit of a footgun when used from JS ...

But yes, this would not be an issue if TCPSocket used WebIDL.

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to Kyle Huey [:khuey] ([e-mail]) (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.

Comment 11

[Frederik Braun [:freddy]]

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

Comment 12

[Frederik Braun [:freddy]]

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.

Comment 13

[Frederik Braun [:freddy]]

FWIW. I could make UDPSocket work, it's not vulnerable.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This seems pretty serious. Adding Donovan and jdm who both have hacked on the TCPSocket API.

Comment 15

[Frederik Braun [:freddy]]

Josh, can you take a look please?

Comment 16

[Josh Matthews [:jdm]]

Attachment: Force sending strings over TCPSocket when strings are required

This should fix the problem. Sorry for taking so long to look into it.

Comment 17

[Josh Matthews [:jdm]]

Attachment: Force sending strings over TCPSocket when strings are required

Now with a test.

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

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

Comment 19

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1610589d6a

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3d1610589d6a

Comment 21

[Frederik Braun [:freddy]]

(This was reported when David Chan was working for Mozilla.)

Comment 22

[Daniel Veditz [:dveditz]]

Restoring the CVE because it's published in an advisory. Assuming it was removed by accident.