Closed Bug 1288246 Opened 4 years ago Closed 4 years ago

Missing DHE ciphers prevent FF 50 from connecting to Spark

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox50 --- affected

People

(Reporter: drno, Assigned: drno)

References

Details

(Keywords: cisco-spark)

Attachments

(3 files)

Cisco Spark calls between Firefox:
- 48 <-> 49 works
- 49 <-> 49 works
- 50 <-> 49 fails

As soon as one side of the call uses Firefox 50 the video rendering of the remote participant on both ends (!) stays grey. And after a couple of seconds Spark terminates the call, without the users hanging up.
Rank: 10
Looks like an ICE failure in Nightly. Looking into it...
Assignee: nobody → drno
ICE failure was a user error (to aggressive local firewall). With that properly adjusted the problem remains though.
According to mozregression this is the pushlog which causes the problem:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b69a5bbb5e40bd426e35222baa600b481e50d265&tochange=0edb9df3c97ff7d205a99f8af607f99a858f140b

Not sure I see in there the obvious commit causing this...
I see DTLS handshake problems in network traces. So I'm guessing that this:
  https://hg.mozilla.org/mozilla-central/rev/1a1d7ef3cb0e
from bug 1279479 is causing the problem.
Can you reproduce this problem on Firefox 49 with DHE cipher suites disabled? (Install <https://addons.mozilla.org/ja/firefox/addon/disable-dhe/> to disable DHE.)
Summary: Video stays grey with FF 50 → Missing DHE ciphers prevent FF 50 from connecting to Spark
Yes I can repro after installing the 'disbale DHE' addon. In wireshark trace I also see that with FF 49 Spark chooses TLS_DHE_RSA_WITH_AES_128_CBC_SHA as the cipher. Where in FF 50 that cipher is no longer offered and the handshake fails.

As WebRTC enforces PFS ciphers since several releases already we are then left with ECDHE only, which I assume will cause quite a bit of interop problems.

:mt, :ekr what are your thoughts on removing DHE from DTLS?
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
Chrome dropped the DHE support recently. Will Cisco Spark work with Chrome Canary? If so, do you know what Chrome is using? (QUIC?)
Cisco Spark does not support Chrome.
(In reply to Masatoshi Kimura [:emk] from comment #7)
> Chrome dropped the DHE support recently. Will Cisco Spark work with Chrome
> Canary? If so, do you know what Chrome is using? (QUIC?)

BTW in which version of Chrome is that?
Because I just manually verified with Wireshark that Chrome 52 still offer TLS_DHE_RSA_WITH_AES_128_CBC_SHA plus a whole set of other ciphers in their DTLS Client Hello message send by their WebRTC implementation.
And even Chrome 54 Canary still offers TLS_DHE_RSA_WITH_AES_128 and even TLS_RSA_WITH_AES_128_CBC_SHA as you can see in this screenshot.
(In reply to Nils Ohlmeier [:drno] from comment #10)
> And even Chrome 54 Canary still offers TLS_DHE_RSA_WITH_AES_128 and even
> TLS_RSA_WITH_AES_128_CBC_SHA as you can see in this screenshot.

Wow, we will have to offer DHE for DTLS until Cisco or Google change their mind :(
WebRTC has an override list to force-enable some cipher suites:
https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/media/mtransport/transportlayerdtls.cpp#660

We can add TLS_DHE_RSA_WITH_AES_128_CBC_SHA to the list.
(In reply to Masatoshi Kimura [:emk] from comment #12)
> WebRTC has an override list to force-enable some cipher suites:
> https://dxr.mozilla.org/mozilla-central/rev/
> 4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/media/mtransport/transportlayerdtls.
> cpp#660
> 
> We can add TLS_DHE_RSA_WITH_AES_128_CBC_SHA to the list.

Thanks for finding these. That helps a lot. I verified already that adding the ciphers back into the list fixes this issue with Spark.
Flags: needinfo?(martin.thomson)
Flags: needinfo?(ekr)
I think instead we should back out the original patch.
(In reply to Eric Rescorla (:ekr) from comment #15)
> I think instead we should back out the original patch.

Sure, I would prefer that. I'll just leave this here as backup, until the original patch is backed out.
Comment on attachment 8773595 [details]
Bug 1288246: added DHE_RSA ciphers back into DTLS handshake.

https://reviewboard.mozilla.org/r/66328/#review63120

I know that this addresses the problem, but as ekr said, we should look at backing out the change that marked DHE as bad.
Attachment #8773595 - Flags: review?(martin.thomson) → review-
Is bug 1227519 WONTFIX, then?
That seems like a different question that we can address in the future after we have fixed this bustage.
The PSM fallback logic should not affect outside PSM. This was not a problem when RC4 was the fallback cipher because DTLS prohibited RC4 from day one.

Review commit: https://reviewboard.mozilla.org/r/66480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/66480/
Attachment #8773786 - Flags: review?(dkeeler)
EMK, there was not agreement to make this change even for HTTPS. Please back out the entire patch and we can discuss afterwards.
Flags: needinfo?(rlb)
Attachment #8773786 - Flags: review?(dkeeler)
Patch has been backed out.

https://bugzilla.mozilla.org/show_bug.cgi?id=1279479#c21
Flags: needinfo?(rlb)
Verified that everything is back to working.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.