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 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2017
Cc:
Components:
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue chromium:690069



Sign in to add a comment
link

Issue 6972: Having a=crypto in an SDP offer causes exception in Canary

Reported by ushunmu...@gmail.com, Jan 11 2017

Issue description

What steps will reproduce the problem?
1. Start a client that creates a PeerConnection and accepts calls.
2. Send an offer SDP to the client with a=crypto attribute for SDES-SRTP along with a=fingerprint for DTLS-SRTP.
3. The client does a setRemoteDescription with the offer SDP.

What is the expected result?
The setRemoteDescription succeeds.

What do you see instead?
The setRemoteDescription fails with the following message:
OperationError: Failed to set remote offer sdp: Session error code: ERROR_CONTENT. Session error description: Cryptos must be empty when DTLS is active.

What version of the product are you using? On what operating system?
Version 57.0.2976.5 canary (64-bit), on Windows 7

Please provide any additional information below.

Since the call is not established yet, both crypto and fingerprint attributes should be accepted for backward compatibility. This works fine on stable Chrome 55.

Here is a sample SDP that causes this issue.
v=0
o=xxxxxx 7 2 IN IP4 0.0.0.0
s=-
c=IN IP4 0.0.0.0
t=0 0
a=group:BUNDLE audio
a=msid-semantic: WMS D3C941D1-DAD9-43D3-BDB7-1FD861915F3D
m=audio 1 RTP/SAVPF 9 0 101
a=sendrecv
a=rtcp-mux
a=mid:audio
a=ssrc:594296634 cname:bFIHH+1iv0K+p4mZxKULHXxb
a=ssrc:594296634 label:a0-41D1-DAD9-43D
a=ssrc:594296634 msid:D3C941D1-DAD9-43D3-BDB7-1FD861915F3D a0-41D1-DAD9-43D
a=ssrc:594296634 mslabel:D3C941D1-DAD9-43D3-BDB7-1FD861915F3D
a=ice-ufrag:u0gW
a=ice-pwd:jl7nSNU1qGLfiej6OwSGnp
a=rtpmap:9 g722/8000
a=rtpmap:0 pcmu/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=fingerprint:sha-256 EB:95:EC:03:8B:21:39:58:81:10:8D:9D:0A:B1:3C:6D:DC:08:9D:59:59:3A:51:8A:BC:88:80:C8:4F:FE:6D:4F
a=setup:actpass
a=crypto:1 AES_CM_128_HMAC_SHA1_80 inline:9cXKvyA9V2T+IZxDZtO+T1oPOFYsS4O9Wzuf7W5h
 

Comment 1 by ushunmu...@gmail.com, Jan 11 2017

Is this a behavior change in Canary, or a bug?

Comment 2 by anatolid@webrtc.org, Jan 12 2017

Project Member
Components: PeerConnection

Comment 3 by deadbeef@chromium.org, Jan 17 2017

Project Member
Labels: EngTriaged
Owner: deadbeef@webrtc.org
Status: Started (was: Unconfirmed)
It's a regression; it appears there was no unit/integration test for DTLS/SDES fallback. I'll fix it and request merge to M57.

Comment 4 by ushunmu...@gmail.com, Jan 18 2017

Thanks for letting us know.

BTW, there are some media issue with the Canary (using DTLS). With the previous version that I had reported above, the callee wasn't playing the media.  With the latest version 57.0.2985.0, that seems fixed, but there are media issues with a video call after a renegotiation.  I'd assume these would get sorted out during your own testing/development.

Comment 5 by deadbeef@webrtc.org, Jan 18 2017

Project Member
What kind of media issues? Also, could you file a new bug for them (or see if one already exists)?

Comment 6 by ushunmu...@gmail.com, Jan 20 2017

Have created  Issue 7027  for the media issue.  

BTW, the original media issue is still there - no need to renegotiate.  When a video call is received, the callee in Canary does not play the media, though both audio and video packets are being received.  Audio calls work fine.

