Closed
Bug 1147919
Opened 10 years ago
Closed 10 years ago
from discuss-webrtc post: DTLS Handshake issue with Firefox-37+ in Asterisk WebRTC Gateway
Categories
(Core :: WebRTC: Signaling, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla39
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.
Reporter | ||
Updated•10 years ago
|
Rank: 10
Flags: firefox-backlog+
Priority: -- → P1
Whiteboard: [investigation]
Assignee | ||
Comment 1•10 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•10 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•10 years ago
|
||
Regarding the error parsing the candidate attribute, it seems to be missing raddr and rport (required for typ srflx).
Assignee | ||
Comment 4•10 years ago
|
||
/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•10 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)
Comment 6•10 years ago
|
||
Updated•10 years ago
|
Attachment #8583933 -
Flags: review?(drno) → review+
Assignee | ||
Comment 7•10 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•10 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•10 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•10 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•10 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•10 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 14•10 years ago
|
||
Comment 15•10 years ago
|
||
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•10 years ago
|
Assignee: nobody → docfaraday
Assignee | ||
Comment 16•10 years ago
|
||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/54e20c191061
https://hg.mozilla.org/mozilla-central/rev/aca75f4ef930
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 18•10 years ago
|
||
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•10 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.
Comment 20•10 years ago
|
||
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•10 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)
Comment 22•10 years ago
|
||
(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•10 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•10 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?
Updated•10 years ago
|
status-firefox38:
--- → affected
Comment 25•10 years ago
|
||
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-
Assignee | ||
Comment 27•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/38cb0f4d54e8
https://hg.mozilla.org/releases/mozilla-beta/rev/a606f164b2a7
Flags: needinfo?(docfaraday)
Updated•10 years ago
|
Assignee | ||
Comment 29•9 years ago
|
||
Attachment #8583933 -
Attachment is obsolete: true
Attachment #8619878 -
Flags: review+
Attachment #8619879 -
Flags: review+
Assignee | ||
Comment 30•9 years ago
|
||
Assignee | ||
Comment 31•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•