Closed Bug 1287266 (CVE-2016-5261) Opened 8 years ago Closed 8 years ago

Integer overflow and memory corruption in WebSocketChannel


(Core :: Networking: WebSockets, defect)

47 Branch
Not set



Tracking Status
firefox47 --- wontfix
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 49+ fixed
firefox50 --- fixed


(Reporter: mozilla, Assigned: michal)


(Keywords: csectype-intoverflow, sec-high, Whiteboard: [necko-active][adv-main48+][adv-esr45.4+])


(2 files, 1 obsolete file)

Attached file Proof of Concept code
From WebSocketChannel::ProcessInput

    if (payloadLength64 + mFragmentAccumulator > mMaxMessageSize) {
      return NS_ERROR_FILE_TOO_BIG;

|payloadLength64| is taken from the incoming websocket frame, and thus attacker controlled, while |mFragmentAccumulator| is the total size of all previous fragments belonging to the current message. The check tries to ensure that no incoming message is ever larger than |kMaxMessageSize|, which itself is at most INT32_MAX. However, The check is incorrect due to the types of the operands:

    int64_t  payloadLength64
    uint32_t mFragmentAccumulator;
    int32_t  mMaxMessageSize;

According to the arithmetic conversion rules [1], |mFragmentAccumulator| and |kMaxMessageSize| will be converted to int64_t before the comparison. As such the comparison is signed and will evaluate to false if the sum becomes negative. This can be achieved as follows:

    1. Send a websocket fragment of size X (where X is less than |kMaxMessageSize|)

    2. Send a second websocket fragment of size 0x7fffffffffffffff

The sum will then become negative and the check fails.

What happens then?

    uint32_t payloadLength = static_cast<uint32_t>(payloadLength64);

    if (avail < payloadLength)

|payloadLength| will be 0xffffffff and so ProcessInput will defer processing of the message until this many bytes have been received. Incoming data is thus buffered internally until a whole message has been received. WebSocketChannel::UpdateReadBuffer does this. Let's see what happens there:

    } else {
        // existing buffer is not sufficient, extend it
        mBufferSize += count + 8192 + mBufferSize/3;
        LOG(("WebSocketChannel: update read buffer extended to %u\n", mBufferSize));
        uint8_t *old = mBuffer;
        mBuffer = (uint8_t *)realloc(mBuffer, mBufferSize);
        if (!mBuffer) {
          mBuffer = old;
          return false;
        mFramePtr = mBuffer + (mFramePtr - old);

      ::memcpy(mBuffer + mBuffered, buffer, count);

The buffer will be resized multiple times. However, at some point 'count + 8192 + mBufferSize/3' will overflow, resulting in realloc actually shrinking the array. The following memcpy will then write controlled data at a known offset from the new buffer.

Attached is an html file and a python script. The html file sets up the websocket connection while the python script implements a simple websocket server and sends the malicious frames once a client is connected. To reproduce: start the python script on localhost and open websocket.html via file://. The python script should start printing progress information and at some point the browser will crash.

The backtrace at the time of the crash:

Process 75634 stopped
* thread #8: tid = 0x206a90, 0x00007fff882ed01c libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 252, name = 'Socket Thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x26d51700a)
    frame #0: 0x00007fff882ed01c libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 252
->  0x7fff882ed01c <+252>: vmovups ymmword ptr [rax], ymm0
    0x7fff882ed020 <+256>: vmovups ymm2, ymmword ptr [rsi + 0x20]
    0x7fff882ed025 <+261>: add    rsi, 0x40
    0x7fff882ed029 <+265>: sub    rdx, 0x80
(lldb) bt
* thread #8: tid = 0x206a90, 0x00007fff882ed01c libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 252, name = 'Socket Thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x26d51700a)
  * frame #0: 0x00007fff882ed01c libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell + 252
    frame #3: 0x0000000102fa06b2 XUL`mozilla::net::WebSocketChannel::OnInputStreamReady(this=0x00000001217c3000, aStream=0x0000000122ee5630) + 898 at WebSocketChannel.cpp:3937
    frame #4: 0x00000001027c6489 XUL`nsInputStreamReadyEvent::Run(this=0x000000011a713880) + 137 at nsStreamUtils.cpp:95
    frame #5: 0x00000001027fabeb XUL`nsThread::ProcessNextEvent(this=0x0000000100719100, aMayWait=true, aResult=0x000070000029f88e) + 1211 at nsThread.cpp:1067
    frame #6: 0x0000000102885e4c XUL`NS_ProcessNextEvent(aThread=0x0000000100719100, aMayWait=true) + 140 at nsThreadUtils.cpp:290
    frame #7: 0x00000001029e8a74 XUL`mozilla::net::nsSocketTransportService::Run(this=0x0000000100719000) + 1572 at nsSocketTransportService2.cpp:911
    frame #8: 0x00000001027fabeb XUL`nsThread::ProcessNextEvent(this=0x0000000100719100, aMayWait=false, aResult=0x000070000029fbfe) + 1211 at nsThread.cpp:1067
    frame #9: 0x0000000102885e4c XUL`NS_ProcessNextEvent(aThread=0x0000000100719100, aMayWait=false) + 140 at nsThreadUtils.cpp:290
    frame #10: 0x00000001031f7c86 XUL`mozilla::ipc::MessagePumpForNonMainThreads::Run(this=0x000000011a82b5c0, aDelegate=0x000000010072b460) + 646 at MessagePump.cpp:354
    frame #11: 0x00000001030f4f15 XUL`MessageLoop::RunInternal(this=0x000000010072b460) + 117 at
    frame #12: 0x00000001030f4e75 XUL`MessageLoop::RunHandler(this=0x000000010072b460) + 21 at
    frame #13: 0x00000001030f4e1d XUL`MessageLoop::Run(this=0x000000010072b460) + 45 at
    frame #14: 0x00000001027f8538 XUL`nsThread::ThreadFunc(aArg=0x0000000100719100) + 472 at nsThread.cpp:467
    frame #15: 0x000000010235692d libnss3.dylib`_pt_root(arg=0x0000000100734eb0) + 429 at ptthread.c:216
    frame #16: 0x00007fff894cf99d libsystem_pthread.dylib`_pthread_body + 131
    frame #17: 0x00007fff894cf91a libsystem_pthread.dylib`_pthread_start + 168
    frame #18: 0x00007fff894cd351 libsystem_pthread.dylib`thread_start + 13

Pat/Jed/Michal, can you look into this?
Flags: needinfo?(michal.novotny)
Flags: needinfo?(mcmanus)
Flags: needinfo?(jld)
thanks - michal can fix this up.
Group: firefox-core-security → network-core-security
Component: Security → Networking: WebSockets
Flags: needinfo?(mcmanus)
Product: Firefox → Core
Whiteboard: [necko-active]
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Flags: needinfo?(jld)
Attached patch fix (obsolete) — Splinter Review
Attachment #8772804 - Flags: review?(mcmanus)
Comment on attachment 8772804 [details] [diff] [review]

Review of attachment 8772804 [details] [diff] [review]:

::: netwerk/protocol/websocket/WebSocketChannel.cpp
@@ +1568,5 @@
>      LOG(("WebSocketChannel::ProcessInput: payload %lld avail %lu\n",
>           payloadLength64, avail));
> +    if (payloadLength64 > UINT32_MAX) {

What about:

CheckedInt<in64_t> payloadLengthChecked(payloadLength64);
payloadLengthChecked += mFragmentAccumulator;
if (!payloadLengthChecked.isValid() || payloadLengthChecked.value() > mMaxMessageSize) {
Attachment #8772804 - Flags: feedback-
Attached patch fixSplinter Review
Attachment #8772804 - Attachment is obsolete: true
Attachment #8772804 - Flags: review?(mcmanus)
Attachment #8772816 - Flags: review?(mcmanus)
Ever confirmed: true
Comment on attachment 8772816 [details] [diff] [review]

Review of attachment 8772816 [details] [diff] [review]:

Thanks Michal - and thanks for the CheckedInt shoutout
Attachment #8772816 - Flags: review?(mcmanus) → review+
Comment on attachment 8772816 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: probably 640003
[User impact if declined]: memory corruption caused by malicious page
[Describe test coverage new/current, TreeHerder]: tested manually against test provided by reporter
[Risks and why]: low, simple integer overflow fix
[String/UUID change made/needed]: none
Attachment #8772816 - Flags: approval-mozilla-beta?
Attachment #8772816 - Flags: approval-mozilla-aurora?
I think we need a sec approval here.
I guess esr45 is also impacted?
Yes, this needs sec-approval to go into trunk (and anywhere else).
Flags: sec-bounty?
(In reply to Sylvestre Ledru [:sylvestre] from comment #8)
> I think we need a sec approval here.
> I guess esr45 is also impacted?

landed without sec-approval in and got now merged in:
Closed: 8 years ago
Flags: needinfo?(abillings)
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment on attachment 8772816 [details] [diff] [review]

Next time, please wait for the sec approval before landing it.
Taking it to beta and aurora as we don't have much choice and we need it for beta 10...

Al, should we take into esr45? Thanks
Attachment #8772816 - Flags: approval-mozilla-beta?
Attachment #8772816 - Flags: approval-mozilla-beta+
Attachment #8772816 - Flags: approval-mozilla-aurora?
Attachment #8772816 - Flags: approval-mozilla-aurora+
This needs a security rating before we can know and the sec-approval? template questions answered.
Flags: needinfo?(abillings)
(In reply to Al Billings [:abillings] from comment #12)
> This needs a security rating before we can know and the sec-approval?
> template questions answered.

--> michal?
Flags: needinfo?(michal.novotny)
The bug allows to write data provided by the web server outside the allocated buffer. I have no idea whether this can be used to execute native code or just to crash the firefox. I guess this is security-critical or security-high.
Flags: needinfo?(michal.novotny)
Flags: sec-bounty? → sec-bounty+
Alias: CVE-2016-5261
Whiteboard: [necko-active] → [necko-active][adv-main48+]
Do we need a different patch for the ESR-45 branch? We're basically out of time to get this into this release, please request esr-approval on the patch if it will work or on a new patch if it's needed.
Flags: needinfo?(michal.novotny)
Comment on attachment 8772816 [details] [diff] [review]

[Approval Request Comment]
User impact if declined: memory corruption caused by malicious page
Fix Landed on Version: 48
Risk to taking this patch (and alternatives if risky): low, simple integer overflow fix
String or UUID changes made by this patch: none
Flags: needinfo?(michal.novotny)
Attachment #8772816 - Flags: approval-mozilla-esr45?
Flags: needinfo?(sledru)
This is too late for esr, we already built it and we have the sign off.
Flags: needinfo?(sledru)
Group: network-core-security → core-security-release
Comment on attachment 8772816 [details] [diff] [review]

sec-high, taking it to esr45 - 45.4.0
Attachment #8772816 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Whiteboard: [necko-active][adv-main48+] → [necko-active][adv-main48+][adv-esr45.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.