Closed Bug 1147919 Opened 6 years ago Closed 6 years ago

from discuss-webrtc post: DTLS Handshake issue with Firefox-37+ in Asterisk WebRTC Gateway

Categories

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

x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: mreavy, Assigned: bwc)

References

Details

(Whiteboard: [investigation])

Attachments

(2 files, 1 obsolete file)

From the original discuss-webrtc post from AjayG:
https://groups.google.com/forum/#!topic/discuss-webrtc/seAJiN8Z2-Y

"DTLS Handshake is not completing in Firefox-37/38(current Beta/Nightly channel), with Asterisk WebRTC Gateway. But the same gateway is working properly for Firefox (<=36) and all chrome versions (including canary).
With next Firefox release all production WebRTC Gateways (built on Asterisk-11.x/12.x/13.x) will stop working."


AjayG (the reporter) came to #media and provided more details: 
* in the case of an incoming call to browser (browser is in accept state), it is not sending the client a hello
* the a=fingerprint is in the m= section, not session-level
* he tried moving fingerprint line above m= line as in this link https://www.dropbox.com/s/ps2x4x65k63lp93/WebRTC_OFFER.txt?dl=0 , but there was no change in behavior.  He also moved setup & connection lines as well.
*  he observed in about:webrtc the log message "Error parsing attribute: candidate:Sc0a80124 1 udp 1694498815 192.168.1.36 15732 typ srflx generation 0" 

AjayG provided captures in his original post to discuss-webrtc which I forwarded in email to bwc and drno.  I can attach them here if that would help move this investigation forward more quickly.
Rank: 10
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [investigation]
From what I can tell, they're being bitten by a case-sensitive comparison on the fingerprint algorithm:

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sdp/SdpAttribute.h#321

From asterisk's SDP:

a=fingerprint:SHA-256 70:41:E5:02:5D:A1:9B:AE:98:F9:EF:BF:F0:0C:E1:34:05:F5:25:11:64:C8:6C:9E:44:20:0C:C0:40:82:A3:E7

From the (working) webrtc2sip+doubango SDP:

a=fingerprint:sha-256 A1:2C:CF:7D:10:0B:11:28:ED:53:BB:57:8C:4D:01:E9:5E:7B:E7:C1:C3:6E:4C:AF:01:D5:C9:A1:92:40:6F:E5

The lower-case form is the only correct form according to spec.
Now, we definitely have a bug in that we do not reject the SDP that does not contain any fingerprint attributes that parsed successfully.
Regarding the error parsing the candidate attribute, it seems to be missing raddr and rport (required for typ srflx).
Attached file MozReview Request: bz://1147919/bwc (obsolete) —
/r/6155 - Bug 1147919: Make sure content gets an error callback when it does not use a fingerprint algorithm we support.

Pull down this commit:

hg pull review -r f3f5fab065bafcce733426130ac3159c4fb0cce1
Comment on attachment 8583933 [details]
MozReview Request: bz://1147919/bwc

The bug here was that when there were no supported fingerprint algorithms, we ended up with an empty fingerprint attribute list instead of an absent one. We weren't checking for the former.

No try push, since try is closed. NI self to follow up on that.
Flags: needinfo?(docfaraday)
Attachment #8583933 - Flags: review?(drno)
Attachment #8583933 - Flags: review?(drno) → review+
Hi Byron Campen,

@fingerprint,
May i know Spec/Draft/RFC, where it mentioned that Encryption algorithm should be in Lower Case(or Case Sensitive).

