New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Edge] ICE error: Wrong MESSAGE-INTEGRITY #498

Closed
ibc opened this Issue Jun 1, 2017 · 27 comments

Comments

Projects
None yet
4 participants
@ibc
Contributor

ibc commented Jun 1, 2017

The issue

In some cases, Edge cannot establish the ICE connection with JVB. The next screenshot shows:

  • At the left: a successful ICE connection, and a random ICE Binding request sent by Edge which gets a OK response from JVB.
  • At the right: a similar ICE Binding request sent by Edge which gets an error response from JBC.

ice-ok-left-ice-wrong-right

The following screenshot shows the error response sent by JVB (Wrong MESSAGE-INTEGRITY):

ice-error-response

So, it may happen that:

  1. Indeed Edge is generating Binding request with wrongly calculated MESSAGE-INTEGRITY, or
  2. JVB is wrongly checking the request's MESSAGE-INTEGRITY.

How to reproduce it

I cannot yet confirm that the issue just happens in the following scenario. However it may be related:

  1. Make Chrome enter a room.
  2. Make Firefox join that room (or another Chrome).
  3. Make Edge join that room.

This scenario means that, at the beginning, Chrome and Firefox try a P2P connection and then they move to the SFU once Edge joins. From my point of view this should have zero impact in how Edge (or any other browser) performs the ICE procedures with JVB.

Said that, I've not seen any kind of ICE role conflict. The JVB sends the SDP offer so it becomes ICE controlling (as per RFC) and Edge receives the SDP offer so becomes ICE controlled. This is ok in the traces above.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Jun 2, 2017

Member

@bgrozev @gpolitis Can we look a this next week guys?

Member

saghul commented Jun 2, 2017

@bgrozev @gpolitis Can we look a this next week guys?

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 5, 2017

Contributor

NOTE: I'm perfectly able to reproduce the problem with just two browsers (Edge and Chrome) in a room. It seems to be random.

Contributor

ibc commented Jun 5, 2017

NOTE: I'm perfectly able to reproduce the problem with just two browsers (Edge and Chrome) in a room. It seems to be random.

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 6, 2017

Contributor

I've ported copy&pasted the jitsi Edge RTCPeerConnection shim into my mediasoup demo.

I've also seen a STUN 401 from mediasoup (wrong credentials) but just once and after doing many "page reloads" in the Edge tab. I will try to confirm this tomorrow so we can figure out whether this is a bug in Edge or in ice4j.

Contributor

ibc commented Jun 6, 2017

I've ported copy&pasted the jitsi Edge RTCPeerConnection shim into my mediasoup demo.

I've also seen a STUN 401 from mediasoup (wrong credentials) but just once and after doing many "page reloads" in the Edge tab. I will try to confirm this tomorrow so we can figure out whether this is a bug in Edge or in ice4j.

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 6, 2017

Member

Iñaki, do you still have the pcap files? Can you please upload them?

Member

bgrozev commented Jun 6, 2017

Iñaki, do you still have the pcap files? Can you please upload them?

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 6, 2017

Member

You can get some useful information from the bridge logs if you add this to /etc/jitsi/videobridge/logging.properties:
org.ice4j.stack.StunStack.level=FINEST

Member

bgrozev commented Jun 6, 2017

You can get some useful information from the bridge logs if you add this to /etc/jitsi/videobridge/logging.properties:
org.ice4j.stack.StunStack.level=FINEST

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 8, 2017

Contributor

AFAIS pcaps generated in the server do NOT contain captures of unsuccessful ICE connections, so I see nothing in those pcaps when Edge fails to establish ICE with the JVB. But I do have a pcap captured in Edge.

Contributor

ibc commented Jun 8, 2017

AFAIS pcaps generated in the server do NOT contain captures of unsuccessful ICE connections, so I see nothing in those pcaps when Edge fails to establish ICE with the JVB. But I do have a pcap captured in Edge.

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 8, 2017

Contributor

PCAP file captured in Edge when ICE fails (as shown above):

error2.zip

Contributor

ibc commented Jun 8, 2017

PCAP file captured in Edge when ICE fails (as shown above):

error2.zip

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 13, 2017

