817431 - Accept extensions for ICE

Status: RESOLVED FIXED

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

Description

[Eric Rescorla (:ekr)]

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

Comment 1

[Eric Rescorla (:ekr)]

Please coordinate landing this patch with uplift of a patch to nICEr.

Comment 2

[Eric Rescorla (:ekr)]

Attachment: Make ICE parser tolerate extensions

Comment 3

[Reed Loden [:reed]]

Why is this a security-sensitive issue?

Comment 4

[Adam Roach [:abr]]

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.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 7

[Eric Rescorla (:ekr)]

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?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 9

[Adam Roach [:abr]]

Attachment: Make ICE parser tolerate extensions

Comment 10

[Adam Roach [:abr]]

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?

https://bugzilla.mozilla.org/show_bug.cgi?id=790517

> 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.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 12

[Alex Keybl [:akeybl]]

Thanks Al. Given where we are in the cycle, we'll avoid unnecessary change for pref'd off features.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7963fe83e7a2

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7963fe83e7a2

Comment 15

[Adam Roach [:abr]]

Landed on resiprocate trunk r9911: http://svn.resiprocate.org/viewsvn/resiprocate?diff_format=l&view=revision&revision=9911