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

RESOLVED FIXED in Firefox 37, Firefox OS v2.2

Status

()

Core
WebRTC
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

({sec-low})

unspecified
mozilla37
sec-low
Points:
---

Firefox Tracking Flags

(firefox37 fixed, firefox-esr31 wontfix, b2g-v2.2 fixed)

Details

(Whiteboard: [adv-main37-][b2g-adv-main2.2-])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Caught by clang ToT warning:
comparison of array 'sdp_p->timespec_p->start_time' equal to a null pointer is always false
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan.akhgari
(Assignee)

Comment 1

4 years ago
Created attachment 8534375 [details] [diff] [review]
Compare the first character of start_time and stop_time instead of the array itself; r=abr

Caught by clang ToT warning:
comparison of array 'sdp_p->timespec_p->start_time' equal to a null pointer is always false
(Assignee)

Updated

4 years ago
Attachment #8534375 - Flags: review?(adam)

Comment 2

4 years ago
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+

Updated

4 years ago
Group: core-security

Comment 3

4 years ago
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
Last Resolved: 4 years ago
status-b2g-v2.2: --- → fixed
status-firefox37: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Does this impact ESR31? Also does it have a rating?
Flags: needinfo?(ehsan.akhgari)
(Assignee)

Comment 6

4 years ago
Not sure, Adam?
Flags: needinfo?(ehsan.akhgari) → needinfo?(adam)

Comment 7

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

Updated

4 years ago
status-firefox-esr31: --- → affected
Keywords: sec-low
status-firefox-esr31: affected → wontfix
Whiteboard: [adv-main37-]
Whiteboard: [adv-main37-] → [adv-main37-][b2g-adv-main2.2-]

Updated

3 years ago
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.