Member

Thanks, @ibc! It looks like there are STUN packets coming from Edge which have the wrong USERNAME attribute. I suspect the length field is not set correctly, which makes the Value field span a byte that should be padding.
https://tools.ietf.org/html/rfc5389#section-15

screen shot 2017-06-13 at 16 57 04

screen shot 2017-06-13 at 16 52 51

Member

bgrozev commented Jun 13, 2017

Thanks, @ibc! It looks like there are STUN packets coming from Edge which have the wrong USERNAME attribute. I suspect the length field is not set correctly, which makes the Value field span a byte that should be padding.
https://tools.ietf.org/html/rfc5389#section-15

screen shot 2017-06-13 at 16 57 04

screen shot 2017-06-13 at 16 52 51

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 13, 2017

Contributor

Amazing catch, @bgrozev !

However, I've never got this issue when connecting EDGE to mediasoup (with the very same RTCPeerConnection shim I've coded for jitsi). There is a difference: mediasoup is a ICE-Lite server (I set iceLite: true in the corresponding ORTC parameter in Edge) but IMHO that should not affect here since being a ICE-Lite endpoint just mean that no Binding requests are sent from the server to Edge.

Anyhow, I cannot guarantee that this error does never happen with mediasoup because I've not done so many tests as with JVB.

I can directly report this error to people in Microsoft involved in Edge's ORTC. I will report it, ok?

Thanks a lot.

Contributor

ibc commented Jun 13, 2017

Amazing catch, @bgrozev !

However, I've never got this issue when connecting EDGE to mediasoup (with the very same RTCPeerConnection shim I've coded for jitsi). There is a difference: mediasoup is a ICE-Lite server (I set iceLite: true in the corresponding ORTC parameter in Edge) but IMHO that should not affect here since being a ICE-Lite endpoint just mean that no Binding requests are sent from the server to Edge.

Anyhow, I cannot guarantee that this error does never happen with mediasoup because I've not done so many tests as with JVB.

I can directly report this error to people in Microsoft involved in Edge's ORTC. I will report it, ok?

Thanks a lot.

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 13, 2017

Member

Yeah, I think we should report this to the Edge developers.

About mediasoup: perhaps they have friendlier parsing, or they discard BRs which fail to authenticate (as opposed to sending an error response).

Member

bgrozev commented Jun 13, 2017

Yeah, I think we should report this to the Edge developers.

About mediasoup: perhaps they have friendlier parsing, or they discard BRs which fail to authenticate (as opposed to sending an error response).

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 13, 2017

Contributor

About mediasoup: perhaps they have friendlier parsing, or they discard BRs which fail to authenticate (as opposed to sending an error response).

Oh no, I'm the author, and ICE parsing is strict and, in case of error, I log it:

:)

Anyhow, the screenshots you have placed above clearly show a padding error in the USERNAME field, probably due to a wrong length value.

I'll report the issue and let you know.

Contributor

ibc commented Jun 13, 2017

About mediasoup: perhaps they have friendlier parsing, or they discard BRs which fail to authenticate (as opposed to sending an error response).

Oh no, I'm the author, and ICE parsing is strict and, in case of error, I log it:

:)

Anyhow, the screenshots you have placed above clearly show a padding error in the USERNAME field, probably due to a wrong length value.

I'll report the issue and let you know.

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 13, 2017

Member

Oh no, I'm the author, and ICE parsing is strict and, in case of error, I log it in a very verbose way

Ah, alright, I didn't know that. Then there must be something in our code which triggers it.

Another thing: in your dump there are pairs of BRs with the same transaction ID, sent one after the other (without a timeout). This doesn't seem right either.

Member

bgrozev commented Jun 13, 2017

Oh no, I'm the author, and ICE parsing is strict and, in case of error, I log it in a very verbose way

Ah, alright, I didn't know that. Then there must be something in our code which triggers it.

Another thing: in your dump there are pairs of BRs with the same transaction ID, sent one after the other (without a timeout). This doesn't seem right either.

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc
Contributor

ibc commented Jun 13, 2017

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 13, 2017

Contributor

Another thing: in your dump there are pairs of BRs with the same transaction ID, sent one after the other (without a timeout). This doesn't seem right either.

