1220493 - (CVE-2015-7205) Underflow in RTPReceiverVideo::ParseRtpPacket causes memory-safety bug and information leak

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[q1]

RTPReceiverVideo::ParseRtpPacket (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_receiver_video.cc) will underflow if an incoming packet is malformed such that its padding length is greater than its payload length. This underflow then causes ParseRtpPacket to pass to its callback function (which eventualy passes it to a video decoder) a buffer length that exceeds the buffer's actual length. The receiver's decoder then reads that buffer and displays the contents as video (which usually, but not always, appears as trash). If the receiver happens to be sharing the tab containing the decoded video with another user via Firefox Hello, the other user will receive the video, thus leaking private information to the other user.

The bug is still present in the current trunk: http://hg.mozilla.org/mozilla-central/file/96377bdbcdf3/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_receiver_video.cc ; http://hg.mozilla.org/mozilla-central/file/96377bdbcdf3/media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc .


Details
-------

The bug is in RTPReceiverVideo::ParseRtpPacket lines 72-3, which don't check for underflow:

72:   const uint16_t payload_data_length =
73:       payload_length - rtp_header->header.paddingLength;

Equivalently, the bug could be considered to be in RtpHeaderParser::Parse (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_utility.cc), which doesn't verify that the padding fits within the packet, or even in RtpDepacketizer::Parse and its overriding functions (ditto).

(BTW, there is also a similar underflow in RtpReceiverImpl::IncomingRtpPacket line 201, but it appears to be harmless because the result appears to be used only for statistics).


A (rather involved) debugger-based proof-of-concept goes like this:

1. Start FF 41.0 x86 debug build with -profilemanager -no-remote. Call this session the "sender". Create a new profile ("sender profile"), run FF with it, and attach a debugger to it.

2. Patch the sender's code in RTPSender::CreateRTPHeader (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_sender.cc) line 1103 to initialize |header [0]| to 0xa0 instead of 0x80 (this sets the P ("padding") bit in the header, which causes RtpHeaderParser::Parse to interpret the last byte of the packet as a count of padding bytes, and to assign it to |header.paddingLength|).

3. Patch the sender's code in RTPSenderVideo::Send (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_sender_video.cc) lines 358-64 to use 0x20 for the outgoing packet length instead of |payload_bytes_in_packet|. To do this, replace the sequence

   movzx edx, word ptr [ebp-620h]   ; 0f b7 95 e0 f9 ff ff
   push  edx                        ; 52

with

   push  20h                        ; 6a 20
   nop                              ; 90
   nop                              ; 90
   nop                              ; 90
   nop                              ; 90
   nop                              ; 90
   nop                              ; 90
   
4. Patch out the ASSERT in CreateBoxShadow (gfx\thebes\gfxBlur.cpp in 445) to avoid an unrelated spurious crash due to https://bugzilla.mozilla.org/show_bug.cgi?id=1181028 and similar bugs.

5. Let execution proceed and detach the debugger from the sender.

6. Play a long video in the sender.

7. Start FF 41.0 x86 debug build with -profilemanager -no-remote. Call this session the "receiver". Create a new profile ("receiver profile"), run FF with it, and attach a debugger to it.

8. Create an FF Hello conversation in the sender. Copy the link and open it in the receiver.

9. When the conversation is connected, share the sender's tabs.

10. Watch the video trash in the receiver's tab until the receiver crashes with a read access violation.

11. Trace back the stack into ViEReceiver::OnReceivedPayloadData and note that |payload_size| is about 0xffxx. Note also that one or more pages in the range [|payload_data|, |payload_data+payload_size-1] are inaccessible, and that the crash read-access violation address is in this range.

12. Trace back the stack into RtpReceiverVideo::ParseRtpPacket and examine |payload_data_length| on line 72. Notice that it's about 0xffxx. Examine |payload_length| and notice it's 0x20, and examine |rtp_header->header.paddingLength| and notice it's > 0x20.

13. You can show that the receiver leaks information to another user by restarting the receiver and re-establishing the Hello session. Then start a third FF session with -profilemanager -no-remote, start another Hello session between the receiver and the third session, and share the receiver's tab with the third session. Notice how the trash displayed in the receiver's tab is also displayed in the third session's window.


Typically the receiver displays garbage for a few seconds to a few minutes, then crashes with a read access violation, sometimes in ViEReceiver::OnReceivedPayloadData where it calls vcm_->IncomingPacket, sometimes near the end of VCMJitterBuffer::GetFrame, sometimes elsewhere.

Comment 1

[q1]

Note that an attacker can determine exactly how far she wants RTPReceiverVideo::ParseRtpPacket's callback function to read beyond the end of a given packet, by manipulating the packet length and padding length. Thus, she probably can avoid crashing the receiver nearly indefinitely. while misdirecting (and possibly extracting) small amounts of data with each packet she sends.

Comment 2

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

Can you assign a security rating (or suggest one), Mr. Jesup?

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: validate RTP packets against underflows

This covers both this bug, and Bug 1219814

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: validate RTP packets against underflows

refactor for cleaner and more readable checks

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8683685 [details] [diff] [review]
validate RTP packets against underflows

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  
Pretty tough.  Clearly there's improvement in the bounds-checking, but constructing an exploit from this would be very rough.  You could force a crash.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? 
They somewhat highlight it, but not much more than the code.

Which older supported branches are affected by this flaw?
All

If not all supported branches, which bug introduced the flaw?
Upstream bug which we'll feed back to webrtc.org.  Note they don't appear to have a secure-bug flag, so I may need to email it to them, or just file it as an open bug.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply trivially to all branches.

How likely is this patch to cause regressions; how much testing does it need?  
Very little chance of regression; I've tested it in basic webrtc calls with no problems.  Mochitests cover normal operation well.

Comment 6

[q1]

It'd be good to test against the POC if you haven't already.

Comment 7

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

Comment on attachment 8683685 [details] [diff] [review]
validate RTP packets against underflows

sec-approval+ for trunk.

Comment 8

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

We'll want patches everywhere for this unless there is a good reason not to do so.

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Hi, do you want to request uplift for this?

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8683685 [details] [diff] [review]
validate RTP packets against underflows

Approval Request Comment
[Feature/regressing bug #]: N/A

[User impact if declined]: Crafted attack packets could cause a crash.  It's unlikely more could be done, but it does allow bounds read violations (small offsets before and after the packet) and may allow for very indirect information leakage (again, very hard to exploit).

[Describe test coverage new/current, TreeHerder]: None.  Would require a relatively fancy c++ unit test.

[Risks and why]: Very low risk.  Just improves bounds-checking.

[String/UUID change made/needed]: none

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8683685 [details] [diff] [review]
validate RTP packets against underflows

Crash fix, ok to uplift to aurora and beta.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/48d8d425cff1

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/48d8d425cff1

Comment 14

[Carsten Book [:Tomcat]]

Hi, this failed to apply to aurora:

merging media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc
warning: conflicts during merge.
merging media/webrtc/trunk/webrtc/modules/rtp_rtcp/source/rtp_utility.cc incomplete! (edit conflicts, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Jesup could you take a look ? Thanks

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Yup - I had to rebase the patch since we landed the webrtc_43 update the day before (which touched that code).  Aurora/beta should use the patch here on the bug.  THanks!

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/575d3aa376b1
https://hg.mozilla.org/releases/mozilla-beta/rev/2c1b396ef5c3

nice, thanks jesup!

Comment 17

[Daniel Veditz [:dveditz]]

(In reply to Randell Jesup [:jesup] from comment #3)
> Created attachment 8682698 [details] [diff] [review]
> validate RTP packets against underflows
> 
> This covers both this bug, and Bug 1219814

Would this patch have covered the other bug regardless (because they're the same kind of error in the same code) or was the fix here improved because Ron reported that one?

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Similar but not identical error.  Bug 1219814 was fixed in the webrtc.org 43 update from upstream which was underway when these were filed; this bug appears to be fixed (in a different manner) in upstream post-43 in webrtc bug 4771 (https://codereview.webrtc.org/1220863002); we'll want to make a fix to rtp_payload_registry.cc as well probably, and consider adding the debug-build check to rtp_receiver_video.cc.

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8683685 [details] [diff] [review]
validate RTP packets against underflows

sec-high, taking in it esr too.

Comment 20

[Carsten Book [:Tomcat]]

http://hg.mozilla.org/releases/mozilla-esr38/rev/8882c22e0d57