Closed
Bug 1287266
(CVE-2016-5261)
Opened 9 years ago
Closed 9 years ago
Integer overflow and memory corruption in WebSocketChannel
Categories
(Core :: Networking: WebSockets, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: mozilla, Assigned: michal)
Details
(Keywords: csectype-intoverflow, reporter-external, sec-high, Whiteboard: [necko-active][adv-main48+][adv-esr45.4+])
Attachments
(2 files, 1 obsolete file)
1.78 KB,
application/zip
|
Details | |
1.07 KB,
patch
|
mcmanus
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
Sylvestre
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Pat/Jed/Michal, can you look into this?
Flags: needinfo?(michal.novotny)
Flags: needinfo?(mcmanus)
Flags: needinfo?(jld)
Comment 2•9 years ago
|
||
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•9 years ago
|
Assignee: nobody → michal.novotny
Flags: needinfo?(michal.novotny)
Updated•9 years ago
|
Flags: needinfo?(jld)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8772804 -
Flags: review?(mcmanus)
Comment 4•9 years ago
|
||
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•9 years ago
|
||
Attachment #8772804 -
Attachment is obsolete: true
Attachment #8772804 -
Flags: review?(mcmanus)
Attachment #8772816 -
Flags: review?(mcmanus)
Assignee | ||
Updated•9 years ago
|
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment 6•9 years ago
|
||
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•9 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?
Comment 8•9 years ago
|
||
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:
--- → ?
Comment 9•9 years ago
|
||
Yes, this needs sec-approval to go into trunk (and anywhere else).
Flags: sec-bounty?
Updated•9 years ago
|
Comment 10•9 years ago
|
||
(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
Closed: 9 years ago
Flags: needinfo?(abillings)
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 11•9 years ago
|
||
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+
Comment 12•9 years ago
|
||
This needs a security rating before we can know and the sec-approval? template questions answered.
Flags: needinfo?(abillings)
Comment 13•9 years ago
|
||
Comment 14•9 years ago
|
||
Comment 15•9 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•9 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)
Updated•8 years ago
|
Flags: sec-bounty? → sec-bounty+
Updated•8 years ago
|
Alias: CVE-2016-5261
Whiteboard: [necko-active] → [necko-active][adv-main48+]
Comment 17•8 years ago
|
||
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•8 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?
Updated•8 years ago
|
Flags: needinfo?(sledru)
Comment 19•8 years ago
|
||
This is too late for esr, we already built it and we have the sign off.
Flags: needinfo?(sledru)
Updated•8 years ago
|
Group: network-core-security → core-security-release
Comment 20•8 years ago
|
||
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+
Updated•8 years ago
|
Whiteboard: [necko-active][adv-main48+] → [necko-active][adv-main48+][adv-esr45.4+]
Updated•8 years ago
|
Group: core-security-release
Updated•8 years ago
|
Keywords: csectype-intoverflow
Updated•8 months ago
|
Keywords: reporter-external
You need to log in
before you can comment on or make changes to this bug.
Description
•