Bug 1287266 (CVE-2016-5261)

Integer overflow and memory corruption in WebSocketChannel

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mozilla, Assigned: michal.novotny)

Tracking

({csectype-intoverflow, sec-high})

47 Branch
mozilla50
csectype-intoverflow, sec-high
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox47 wontfix, firefox48 fixed, firefox49 fixed, firefox-esr4549+ fixed, firefox50 fixed)

Details

(Whiteboard: [necko-active][adv-main48+][adv-esr45.4+])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
poctestcase
Created attachment 8771665 [details]
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

2 years ago
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)

Updated

2 years ago
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Flags: needinfo?(jld)
(Assignee)

Comment 3

2 years ago
Created attachment 8772804 [details] [diff] [review]
fix
Attachment #8772804 - Flags: review?(mcmanus)
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;
}
Attachment #8772804 - Flags: feedback-
(Assignee)

Comment 5

2 years ago
Created attachment 8772816 [details] [diff] [review]
fix
Attachment #8772804 - Attachment is obsolete: true
Attachment #8772804 - Flags: review?(mcmanus)
Attachment #8772816 - Flags: review?(mcmanus)
(Assignee)

Updated

2 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8772816 [details] [diff] [review]
fix

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

Thanks Michal - and thanks for the CheckedInt shoutout
Attachment #8772816 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 7

2 years ago
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
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?
status-firefox47: --- → wontfix
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox-esr45: --- → ?
Yes, this needs sec-approval to go into trunk (and anywhere else).
Flags: sec-bounty?
status-firefox-esr45: ? → affected
(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
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Flags: needinfo?(abillings)
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
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)

Comment 15

2 years ago
(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)
(Assignee)

Comment 16

2 years ago
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)
Keywords: sec-high
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.
tracking-firefox-esr45: --- → 48+
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 18

2 years ago
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
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.
tracking-firefox-esr45: 48+ → 49+
Flags: needinfo?(sledru)
Group: network-core-security → core-security-release
Comment on attachment 8772816 [details] [diff] [review]
fix

sec-high, taking it to esr45 - 45.4.0
Attachment #8772816 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-esr45/rev/bc2f5467b33d
status-firefox-esr45: affected → fixed
Whiteboard: [necko-active][adv-main48+] → [necko-active][adv-main48+][adv-esr45.4+]
Group: core-security-release
Keywords: csectype-intoverflow
You need to log in before you can comment on or make changes to this bug.