From 3f402b132b122f0018a9b27afe846d558ae7b40c Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Mon, 15 Nov 2021 13:03:04 +0100 Subject: [PATCH] respond with tie-break to prevent ICE restart race --- .../xmpp/jingle/JingleRtpConnection.java | 80 +++++++++++-------- .../jingle/stanzas/IceUdpTransportInfo.java | 9 +++ 2 files changed, 56 insertions(+), 33 deletions(-) diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java index 71cdb02c4..e6a34cd8e 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java @@ -261,22 +261,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web respondOk(jinglePacket); return; } - final Set> candidates = contentMap.contents.entrySet(); - if (this.state == State.SESSION_ACCEPTED) { - //zero candidates + modified credentials are an ICE restart offer - if (checkForIceRestart(contentMap, jinglePacket)) { - return; - } - respondOk(jinglePacket); - try { - processCandidates(candidates); - } catch (final WebRTCWrapper.PeerConnectionNotInitialized e) { - Log.w(Config.LOGTAG, id.account.getJid().asBareJid() + ": PeerConnection was not initialized when processing transport info. this usually indicates a race condition that can be ignored"); - } - } else { - respondOk(jinglePacket); - pendingIceCandidates.addAll(candidates); - } + receiveTransportInfo(jinglePacket, contentMap); } else { if (isTerminated()) { respondOk(jinglePacket); @@ -288,7 +273,26 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web } } - private boolean checkForIceRestart(final RtpContentMap rtpContentMap, final JinglePacket jinglePacket) { + private void receiveTransportInfo(final JinglePacket jinglePacket, final RtpContentMap contentMap) { + final Set> candidates = contentMap.contents.entrySet(); + if (this.state == State.SESSION_ACCEPTED) { + //zero candidates + modified credentials are an ICE restart offer + if (checkForIceRestart(jinglePacket, contentMap)) { + return; + } + respondOk(jinglePacket); + try { + processCandidates(candidates); + } catch (final WebRTCWrapper.PeerConnectionNotInitialized e) { + Log.w(Config.LOGTAG, id.account.getJid().asBareJid() + ": PeerConnection was not initialized when processing transport info. this usually indicates a race condition that can be ignored"); + } + } else { + respondOk(jinglePacket); + pendingIceCandidates.addAll(candidates); + } + } + + private boolean checkForIceRestart(final JinglePacket jinglePacket, final RtpContentMap rtpContentMap) { final RtpContentMap existing = getRemoteContentMap(); final Map existingCredentials = existing.getCredentials(); final Map newCredentials = rtpContentMap.getCredentials(); @@ -299,25 +303,30 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web return false; } final boolean isOffer = rtpContentMap.emptyCandidates(); - Log.d(Config.LOGTAG, "detected ICE restart. offer=" + isOffer + " " + jinglePacket); - //TODO reset to 'actpass'? + if (isOffer) { + Log.d(Config.LOGTAG, "received offer to restart ICE " + newCredentials); + } else { + Log.d(Config.LOGTAG, "received confirmation of ICE restart" + newCredentials); + } + //TODO rewrite setup attribute + //https://groups.google.com/g/discuss-webrtc/c/DfpIMwvUfeM + //https://datatracker.ietf.org/doc/html/draft-ietf-mmusic-dtls-sdp-15#section-5.5 final RtpContentMap restartContentMap = existing.modifiedCredentials(newCredentials); try { - if (applyIceRestart(isOffer, restartContentMap)) { - return false; + if (applyIceRestart(jinglePacket, restartContentMap, isOffer)) { + return isOffer; } else { - Log.d(Config.LOGTAG, "responding with tie break"); - //TODO respond with conflict + respondWithTieBreak(jinglePacket); return true; } } catch (Exception e) { Log.d(Config.LOGTAG, "failure to apply ICE restart. sending error", e); - //TODO send some kind of error + //TODO respond OK and then terminate session return true; } } - private boolean applyIceRestart(final boolean isOffer, final RtpContentMap restartContentMap) throws ExecutionException, InterruptedException { + private boolean applyIceRestart(final JinglePacket jinglePacket, final RtpContentMap restartContentMap, final boolean isOffer) throws ExecutionException, InterruptedException { final SessionDescription sessionDescription = SessionDescription.of(restartContentMap); final org.webrtc.SessionDescription.Type type = isOffer ? org.webrtc.SessionDescription.Type.OFFER : org.webrtc.SessionDescription.Type.ANSWER; org.webrtc.SessionDescription sdp = new org.webrtc.SessionDescription(type, sessionDescription.toString()); @@ -339,6 +348,8 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web webRTCWrapper.setIsReadyToReceiveIceCandidates(false); final SessionDescription localSessionDescription = setLocalSessionDescription(); setLocalContentMap(RtpContentMap.of(localSessionDescription)); + //We need to respond OK before sending any candidates + respondOk(jinglePacket); webRTCWrapper.setIsReadyToReceiveIceCandidates(true); } return true; @@ -447,6 +458,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web private void receiveSessionInitiate(final JinglePacket jinglePacket, final RtpContentMap contentMap) { try { contentMap.requireContentDescriptions(); + //TODO require actpass contentMap.requireDTLSFingerprint(); } catch (final RuntimeException e) { Log.d(Config.LOGTAG, id.account.getJid().asBareJid() + ": improperly formatted contents", Throwables.getRootCause(e)); @@ -1072,8 +1084,16 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web this.finish(); } + private void respondWithTieBreak(final JinglePacket jinglePacket) { + respondWithJingleError(jinglePacket, "tie-break", "conflict", "cancel"); + } + private void respondWithOutOfOrder(final JinglePacket jinglePacket) { - jingleConnectionManager.respondWithJingleError(id.account, jinglePacket, "out-of-order", "unexpected-request", "wait"); + respondWithJingleError(jinglePacket, "out-of-order", "unexpected-request", "wait"); + } + + void respondWithJingleError(final IqPacket original, String jingleCondition, String condition, String conditionType) { + jingleConnectionManager.respondWithJingleError(id.account, original, jingleCondition, condition, conditionType); } private void respondOk(final JinglePacket jinglePacket) { @@ -1409,7 +1429,7 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web final Collection currentUfrags = Collections2.transform(rtpContentMap.getCredentials().values(), c -> c.ufrag); final IceUdpTransportInfo.Candidate candidate = IceUdpTransportInfo.Candidate.fromSdpAttribute(iceCandidate.sdp, currentUfrags); if (candidate == null) { - Log.d(Config.LOGTAG,"ignoring (not sending) candidate: "+iceCandidate.toString()); + Log.d(Config.LOGTAG, "ignoring (not sending) candidate: " + iceCandidate.toString()); return; } Log.d(Config.LOGTAG, "sending candidate: " + iceCandidate.toString()); @@ -1448,16 +1468,10 @@ public class JingleRtpConnection extends AbstractJingleConnection implements Web @Override public void onRenegotiationNeeded() { - Log.d(Config.LOGTAG, "onRenegotiationNeeded()"); this.webRTCWrapper.execute(this::initiateIceRestart); } private void initiateIceRestart() { - PeerConnection.SignalingState signalingState = webRTCWrapper.getSignalingState(); - Log.d(Config.LOGTAG, "initiateIceRestart() - " + signalingState); - if (signalingState != PeerConnection.SignalingState.STABLE) { - return; - } this.webRTCWrapper.setIsReadyToReceiveIceCandidates(false); final SessionDescription sessionDescription; try { diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/stanzas/IceUdpTransportInfo.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/stanzas/IceUdpTransportInfo.java index 2b8770578..1586557b7 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/stanzas/IceUdpTransportInfo.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/stanzas/IceUdpTransportInfo.java @@ -1,6 +1,7 @@ package eu.siacs.conversations.xmpp.jingle.stanzas; import com.google.common.base.Joiner; +import com.google.common.base.MoreObjects; import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.base.Strings; @@ -111,6 +112,14 @@ public class IceUdpTransportInfo extends GenericTransportInfo { public int hashCode() { return Objects.hashCode(ufrag, password); } + + @Override + public String toString() { + return MoreObjects.toStringHelper(this) + .add("ufrag", ufrag) + .add("password", password) + .toString(); + } } public static class Candidate extends Element {