Comment 7 by bugdroid1@chromium.org, Jan 21 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/8662f940230bb0f8989ddc993488cde3a9c5b76a

commit 8662f940230bb0f8989ddc993488cde3a9c5b76a
Author: deadbeef <deadbeef@webrtc.org>
Date: Sat Jan 21 05:20:51 2017

Only set certificate on DTLS transport if fingerprint is found in SDP.

This is used for fallback from DTLS to SDES encryption, which we probably still
want to support. Setting a certificate puts the DTLS transport in a "DTLS
enabled" mode, so it should be delayed until SDP with "a=fingerprint" is set.

BUG= webrtc:6972 

Review-Url: https://codereview.webrtc.org/2641633002
Cr-Commit-Position: refs/heads/master@{#16199}

[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/api/peerconnectioninterface_unittest.cc
[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/api/test/fakertccertificategenerator.h
[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/base/sslfingerprint.cc
[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/base/sslfingerprint.h
[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/p2p/base/faketransportcontroller.h
[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/p2p/base/jseptransport.cc
[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/p2p/base/transportcontroller.cc
[modify] https://crrev.com/8662f940230bb0f8989ddc993488cde3a9c5b76a/webrtc/p2p/base/transportdescriptionfactory.cc

Comment 8 by deadbeef@webrtc.org, Jan 21 2017

Project Member
Labels: Merge-Requested M-57
Status: Fixed (was: Started)
Requesting merge to M57, since this is a regression.

Comment 9 by bugdroid1@chromium.org, Jan 21 2017

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/external/webrtc.git/+/3e4faae0edeb2b30e899712f92217d2aa5cd6ff2

commit 3e4faae0edeb2b30e899712f92217d2aa5cd6ff2
Author: deadbeef <deadbeef@webrtc.org>
Date: Sat Jan 21 06:43:34 2017

Fixing memory leak in FakeTransportController.

Introduced by: https://codereview.webrtc.org/2641633002/
Only occurs with test code.

BUG= webrtc:6972 
TBR=pthatcher@webrtc.org

Review-Url: https://codereview.webrtc.org/2648093002
Cr-Commit-Position: refs/heads/master@{#16200}

[modify] https://crrev.com/3e4faae0edeb2b30e899712f92217d2aa5cd6ff2/webrtc/p2p/base/faketransportcontroller.h

Comment 10 by deadbeef@chromium.org, Feb 8 2017

Project Member
Blockedon: chromium:690069

Comment 11 by anatolid@chromium.org, Feb 23 2017

Project Member
Labels: -Merge-Requested -M-57 M-58

Comment 12 by deadbeef@webrtc.org, Feb 23 2017

Project Member
Labels: -M-58 M-57

Comment 13 by anatolid@webrtc.org, Mar 6 2017

Project Member
Labels: -M-57 merge-merged-m57 M-58

Comment 14 by ushunmu...@gmail.com, Mar 29 2017

The  issue 7027  that I created for the media issue (related to video codec name being case sensitive) is still untriaged, but the problem is still there in stable version 57!  Could someone take a look at that?

Comment 15 by deadbeef@chromium.org, Sep 11 2017

Project Member
Do you still need this functionality (applying an offer that allows both DTLS-SRTP and SDES-SRTP)? We're planning to remove it, since it's non-standard and has a maintenance cost. Also, we'll eventually need to remove support for SDES completely (at least, from Chrome), so this seems like a logical first step towards that goal.

If you still need this, can you explain your use case more so I can help figure out an alternative?

Comment 16 by deadbeef@webrtc.org, Sep 22 2017

Project Member
Cc: ushunmu...@gmail.com
FYI, we've now done this. See: https://groups.google.com/d/msg/discuss-webrtc/_aePBY7Ga4E/a6qljdbXBQAJ

Sign in to add a comment