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

RESOLVED FIXED in Firefox 38

Status

()

defect
P1
normal
Rank:
10
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mreavy, Assigned: bwc)

Tracking

unspecified
mozilla39
x86_64
Windows 7
Points:
---
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed)

Details

(Whiteboard: [investigation])

Attachments

(2 attachments, 1 obsolete attachment)

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]
Assignee

Comment 1

4 years ago
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.
Assignee

Comment 2

4 years ago
Now, we definitely have a bug in that we do not reject the SDP that does not contain any fingerprint attributes that parsed successfully.
Assignee

Comment 3

4 years ago
Regarding the error parsing the candidate attribute, it seems to be missing raddr and rport (required for typ srflx).
Assignee

Comment 4

4 years ago
Posted 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
Assignee

Comment 5

4 years ago
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+

Comment 8

4 years ago
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)
Assignee

Comment 9

4 years ago
(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.

Comment 10

4 years ago
(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.
Assignee

Comment 11

4 years ago
(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.
Assignee

Comment 12

4 years ago
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+
Assignee

Comment 13

4 years ago
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

Updated

4 years ago
Assignee: nobody → docfaraday
https://hg.mozilla.org/mozilla-central/rev/54e20c191061
https://hg.mozilla.org/mozilla-central/rev/aca75f4ef930
Status: NEW → RESOLVED
Closed: 4 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
Assignee

Comment 19

4 years ago
(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)

Comment 21

4 years ago
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)

Comment 23

4 years ago
@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)
Assignee

Comment 24

4 years ago
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)

Updated

4 years ago
Duplicate of this bug: 1162015
Assignee

Comment 29

4 years ago
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.