1287266 - (CVE-2016-5261) Integer overflow and memory corruption in WebSocketChannel

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect)

Description

[Samuel Groß]

Attachment: 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)
      break;

|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
libsystem_platform.dylib`_platform_memmove$VARIANT$Haswell:
->  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 #1: 0x0000000102f91eb8 XUL`mozilla::net::WebSocketChannel::UpdateReadBuffer(this=0x00000001217c3000, buffer="AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"..., count=2048, accumulatedFragments=1024, available=0x000070000029ee0c) + 1096 at WebSocketChannel.cpp:1481
    frame #2: 0x0000000102f92088 XUL`mozilla::net::WebSocketChannel::ProcessInput(this=0x00000001217c3000, buffer="AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"..., count=2048) + 360 at WebSocketChannel.cpp:1512
    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 message_loop.cc:235
    frame #12: 0x00000001030f4e75 XUL`MessageLoop::RunHandler(this=0x000000010072b460) + 21 at message_loop.cc:228
    frame #13: 0x00000001030f4e1d XUL`MessageLoop::Run(this=0x000000010072b460) + 45 at message_loop.cc:208
    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


[1] http://en.cppreference.com/w/c/language/conversion#Usual_arithmetic_conversions

Comment 1

[:Gijs (he/him)]

Pat/Jed/Michal, can you look into this?

Comment 2

[Patrick McManus [:mcmanus]]

thanks - michal can fix this up.

Comment 3

[Michal Novotny [:michal]]

Attachment: fix

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8772804 [details] [diff] [review]
fix

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) {
  return NS_ERROR_FILE_TOO_BIG;
}

Comment 5

[Michal Novotny [:michal]]

Attachment: fix

Comment 6

[Patrick McManus [:mcmanus]]

Comment on attachment 8772816 [details] [diff] [review]
fix

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

Thanks Michal - and thanks for the CheckedInt shoutout

Comment 7

[Michal Novotny [:michal]]

Comment on attachment 8772816 [details] [diff] [review]
fix

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

I think we need a sec approval here.
I guess esr45 is also impacted?

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Yes, this needs sec-approval to go into trunk (and anywhere else).

Comment 10

[Carsten Book [:Tomcat]]

(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 https://hg.mozilla.org/integration/mozilla-inbound/rev/311e127edfff and got now merged in:

https://hg.mozilla.org/mozilla-central/rev/311e127edfff

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8772816 [details] [diff] [review]
fix

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

Comment 12

[Al Billings [:abillings - ex-MoCo]]

This needs a security rating before we can know and the sec-approval? template questions answered.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/99b62d79d2e4f8e8e5cb606891c1df80f0771bd1

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/68ca20cf8042

Comment 15

[:Gijs (he/him)]

(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?

Comment 16

[Michal Novotny [:michal]]

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.

Comment 17

[Daniel Veditz [:dveditz]]

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.

Comment 18

[Michal Novotny [:michal]]

Comment on attachment 8772816 [details] [diff] [review]
fix

[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

Comment 19

[Sylvestre Ledru [:Sylvestre]]

This is too late for esr, we already built it and we have the sign off.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8772816 [details] [diff] [review]
fix

sec-high, taking it to esr45 - 45.4.0

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/bc2f5467b33d