Closed
Bug 1089207
Opened 10 years ago
Closed 10 years ago
sipcc SDP parser can corrupt memory
Categories
(Core :: WebRTC: Signaling, defect, P1)
Core
WebRTC: Signaling
Tracking
()
People
(Reporter: drno, Assigned: drno)
Details
(Keywords: csectype-bounds, sec-high, Whiteboard: [adv-main34+][adv-esr31.3+])
Attachments
(1 file, 3 obsolete files)
|
6.14 KB,
patch
|
drno
:
review+
dveditz
:
approval-mozilla-aurora+
dveditz
:
approval-mozilla-beta+
lmandel
:
approval-mozilla-esr31+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
While fuzzing SDParta I found this little gem in our SDP parsing code:
http://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c#508
I think if you hand Firefox some SDP with a missing newline the above loop will walk through all your memory and replace '/' with ';' where ever it finds '/'s in you memory until it finally comes across a newline some where in you memory.
Attachment #8511551 -
Flags: review?(ekr)
Comment 1•10 years ago
|
||
Comment on attachment 8511551 [details] [diff] [review]
sdp_attr_security_hole.patch
Review of attachment 8511551 [details] [diff] [review]:
-----------------------------------------------------------------
Are you certain that the hack isn't still necessary?
::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ -505,5 @@
> return (SDP_FAILURE);
> }
> fmtp_ptr = src_ptr = temp_ptr;
> - while (flag == FALSE) {
> - if (*src_ptr == '\n') {
why not just add !*src_ptr here?
Comment 2•10 years ago
|
||
Adding Cullen Jennings.
| Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Martin Thomson [:mt] from comment #1)
> Are you certain that the hack isn't still necessary?
Nothing is for sure. I don't see us connecting to such devices soon. And if we encounter such broken SDP I would rather make the parser accept the illegal chars, then modifying what you parse.
> ::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
> @@ -505,5 @@
> > return (SDP_FAILURE);
> > }
> > fmtp_ptr = src_ptr = temp_ptr;
> > - while (flag == FALSE) {
> > - if (*src_ptr == '\n') {
>
> why not just add !*src_ptr here?
Sure, checking for \0 is the other option here. But as the comment below the hack indicates that this is a hack anyway I would prefer to actually remove it rather then attempting to fix the hack.
Comment 4•10 years ago
|
||
Note: I believe not an actual sec issue in practice, because of sdp_main.c:1007:
ptr = next_ptr;
line_end = sdp_findchar(ptr, "\n");
if (line_end >= (*bufp + len)) {
<error out>
before calling the attribute parse from line 1117. So while not noted anywhere(!), the buffer is guaranteed to have a \n before the end. If so, and I think it is so, we don't need to uplift this.
Note I'm not sure spec-wise the comment is correct that parameters in fmtp MUST be separated by ; is true - but it's true for all the codecs we care about.
I would be fine with removing it or (perhaps better since it does document a known interop issue) #ifdefing it.
Comment 5•10 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #4)
> the buffer is guaranteed to have a \n before the end.
Yeah, un-sec-flag this bug. I'd have said that a \0 check is probably the most conservative change that doesn't leave a booby-trap.
| Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Randell Jesup [:jesup] from comment #4)
> Note: I believe not an actual sec issue in practice, because of
> sdp_main.c:1007:
> ptr = next_ptr;
> line_end = sdp_findchar(ptr, "\n");
> if (line_end >= (*bufp + len)) {
> <error out>
> before calling the attribute parse from line 1117. So while not noted
> anywhere(!), the buffer is guaranteed to have a \n before the end. If so,
> and I think it is so, we don't need to uplift this.
So here is how my little fuzzer received a SIGSTOP while trying to parse:
m=video...\r\n
a=fmtp:foo\0bar
In this case sdp_findchar returns the position of the null char, which is before the end of the buffer. But there is no further \n before the end of the buffer which would stop the loop in sdp_parse_attr_fmtp().
In other words the buffer is guaranteed to either have \n OR \0 in it before (or at) its end. But the loop in sdp_parse_attr_fmtp() assumes it is guaranteed to have a \n.
As I'm not a security or JS expert I can't judge how hard it is to get a string into the buffer from JS land into the C code of the SDP parser.
With that being said I think it is safest to extend the exit criteria of the while loop in sdp_parse_attr_fmtp to look for \0 as well. I'll provide a patch for that later if people agree with that.
Comment 7•10 years ago
|
||
Because this code is in SIPCC, please do not un-flag prior to approval from Ethan or someone else from Cisco.
Comment 8•10 years ago
|
||
(In reply to Nils Ohlmeier [:drno] from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #4)
> > Note: I believe not an actual sec issue in practice, because of
> > sdp_main.c:1007:
> > ptr = next_ptr;
> > line_end = sdp_findchar(ptr, "\n");
> > if (line_end >= (*bufp + len)) {
> > <error out>
> > before calling the attribute parse from line 1117. So while not noted
> > anywhere(!), the buffer is guaranteed to have a \n before the end. If so,
> > and I think it is so, we don't need to uplift this.
>
> So here is how my little fuzzer received a SIGSTOP while trying to parse:
> m=video...\r\n
> a=fmtp:foo\0bar
>
> In this case sdp_findchar returns the position of the null char, which is
> before the end of the buffer. But there is no further \n before the end of
> the buffer which would stop the loop in sdp_parse_attr_fmtp().
> In other words the buffer is guaranteed to either have \n OR \0 in it before
> (or at) its end. But the loop in sdp_parse_attr_fmtp() assumes it is
> guaranteed to have a \n.
If there's no \n, sdp_findchar returns a pointer to the \0. If so,
Ugh; so my belief is wrong. (I didn't have time to ferret it out all the way to the bottom.) At least I was smart enough to include wiggle words.
Getting bad SDP data in is likely trivial; exploiting a walk-through-memory-until-\n-and-replace-/-with-; is likely super-hard, but never say never with this sort of thing.
> With that being said I think it is safest to extend the exit criteria of the
> while loop in sdp_parse_attr_fmtp to look for \0 as well. I'll provide a
> patch for that later if people agree with that.
That's likely the simplest, safest patch. Almost as safe and perhaps even safer is removing the kludge entirely. One could also avoid any other instances like this elsewhere in the sdp code by having it check after sdp_findchar() if (*bufp)[len] == '\n', and if that fails fall into the missing-\n case there. I'd be tempted to take that *and* one of the proposed items above - check for \0 in the hack, or remove the hack.
Comment 9•10 years ago
|
||
Comment on attachment 8511551 [details] [diff] [review]
sdp_attr_security_hole.patch
Review of attachment 8511551 [details] [diff] [review]:
-----------------------------------------------------------------
I don't see a reason to keep this hack for '/'.
::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ -504,5 @@
> if (temp_ptr == NULL) {
> return (SDP_FAILURE);
> }
> fmtp_ptr = src_ptr = temp_ptr;
> - while (flag == FALSE) {
I believe this and the flag = TRUE below are the only uses of 'flag' so you'll want to remove its decl above if you decide to remove this hack.
Comment 10•10 years ago
|
||
Ok, unless there's objection: let's remove the hack *and* add the safety check before parsing a line, just in case there's another instance somewhere.
| Assignee | ||
Comment 11•10 years ago
|
||
I added a test case to sdp_unittest.cpp to demonstrate and verify the fixes.
The test shows that adding the check of line_end == '\0' into the while loop in sdp_parse() fixes the problem as expected. But in that case sdp_parse() still returns SDP_SUCCESS, as the if condition does not update the result value.
My guess is that this aligns with the existing behavior of accepting a last line without trailing \n as a valid SDP. I'm sure there are SDP implementations out there which will send you such broken SDP.
Now with the additional safety check we simply ignore the last line, but still accept the rest as a valid SDP. Is that what we want here, or should we return an error in case the last line of the SDP was bogus?
I lean towards keep returning SDP_SUCCESS, as changing this would be yet another change. And the that change would not be directly justified by this bug report here. But I thought I bring this up here, so everyone is aware of this behavior (I can add a comment in the code about this).
| Assignee | ||
Comment 12•10 years ago
|
||
Removed the hack loop, added \0 check in sdp_parse() and added a unit test.
Attachment #8511551 -
Attachment is obsolete: true
Attachment #8511551 -
Flags: review?(ekr)
Attachment #8512280 -
Flags: review?(rjesup)
Attachment #8512280 -
Flags: review?(ethanhugg)
Attachment #8512280 -
Flags: review?(ekr)
Comment 13•10 years ago
|
||
Comment on attachment 8512280 [details] [diff] [review]
bug_1089207_fix_sdp_attr_parsing.patch
Review of attachment 8512280 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me. Thanks.
Attachment #8512280 -
Flags: review?(ethanhugg) → review+
Comment 14•10 years ago
|
||
Comment on attachment 8512280 [details] [diff] [review]
bug_1089207_fix_sdp_attr_parsing.patch
Review of attachment 8512280 [details] [diff] [review]:
-----------------------------------------------------------------
::: media/webrtc/signaling/test/sdp_unittests.cpp
@@ +796,5 @@
> + "t=0 0\r\n"
> + "m=video 56436 RTP/SAVPF 120\r\n"
> + "c=IN IP4 198.51.100.7\r\n"
> + "a=rtpmap:120 VP8/90000\r\n"
> + "a=fmtp:120 max-fs=300;max\0fr=30";
Please highlight the fact that there is a trap on this line.
| Assignee | ||
Comment 16•10 years ago
|
||
Addressed Martin's comment.
Carrying forward r+=ehugg
Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=50306783202d
Attachment #8512280 -
Attachment is obsolete: true
Attachment #8512280 -
Flags: review?(rjesup)
Attachment #8512280 -
Flags: review?(ekr)
Attachment #8515248 -
Flags: review+
| Assignee | ||
Comment 17•10 years ago
|
||
Ethan please let us know when can land this from Cisco's perspective.
Flags: needinfo?(ethanhugg)
Comment 18•10 years ago
|
||
We can land this now. The team here has already been notified. Thanks.
Flags: needinfo?(ethanhugg)
| Assignee | ||
Comment 19•10 years ago
|
||
Updated the commit message to include this bug number (after running it on try without a reference).
Carrying forward r+=ehugg
Attachment #8515248 -
Attachment is obsolete: true
Attachment #8517019 -
Flags: review+
| Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8517019 [details] [diff] [review]
fix_sdp_attr_parsing.patch
Approval Request Comment
[Feature/regressing bug #]: This bug probably has been around since we added Sipcc to enable WebRTC in Firefox in bug 792188.
[User impact if declined]: Firefox could crash if someone manages to get a properly crafted string into the SDP parser.
[Describe test coverage new/current, TBPL]: A new unit test case which verifies the fix is part of the patch.
[Risks and why]: There is a small risk that after applying this patch Firefox no longer is able to parse SDP's from certain devices, but it is very unlikely that Firefox will try to establish calls in the filed with such broken devices.
[String/UUID change made/needed]: N/A
Attachment #8517019 -
Flags: approval-mozilla-release?
Attachment #8517019 -
Flags: approval-mozilla-beta?
Attachment #8517019 -
Flags: approval-mozilla-aurora?
Comment 21•10 years ago
|
||
Dan - Does this bug need sec approval before landing?
Flags: needinfo?(dveditz)
Comment 22•10 years ago
|
||
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #21)
> Dan - Does this bug need sec approval before landing?
I'm not Dan, but normally sec-high and sec-crit need approval if the bug has reached Aurora; this bug is in release, so it does need sec approval IMO
Comment 23•10 years ago
|
||
Comment on attachment 8517019 [details] [diff] [review]
fix_sdp_attr_parsing.patch
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily since it's a character replace through memory following the string, but perhaps not quite impossible.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The test paints it with a bullseye. The rest of the code looks like removing an old unneeded hack. This could land on a bug of "remove non-compliant fmtp parsing" without the test, and land the test later and/or under the sec bug ID.
Which older supported branches are affected by this flaw? All
If not all supported branches, which bug introduced the flaw? Original SIPCC code imported.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should apply with no trouble.
How likely is this patch to cause regressions; how much testing does it need? Almost no chance of regressions.
Attachment #8517019 -
Flags: sec-approval?
Comment 24•10 years ago
|
||
Comment on attachment 8517019 [details] [diff] [review]
fix_sdp_attr_parsing.patch
sec-approval+ and a=dveditz for aurora/beta
If lmandel objects to landing on beta then please land without the test, set the in-testsuite flag to '?' and try to remember to land the test later. We're not chemspilling for this so no need to land on mozilla-release.
Flags: needinfo?(dveditz)
Attachment #8517019 -
Flags: sec-approval?
Attachment #8517019 -
Flags: sec-approval+
Attachment #8517019 -
Flags: approval-mozilla-release?
Attachment #8517019 -
Flags: approval-mozilla-beta?
Attachment #8517019 -
Flags: approval-mozilla-beta+
Attachment #8517019 -
Flags: approval-mozilla-aurora?
Attachment #8517019 -
Flags: approval-mozilla-aurora+
Comment 25•10 years ago
|
||
Please request esr31 approval on this when you get a chance too.
Comment 26•10 years ago
|
||
Comment on attachment 8517019 [details] [diff] [review]
fix_sdp_attr_parsing.patch
See sec approval request
Attachment #8517019 -
Flags: approval-mozilla-esr31?
Comment 27•10 years ago
|
||
Lawrence: ok for landing this on Beta (and all other branches) per dveditz's comment 24?
Flags: needinfo?(lmandel)
Comment 28•10 years ago
|
||
Comment on attachment 8517019 [details] [diff] [review]
fix_sdp_attr_parsing.patch
(In reply to Randell Jesup [:jesup] from comment #27)
> Lawrence: ok for landing this on Beta (and all other branches) per dveditz's
> comment 24?
Yes. Let's land this today in time for beta7. As per Ryan's request, you have approval to land the fix on ESR31 as well.
Flags: needinfo?(lmandel)
Attachment #8517019 -
Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
| Assignee | ||
Comment 29•10 years ago
|
||
Lawrence, can we land the patch together with the test or do you want it broken up in two separate patches?
Flags: needinfo?(lmandel)
| Assignee | ||
Comment 30•10 years ago
|
||
As this is landing already my question is obsolete.
Flags: needinfo?(lmandel)
Comment 31•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ccde6ac141db
https://hg.mozilla.org/releases/mozilla-beta/rev/f30e1c0c0694
inbound/central landing is waiting on tree opening
esr31 waiting on a local checkbuild
Updated•10 years ago
|
Flags: in-testsuite+
Comment 32•10 years ago
|
||
Comment 33•10 years ago
|
||
Comment 34•10 years ago
|
||
Comment 35•10 years ago
|
||
Comment 36•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Updated•10 years ago
|
Whiteboard: [adv-main34+][adv-esr31.3+]
Updated•10 years ago
|
tracking-firefox-esr31:
--- → 34+
| Assignee | ||
Comment 38•10 years ago
|
||
I verified that the new unit test for this fix is working as expected on the versions I marked as verified.
Flags: needinfo?(drno)
| Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•