Closed
Bug 1082142
Opened 10 years ago
Closed 10 years ago
Potentially unterminated string buffers in |CC_SIPCCCall|
Categories
(Core :: WebRTC: Signaling, defect)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla36
Tracking | Status | |
---|---|---|
firefox34 | --- | unaffected |
firefox35 | --- | fixed |
firefox36 | + | fixed |
firefox-esr31 | --- | unaffected |
b2g-v1.4 | --- | unaffected |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.2 | --- | fixed |
People
(Reporter: erahm, Assigned: bwc)
References
(Blocks 1 open bug)
Details
(Keywords: coverity, sec-moderate, Whiteboard: [CID 1244245][CID 1244246][CID 1244247])
Attachments
(1 file)
3.19 KB,
patch
|
ehugg
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
Several spots in |CC_SIPCCCall| perform an |strncpy| [1,2,3,4,5] of the target buffer size which by design will omit the null terminator if the source buffer size >= target buffer size. This can lead to OOB reads/writes later on. [1] http://hg.mozilla.org/mozilla-central/annotate/71edd80236b2/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp#l753 [2] http://hg.mozilla.org/mozilla-central/annotate/71edd80236b2/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp#l839 [3] http://hg.mozilla.org/mozilla-central/annotate/71edd80236b2/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp#l842 [4] http://hg.mozilla.org/mozilla-central/annotate/71edd80236b2/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp#l876 [5] http://hg.mozilla.org/mozilla-central/annotate/71edd80236b2/media/webrtc/signaling/src/softphonewrapper/CC_SIPCCCall.cpp#l879
Assignee | ||
Comment 1•10 years ago
|
||
The candidate and mid cases in addICECandidate are where we need to be worried here, I think.
Comment 2•10 years ago
|
||
In Bug 776682 we replaced all copies of strncpy with a safe equivalent in the signaling code. I imagine we need to find all of these new instances and replace them again.
Assignee | ||
Comment 3•10 years ago
|
||
Should do the trick.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → docfaraday
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•10 years ago
|
||
Comment on attachment 8504339 [details] [diff] [review] Use sstrncpy instead of strncpy in CC_SIPCCCall Review of attachment 8504339 [details] [diff] [review]: ----------------------------------------------------------------- https://tbpl.mozilla.org/?tree=Try&rev=404a51b0bc31
Attachment #8504339 -
Flags: review?(ethanhugg)
Comment 5•10 years ago
|
||
Comment on attachment 8504339 [details] [diff] [review] Use sstrncpy instead of strncpy in CC_SIPCCCall Review of attachment 8504339 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Grep tells me that these are the only instances of this in the signaling directory.
Attachment #8504339 -
Flags: review?(ethanhugg) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8504339 [details] [diff] [review] Use sstrncpy instead of strncpy in CC_SIPCCCall [Security approval request comment] How easily could an exploit be constructed based on the patch? I'm pretty sure it would be easy to write a non-null-terminated string into the candidate field from content (via CC_SIPCCCall::addICECandidate), as well as into the mid field. However, there are fields following these that will contain null. cc_feature_t.data, note how stuff like data_valid, action, and sdp follow, all of which will be null in this case: http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/includes/ccapi.h#928 cc_feature_candidate_t: http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/includes/ccapi.h#807 I am reasonably sure that this is not a security threat currently. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Well, changing strncpy to sstrncpy makes things pretty obvious, regardless of any comments. That said, I don't think this is exploitable in any meaningful way right now. Which older supported branches are affected by this flaw? None, just 35. If not all supported branches, which bug introduced the flaw? Bug 1060625 How likely is this patch to cause regressions; how much testing does it need? Extremely unlikely. CI testing should be fine.
Attachment #8504339 -
Flags: sec-approval?
Comment 7•10 years ago
|
||
Comment on attachment 8504339 [details] [diff] [review] Use sstrncpy instead of strncpy in CC_SIPCCCall [Triage Comment] sec-approval+ for trunk. Now that 35 is Aurora, we'll need to get it on Aurora nomination but I assume the patch is still good since it has only been a few days.
Attachment #8504339 -
Flags: sec-approval?
Attachment #8504339 -
Flags: sec-approval+
Attachment #8504339 -
Flags: approval-mozilla-aurora+
Comment 8•10 years ago
|
||
Do you have a suggested security rating for this?
Updated•10 years ago
|
status-firefox34:
--- → unaffected
status-firefox35:
--- → affected
status-firefox36:
--- → affected
status-firefox-esr31:
--- → unaffected
tracking-firefox36:
--- → +
Assignee | ||
Comment 9•10 years ago
|
||
I'm pretty sure there's nothing exploitable here, but there's always that shred of doubt. Not sure what the rubric here is.
Comment 10•10 years ago
|
||
I'd be tempted to say sec-med given the fields following; no more than sec-high
Updated•10 years ago
|
Keywords: sec-moderate
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aa6ce5e6c76
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0aa6ce5e6c76
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-b2g-v1.4:
--- → unaffected
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.2:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•9 years ago
|
Group: core-security
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•