Closed Bug 817431 Opened 10 years ago Closed 10 years ago

Accept extensions for ICE


(Core :: WebRTC: Networking, defect, P1)




Tracking Status
firefox17 --- unaffected
firefox18 - disabled
firefox19 - disabled
firefox20 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- unaffected


(Reporter: ekr, Assigned: abr)



(Keywords: csectype-dos, Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] [nICEr][qa-][adv-main20-])


(1 file, 1 obsolete file)

Need to accept ICE extensions. Current code throws an error.

Here is a partial patch. There are two more assert(strlen(str) == 0)s that need to be turned
into either ignores or run-time errors. Please review the RFC 5245 grammar to see which.

lines: 385, 525
Please coordinate landing this patch with uplift of a patch to nICEr.
Assignee: nobody → adam
Why is this a security-sensitive issue?
Reed: the issue that that this bug allows a remote party to cause the browser to exit by sending an ICE packet of a certain form.
Whiteboard: [WebRTC],[blocking-webrtc-interop]
Generally, non-exploitable crash bugs aren't marked as security-sensitive, even though they could be used for a DoS on the user by crashing their browser - there are many ways to make someone's browser unusable even if it doesn't crash, though we try to maximize the ability of the user to get out of these situations (as they can just be errors by a website/author).

If a *third* party could crash your browser with a single packet, it might be more of an issue, though perhaps not a critical flaw in practice.
Duplicate of this bug: 814741
Priority: -- → P1
Whiteboard: [WebRTC],[blocking-webrtc-interop] → [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+]
Keywords: csec-dos
Jesup: just so I understand the policy.

If there are bugs that cause a crash from a Web site but not machine compromise (e.g., asserts)
then we don't file them as security sensitive? How about, e.g., packets that aren't from a Web site
but are part of a flow initiated by them, like in the RTP stack?
From discussion with abillings: "I'll move forward assuming non-exploitable null derefs do not need to be s-s (as we have been), and I'll extend that to include incoming SRTP streams." - and he agreed that's reasonable.
Attachment #687556 - Attachment is obsolete: true
Attachment #690595 - Flags: review?(rjesup)
Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] → [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] [nICEr]
Attachment #690595 - Flags: review?(rjesup) → review+
Comment on attachment 690595 [details] [diff] [review]
Make ICE parser tolerate extensions

[Security approval request comment]
> How easily can the security issue be deduced from the patch?

Deducing the nature of the problem would require an understanding of SDP in general, and of the ICE extensions to SDP in particular. These are both published IETF specifications.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No. On a casual examination of comments, this appears to be a patch to enhance compliance to the specification.

> Which older supported branches are affected by this flaw?

FF 18 and 19 contain this code. However, this code is only executed as part of WebRTC handling. WebRTC is preffed off by default in these builds, so exposure in 18 and 19 is very limited.

> If not all supported branches, which bug introduced the flaw?

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The source file in question does not appear to have been changed since its introduction. The patch should apply cleanly to all branches.

> How likely is this patch to cause regressions; how much testing does it need?

Since we are now generating errors for invalid SDP where we weren't before, any errors in the patch's logic may lead to rejection of valid SDP. On the other hand, the changes are small and easy to review. Determining their correctness by casual examination is a rather straightforward exercise.
Attachment #690595 - Flags: sec-approval?
Attachment #690595 - Flags: sec-approval? → sec-approval+
sec-approval+ to go into trunk.

I'm cc'ing Release Management so they can determine whether they need or want this for 18 and 19.
Thanks Al. Given where we are in the cycle, we'll avoid unnecessary change for pref'd off features.
Attachment #690595 - Flags: checkin?(rjesup)
Attachment #690595 - Flags: checkin?(rjesup) → checkin+
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] [nICEr] → [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] [nICEr][qa-]
Whiteboard: [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] [nICEr][qa-] → [WebRTC],[blocking-webrtc-interop],[blocking-webrtc+] [nICEr][qa-][adv-main20-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.