New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 5 users

Issue metadata

Status: Verified
Owner:
Email to this user bounced
Closed: Jun 2014
Cc:
Components:
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

PeerConnection accepts SDP offer/answer with ICE credentials longer than 256 characters

Reported by kape...@gmail.com, Aug 4 2013

Issue description

What steps will reproduce the problem?
1. Create a PeerConnection
2. Let it create a SDP offer.
3. Feed it a SDP answer with ICE u-frag/pwd longer than 256 characters.
 

What is the expected result?
setRemoteDescription() should fail, because ICE credentials have a length limit of 256 characters (see http://tools.ietf.org/html/rfc5245#section-15.4)

What do you see instead?
setRemoteDescription() succeeds and turns Chrome into a STUN gun, see http://www.kapejod.org/2013/08/security-implications-of-webrtc-part-1-the-stun-gun/


What version of the product are you using? On what operating system?
Chrome 28.0.1500.95 (Official Build 213514) on Linux
I am pretty sure this applies to other versions, too.

Please provide any additional information below.


 
the attached patch should accomplish that. Not sure whether it would be more appropriate during operations like PushdownTransportDescription, it seems like something that should be enforced at a lower layer.
ufragpwd256.patch
1.8 KB View Download

Comment 2 Deleted

slightly updated patch
ufragpwd256.patch
2.2 KB View Download

Comment 4 by kape...@gmail.com, Aug 5 2013

While you are at it, you might also enforce the minimum length for ufrag (4) and pwd (22).
mh... while I am at it ;-)

One might also enforce the alpha|digit|"+"|"/" ice-char rule, but that isn't checked in other places either and I did not find any string util to do so.
ufragpwd_minmax.patch
2.6 KB View Download

Comment 6 by kape...@gmail.com, Aug 5 2013

Splendid. :-) Now the question remains when this will make it into a Chrome version?
Owner: mallinath@webrtc.org
Status: Available
We will try to fix it in M30, otherwise for sure in M31. Thanks Philipp for the patch, I will take a look at it.
Labels: Mstone-30
mallinath: thanks. The unittests need also need fixes (because they're actually wrong), wait for a new patch.
Enforcing a length of 4+ breaks lots libjingle_peerconnection unittests, some of them in a nontrivial way (things that serialize and reparse).

I'd go for the second ufragpdw256.patch first and solve the 4+ problem later (along with ice-char possibly).  Issue #1895  should prevent empty ufrag/pwd in the meantime (review appreciated, too :-)
I think these changes should go in at a lower layer, i.e. SetRemoteIceCredentials down in P2PTC. Then they will still work with non-SDP use cases.
Labels: Area-PeerConnection
Project Member

Comment 13 by juberti@webrtc.org, Aug 14 2013

Labels: Area-Transport
Project Member

Comment 14 by juberti@webrtc.org, Aug 14 2013

Labels: -Mstone-30 Mstone-31
Project Member

Comment 15 by juberti@webrtc.org, Aug 14 2013

Labels: -Pri-2 Pri-1
Justin: for webrtc-usage this is somewhat difficult, since SetRemoteIceCredentials is called in the worker thread after PushdownTransportDescription has returned. See the attached patch (not fully ready yet, can submit later if desired)

Catching this in Transport::SetLocalTransportDescription or SetRemoteTransportDescription seems more appropriate to me.
icemaxlength_lowlevel.patch
11.6 KB View Download
wrt comment 10, enforcing a length of 4+ breaks several unittests because BuildMediaDescription in webrtcsdp.cc doesn't check for empty ufrag/pwd and therefore adds "a=ice-ufrag:\r\n" lines.
https://webrtc-codereview.appspot.com/2059004 should take care of comment 17.

Comment 19 by kape...@gmail.com, Jan 8 2014

Any idea on when/if this will be fixed? This is a serious DoS opportunity.
Project Member

Comment 20 by vikasmarwaha@webrtc.org, May 7 2014

Labels: -Mstone-31 Mstone-37
Moving to M37 for now.
There is already a rate limiter in Chrome that prevents this being used for DoS. If you think there is still a problem here, please supply a example page.

Comment 22 by kape...@gmail.com, May 29 2014

Chrome is rate limiting this to 2 Mbit/s which is 16x more than you would be able to create with RFC compliant credentials. This looks pretty DoSy to me.

If you want to give it a try see my example at: https://gist.github.com/kapejod/63520e94bdd6755465ee
Fix is available in WebRTC trunk, this issue will be fixed in next libjingle roll into Chrome.

Comment 24 by kape...@gmail.com, May 29 2014

Nice. Can you point me to the change? I can't seem to find it.

Comment 25 by kape...@gmail.com, May 29 2014

Never mind. I thought that you fixed it just now. I tested again with Chrome 37 dev and it's fine. Thanks.
Project Member

Comment 26 by pthatcher@google.com, Jun 2 2014

Status: Fixed
Project Member

Comment 27 by tnakamura@webrtc.org, Jul 10 2014

Cc: jansson@webrtc.org tnakamura@webrtc.org
Status: Verified
lgtm in 37.0.2062.15
Project Member

Comment 28 by kjellander@webrtc.org, Oct 5 2016

Components: Network
Project Member

Comment 29 by kjellander@webrtc.org, Oct 5 2016

Components: -Transport

Sign in to add a comment