What does BR stand for?

Contributor

ibc commented Jun 13, 2017

Another thing: in your dump there are pairs of BRs with the same transaction ID, sent one after the other (without a timeout). This doesn't seem right either.

What does BR stand for?

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 13, 2017

Member

Binding Request

Member

bgrozev commented Jun 13, 2017

Binding Request

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 13, 2017

Contributor

Ah ok. Yes, I've sent that, and it produces an error in JVB ("the request with transaction ID N was already replied") but I just ignored it given that should not produce any real problem (IMHO).

Contributor

ibc commented Jun 13, 2017

Ah ok. Yes, I've sent that, and it produces an error in JVB ("the request with transaction ID N was already replied") but I just ignored it given that should not produce any real problem (IMHO).

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 23, 2017

Member

Workaround here: jitsi/ice4j#118

Member

bgrozev commented Jun 23, 2017

Workaround here: jitsi/ice4j#118

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 26, 2017

Contributor

Hi @bgrozev, the workaround does not work. Having it enabled ICE never connects.

I attach two pcaps:

a) ICE hack enabled (it does not work)
b) ICE hack disabled (it randomly works, and it this pcap it works so ICE connects)
ice-hack.zip

Contributor

ibc commented Jun 26, 2017

Hi @bgrozev, the workaround does not work. Having it enabled ICE never connects.

I attach two pcaps:

a) ICE hack enabled (it does not work)
b) ICE hack disabled (it randomly works, and it this pcap it works so ICE connects)
ice-hack.zip

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 26, 2017

Member

Thanks, @ibc, I'm looking into it.

Member

bgrozev commented Jun 26, 2017

Thanks, @ibc, I'm looking into it.

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jun 26, 2017

Member

Can you please test again on edge.jitsi.net? My fix seems broken, if that works I'll push an update.

Member

bgrozev commented Jun 26, 2017

Can you please test again on edge.jitsi.net? My fix seems broken, if that works I'll push an update.

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 26, 2017

Contributor

I'll test tomorrow. Thanks.

Contributor

ibc commented Jun 26, 2017

I'll test tomorrow. Thanks.

@saghul

This comment has been minimized.

Show comment
Hide comment
@saghul

saghul Jun 26, 2017

Member

I had a quick test with @bgrozev and it looked promising at first (3 successful calls in a row) but it failed again afterwards. We'll take more pcaps tomorrow.

Member

saghul commented Jun 26, 2017

I had a quick test with @bgrozev and it looked promising at first (3 successful calls in a row) but it failed again afterwards. We'll take more pcaps tomorrow.

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 27, 2017

Contributor

In my current tests, it's behaving as the original code. This is, it randomly successes or fails.

Contributor

ibc commented Jun 27, 2017

In my current tests, it's behaving as the original code. This is, it randomly successes or fails.

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 27, 2017

Contributor

Here a new pcap of a failing Edge connection:

2017-06-27-Edge-ICE-error.zip

Contributor

ibc commented Jun 27, 2017

Here a new pcap of a failing Edge connection:

2017-06-27-Edge-ICE-error.zip

@ibc

This comment has been minimized.

Show comment
Hide comment
@ibc

ibc Jun 27, 2017

Contributor

And here a pcap of a successful connection from Chrome:

2017-06-27-Chrome-ICE-OK.pcapng.zip

Contributor

ibc commented Jun 27, 2017

And here a pcap of a successful connection from Chrome:

2017-06-27-Chrome-ICE-OK.pcapng.zip

@bbaldino

This comment has been minimized.

Show comment
Hide comment
@bbaldino

bbaldino Aug 19, 2017

Member

i've been looking into this and have what i believe is a fix here: jitsi/ice4j#130

Member

bbaldino commented Aug 19, 2017

i've been looking into this and have what i believe is a fix here: jitsi/ice4j#130

@bbaldino

This comment has been minimized.

Show comment
Hide comment
@bbaldino

bbaldino Nov 30, 2017

Member

this was fixed by the pr above

Member

bbaldino commented Nov 30, 2017

this was fixed by the pr above

@bbaldino bbaldino closed this Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment