Closed Bug 1226387 Opened 5 years ago Closed 5 years ago

Latent missing check and truncation in RtpHeaderParser::ParseRtcp could cause memory-safety bug

Categories

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

42 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox43 --- wontfix
firefox44 --- wontfix
firefox45 --- disabled
firefox46 --- fixed
firefox-esr38 --- disabled
firefox-esr45 --- disabled
b2g-v2.5 --- affected
b2g-master --- fixed

People

(Reporter: q1, Assigned: jesup)

Details

(Keywords: csectype-bounds, sec-low, Whiteboard: [post-critsmash-triage][adv-main46-])

Attachments

(1 file)

RtpHeaderParser::ParseRtcp (media\webrtc\trunk\webrtc\modules\rtp_rtcp\source\rtp_utility.cc) does not check whether the header specified by the packet actually lies within the memory that contains the packet. This bug could cause a caller to read memory to which it has no right, and possibly to do other insecure things in consequence. This function also can truncate a result, with unclear consequences.

These bugs are latent because ParseRtcp's only current caller is test code (ReadPacket in media\webrtc\trunk\webrtc\test\rtp_file_reader.cc and media\webrtc\trunk\webrtc\video_engine\test\auto_test\automated/vie_network_test.cc)


Details
-------

The first bug is the lack of a test against |_ptrRTPDataEnd| following line 299. The second bug is that line 299 can truncate the assignment of the RHS to the uint16_t |header->headerLength|.

275: bool RtpHeaderParser::ParseRtcp(RTPHeader* header) const {
276:   assert(header != NULL);
277: 
278:   const ptrdiff_t length = _ptrRTPDataEnd - _ptrRTPDataBegin;
279:   if (length < kRtcpMinParseLength) {
280:     return false;
281:   }
282: 
283:   const uint8_t V = _ptrRTPDataBegin[0] >> 6;
284:   if (V != kRtcpExpectedVersion) {
285:     return false;
286:   }
287: 
288:   const uint8_t PT = _ptrRTPDataBegin[1];
289:   const uint16_t len = (_ptrRTPDataBegin[2] << 8) + _ptrRTPDataBegin[3];
290:   const uint8_t* ptr = &_ptrRTPDataBegin[4];
291: 
292:   uint32_t SSRC = *ptr++ << 24;
293:   SSRC += *ptr++ << 16;
294:   SSRC += *ptr++ << 8;
295:   SSRC += *ptr++;
296: 
297:   header->payloadType  = PT;
298:   header->ssrc         = SSRC;
299:   header->headerLength = 4 + (len << 2);
300: 
301:   return true;
302: }

Ironically, the function does check against |_ptrRTPDataEnd| on lines 278-9.
Summary: Latent overflow in RtpHeaderParser::ParseRtcp could cause memory-safety bug → Latent missing check and truncation in RtpHeaderParser::ParseRtcp could cause memory-safety bug
Group: core-security → media-core-security
Flags: sec-bounty?
sec-moderate since it's not used in our builds as you noted (and we don't run the test suite that does use it).  Patch coming.  Thanks!
Assignee: nobody → rjesup
Rank: 15
Priority: -- → P1
Flags: sec-bounty? → sec-bounty+
no need to check for overflow; headerLength is now a size_t
Attachment #8707783 - Flags: review?(pkerr)
Attachment #8707783 - Flags: review?(pkerr) → review+
https://hg.mozilla.org/mozilla-central/rev/1923d83bb316
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Group: media-core-security → core-security-release
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main46+]
Alias: CVE-2016-2819
"disabled" isn't quite right but "wontfix" can be interpreted to mean those older branches didn't have the bug we fixed. "unaffected" would be in the ballpark, too, but is also kind of wrong since the buggy code is clearly present even if we don't use it.
Removing CVE and advisory flags as, in discussion with Dan, we aren't going to write an advisory to an issue that can't affect us in practice.
Alias: CVE-2016-2819
Whiteboard: [post-critsmash-triage][adv-main46+] → [post-critsmash-triage][adv-main46-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.