Closed Bug 1109683 Opened 5 years ago Closed 5 years ago

Compare the first character of start_time and stop_time instead of the array itself; r=abr

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox37 --- fixed
firefox-esr31 --- wontfix
b2g-v2.2 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

Details

(Keywords: sec-low, Whiteboard: [adv-main37-][b2g-adv-main2.2-])

Attachments

(1 file)

Caught by clang ToT warning:
comparison of array 'sdp_p->timespec_p->start_time' equal to a null pointer is always false
Assignee: nobody → ehsan.akhgari
Caught by clang ToT warning:
comparison of array 'sdp_p->timespec_p->start_time' equal to a null pointer is always false
Attachment #8534375 - Flags: review?(adam)
Comment on attachment 8534375 [details] [diff] [review]
Compare the first character of start_time and stop_time instead of the array itself; r=abr

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

Looks good to me.
Attachment #8534375 - Flags: review?(adam) → review+
Group: core-security
Ehsan: I had jesup mark this for security, since I'm not sure whether it can be exploited via a malformed t= line to execute an unexpected code path.
https://hg.mozilla.org/mozilla-central/rev/9f63a47fc181
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Does this impact ESR31? Also does it have a rating?
Flags: needinfo?(ehsan.akhgari)
Not sure, Adam?
Flags: needinfo?(ehsan.akhgari) → needinfo?(adam)
(In reply to Benjamin Kerensa [:bkerensa] from comment #5)
> Does this impact ESR31? Also does it have a rating?

tl;dr: Yes, and "sec-low"
____

This is in all codebases dating back to the initial landing of WebRTC, which is here (Tue Mar 13 11:22:14 2012 -0700):

https://hg.mozilla.org/mozilla-central/rev/f1b524d51e6b

This would include release from 13 forward, although it wasn't exposed to content until 19. So it would be exposed in releases 19 through 36, and ESRs 24 and 31.

In practice, after reading the code in more depth, I suspect there isn't an exploit here: by my reading, the parser should reject malformed lines before they're stored in this structure, and even if they make it through, the worse case is that they're copied into an outgoing t= line, which will be similarly malformed.

Based on this quick read-through of the code, I would suggest sec-low at the highest: the current flaw is poor hygiene, but doesn't appear to be exploitable.
Flags: needinfo?(adam)
Whiteboard: [adv-main37-]
Whiteboard: [adv-main37-] → [adv-main37-][b2g-adv-main2.2-]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.