Closed Bug 1089207 Opened 5 years ago Closed 5 years ago

sipcc SDP parser can corrupt memory

Categories

(Core :: WebRTC: Signaling, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox33 --- wontfix
firefox34 --- verified
firefox35 --- verified
firefox36 --- verified
firefox-esr31 34+ verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: drno, Assigned: drno)

Details

(Keywords: csectype-bounds, sec-high, Whiteboard: [adv-main34+][adv-esr31.3+])

Attachments

(1 file, 3 obsolete files)

Attached patch sdp_attr_security_hole.patch (obsolete) — 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 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?
Adding Cullen Jennings.
(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.
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.
(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.
(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.
Because this code is in SIPCC, please do not un-flag prior to approval from Ethan or someone else from Cisco.
(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 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.
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.
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).
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 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 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.
Marking sec-high per comment 8.
Attached patch fix_sdp_attr_parsing.patch (obsolete) — Splinter Review
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+
Ethan please let us know when can land this from Cisco's perspective.
Flags: needinfo?(ethanhugg)
We can land this now.  The team here has already been notified.  Thanks.
Flags: needinfo?(ethanhugg)
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+
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?
Dan - Does this bug need sec approval before landing?
Flags: needinfo?(dveditz)
(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 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 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+
Please request esr31 approval on this when you get a chance too.
Comment on attachment 8517019 [details] [diff] [review]
fix_sdp_attr_parsing.patch

See sec approval request
Attachment #8517019 - Flags: approval-mozilla-esr31?
Lawrence: ok for landing this on Beta (and all other branches) per dveditz's comment 24?
Flags: needinfo?(lmandel)
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+
Lawrence, can we land the patch together with the test or do you want it broken up in two separate patches?
Flags: needinfo?(lmandel)
As this is landing already my question is obsolete.
Flags: needinfo?(lmandel)
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/e06adcf04636
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Whiteboard: [adv-main34+][adv-esr31.3+]
Nils, could you confirm that this has been fixed?
Flags: needinfo?(drno)
I verified that the new unit test for this fix is working as expected on the versions I marked as verified.
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.