I verified RFC's (4572 & 5763), in the sample SDP they mentioned Encryption Algorithm in in Upper case.  (a=fingerprint: SHA-1)
(In reply to AjayG from comment #8)
> Hi Byron Campen,
> 
> @fingerprint,
> May i know Spec/Draft/RFC, where it mentioned that Encryption algorithm
> should be in Lower Case(or Case Sensitive).
> 
> I verified RFC's (4572 & 5763), in the sample SDP they mentioned Encryption
> Algorithm in in Upper case.  (a=fingerprint: SHA-1)

   RFC 4572 Sec 5, page 7 (the formal grammar). But, if even the examples are wrong, it seems fair to compare in a case-insensitive fashion for the sake of interop.
(In reply to Byron Campen [:bwc] from comment #9)
> (In reply to AjayG from comment #8)
> > Hi Byron Campen,
> > 
> > @fingerprint,
> > May i know Spec/Draft/RFC, where it mentioned that Encryption algorithm
> > should be in Lower Case(or Case Sensitive).
> > 
> > I verified RFC's (4572 & 5763), in the sample SDP they mentioned Encryption
> > Algorithm in in Upper case.  (a=fingerprint: SHA-1)
> 
>    RFC 4572 Sec 5, page 7 (the formal grammar). But, if even the examples
> are wrong, it seems fair to compare in a case-insensitive fashion for the
> sake of interop.

In the RFC 4572 Section-5, case is strictly mentioned for HASH value only, but not for HASH Function. In the following lines.

"a=fingerprint" consists of the name of the hash function used, followed by the hash value itself. The hash value is represented as a sequence of uppercase hexadecimal bytes, separated by colons. 
And in following Figure-2, they mentioned case sensitivity for UHEX only.
(In reply to AjayG from comment #10)
> (In reply to Byron Campen [:bwc] from comment #9)
> > (In reply to AjayG from comment #8)
> > > Hi Byron Campen,
> > > 
> > > @fingerprint,
> > > May i know Spec/Draft/RFC, where it mentioned that Encryption algorithm
> > > should be in Lower Case(or Case Sensitive).
> > > 
> > > I verified RFC's (4572 & 5763), in the sample SDP they mentioned Encryption
> > > Algorithm in in Upper case.  (a=fingerprint: SHA-1)
> > 
> >    RFC 4572 Sec 5, page 7 (the formal grammar). But, if even the examples
> > are wrong, it seems fair to compare in a case-insensitive fashion for the
> > sake of interop.
> 
> In the RFC 4572 Section-5, case is strictly mentioned for HASH value only,
> but not for HASH Function. In the following lines.
> 
> "a=fingerprint" consists of the name of the hash function used, followed by
> the hash value itself. The hash value is represented as a sequence of
> uppercase hexadecimal bytes, separated by colons. 
> And in following Figure-2, they mentioned case sensitivity for UHEX only.

   The formal grammar has a literal "sha-256" (lower-case), so that is what implementations should be emitting. There is no ambiguity here. There is also no language specifying whether comparisons should be case sensitive or not, but in the absence of such language you cannot assume what other implementations will do, which just reinforces my previous point (other specifications, like 3261, are pretty thorough about specifying which things are case-sensitive and which are not, but in many cases where case-insensitivity is specified the sender is still explicitly required to emit exactly what is found in the grammar).

However, as I mentioned before, given that the examples are incorrect, I'd be fine with switching over to a case-insensitive comparison. Be aware, however, that the odds of getting this change into the 37 release (or even 38 at this point) are slim to none. Only serious security fixes can be uplifted straight to release, and beta is an extremely hard sell for anything but security.
Comment on attachment 8583933 [details]
MozReview Request: bz://1147919/bwc

/r/6155 - Bug 1147919 - Part 1: Make sure content gets an error callback when it does not use a fingerprint algorithm we support. r=drno
/r/6213 - Bug 1147919 - Part 2: Lowercase fingerprint alg before comparing.

Pull down these commits:

hg pull review -r 87175ea26ebf5bb780579cbc88739fd6cc1a2698
Attachment #8583933 - Flags: review+
Comment on attachment 8583933 [details]
MozReview Request: bz://1147919/bwc

Added another part that needs review.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7b08990cd9e
Flags: needinfo?(docfaraday)
Attachment #8583933 - Flags: review?(drno)
Comment on attachment 8583933 [details]
MozReview Request: bz://1147919/bwc

https://reviewboard.mozilla.org/r/6153/#review5231

Ship It!
Attachment #8583933 - Flags: review?(drno) → review+
Assignee: nobody → docfaraday
https://hg.mozilla.org/mozilla-central/rev/54e20c191061
https://hg.mozilla.org/mozilla-central/rev/aca75f4ef930
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
According to https://tools.ietf.org/html/rfc5234 ABNF literals are case insensitive:
ABNF strings are case insensitive and the character set for these
      strings is US-ASCII.

   Hence:

         rulename = "abc"

   and:

         rulename = "aBc"

   will match "abc", "Abc", "aBc", "abC", "ABc", "aBC", "AbC", and
   "ABC".

      To specify a rule that is case sensitive, specify the characters
      individually.

   For example:

         rulename    =  %d97 %d98 %d99

   or

         rulename    =  %d97.98.99
(In reply to sergio.garcia.murillo from comment #18)
> According to https://tools.ietf.org/html/rfc5234 ABNF literals are case
> insensitive:
> ABNF strings are case insensitive and the character set for these
>       strings is US-ASCII.

Ahh, right you are. I missed that. I'll have to look around and see if there are other places that need fixing.
This may be upliftable, given it's a small, relatively safe change.

Knowing the impact on use in the field would help - has Asterisk been updated?  Also, even if it has, how soon will users of Asterisk using webrtc likely update?
Flags: needinfo?(docfaraday)
Flags: needinfo?(ajay6ft)
Yes @jesup, it will be very good if we can update the path to Firefox-37.
Firefox beta channel is already updated to 38, is it possible to modify 37 now?
Flags: needinfo?(ajay6ft)
(In reply to AjayG from comment #21)
> Yes @jesup, it will be very good if we can update the path to Firefox-37.
> Firefox beta channel is already updated to 38, is it possible to modify 37
> now?

No, it's not possible to uplift to 37.  Fixing FF38 is important as that will be the basis for ESR-38 (the long-term-support version of Firefox, which will be supported for around a year (sec fixes only though).

My question to you was "has Asterisk been updated or will it be, and how quickly will that update be picked up by people using it for WebRTC?"   (Since WebRTC users may be more likely to update than general Asterisk users.)
Flags: needinfo?(ajay6ft)
@jseup, I already filed a bug to Asterisk (https://issues.asterisk.org/jira/browse/ASTERISK-24911).
And also mentioned the Change/fix there. 
Am sure that it will take couple of months, if they really want to update.
Better to uplift the fix in Firefox, as we did the mistake.
Flags: needinfo?(ajay6ft)
Comment on attachment 8583933 [details]
MozReview Request: bz://1147919/bwc

Approval Request Comment
[Feature/regressing bug #]:

   Bug 1080765

[User impact if declined]:

   Failure to interop with asterisk-based webrtc services.

[Describe test coverage new/current, TreeHerder]:

   A new test case was added to jsep_session_unittest

[Risks and why]:

   Extremely low, since these are very simple changes.

[String/UUID change made/needed]:

   None.
Flags: needinfo?(docfaraday)
Attachment #8583933 - Flags: approval-mozilla-beta?
Attachment #8583933 - Flags: approval-mozilla-aurora?
Comment on attachment 8583933 [details]
MozReview Request: bz://1147919/bwc

I landed before the last merge. No need to uplift to aurora (39) as it was in mc at this time.

Should be in 38 beta 2
Attachment #8583933 - Flags: approval-mozilla-beta?
Attachment #8583933 - Flags: approval-mozilla-beta+
Attachment #8583933 - Flags: approval-mozilla-aurora?
Attachment #8583933 - Flags: approval-mozilla-aurora-
Needs rebasing for Beta uplift.
Flags: needinfo?(docfaraday)
Duplicate of this bug: 1162015
Attachment #8583933 - Attachment is obsolete: true
Attachment #8619878 - Flags: review+
Attachment #8619879 - Flags: review+
You need to log in before you can comment on or make changes to this bug.