From 372078629b23a39ff3ef1ce2d11081e489af8e5a Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Fri, 25 Feb 2022 17:26:36 +0100 Subject: [PATCH] fix ice candidate sending when different credentials are used --- .../xmpp/jingle/JingleRtpConnection.java | 19 ++- .../xmpp/jingle/RtpContentMap.java | 150 ++++++++++++------ .../jingle/stanzas/IceUdpTransportInfo.java | 3 + 3 files changed, 117 insertions(+), 55 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 d834d81a1..fd918fa9b 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/JingleRtpConnection.java @@ -33,8 +33,6 @@ import java.util.List; import java.util.Map; import java.util.Queue; import java.util.Set; -import java.util.Timer; -import java.util.TimerTask; import java.util.concurrent.ExecutionException; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -349,16 +347,16 @@ public class JingleRtpConnection extends AbstractJingleConnection private boolean checkForIceRestart( final JinglePacket jinglePacket, final RtpContentMap rtpContentMap) { final RtpContentMap existing = getRemoteContentMap(); - final IceUdpTransportInfo.Credentials existingCredentials; + final Set existingCredentials; final IceUdpTransportInfo.Credentials newCredentials; try { existingCredentials = existing.getCredentials(); - newCredentials = rtpContentMap.getCredentials(); + newCredentials = rtpContentMap.getDistinctCredentials(); } catch (final IllegalStateException e) { Log.d(Config.LOGTAG, "unable to gather credentials for comparison", e); return false; } - if (existingCredentials.equals(newCredentials)) { + if (existingCredentials.contains(newCredentials)) { return false; } // TODO an alternative approach is to check if we already got an iq result to our @@ -1849,9 +1847,16 @@ public class JingleRtpConnection extends AbstractJingleConnection public void onIceCandidate(final IceCandidate iceCandidate) { final RtpContentMap rtpContentMap = isInitiator() ? this.initiatorRtpContentMap : this.responderRtpContentMap; - final String ufrag = rtpContentMap.getCredentials().ufrag; + final IceUdpTransportInfo.Credentials credentials; + try { + credentials = rtpContentMap.getCredentials(iceCandidate.sdpMid); + } catch (final IllegalArgumentException e) { + Log.d(Config.LOGTAG, "ignoring (not sending) candidate: " + iceCandidate, e); + return; + } + final String uFrag = credentials.ufrag; final IceUdpTransportInfo.Candidate candidate = - IceUdpTransportInfo.Candidate.fromSdpAttribute(iceCandidate.sdp, ufrag); + IceUdpTransportInfo.Candidate.fromSdpAttribute(iceCandidate.sdp, uFrag); if (candidate == null) { Log.d(Config.LOGTAG, "ignoring (not sending) candidate: " + iceCandidate); return; diff --git a/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java b/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java index 21684a165..e95a7e36d 100644 --- a/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java +++ b/src/main/java/eu/siacs/conversations/xmpp/jingle/RtpContentMap.java @@ -16,7 +16,6 @@ import org.checkerframework.checker.nullness.compatqual.NullableDecl; import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.Objects; import java.util.Set; import eu.siacs.conversations.xmpp.jingle.stanzas.Content; @@ -39,7 +38,8 @@ public class RtpContentMap { } public static RtpContentMap of(final JinglePacket jinglePacket) { - final Map contents = DescriptionTransport.of(jinglePacket.getJingleContents()); + final Map contents = + DescriptionTransport.of(jinglePacket.getJingleContents()); if (isOmemoVerified(contents)) { return new OmemoVerifiedRtpContentMap(jinglePacket.getGroup(), contents); } else { @@ -62,22 +62,30 @@ public class RtpContentMap { } public static RtpContentMap of(final SessionDescription sessionDescription) { - final ImmutableMap.Builder contentMapBuilder = new ImmutableMap.Builder<>(); + final ImmutableMap.Builder contentMapBuilder = + new ImmutableMap.Builder<>(); for (SessionDescription.Media media : sessionDescription.media) { final String id = Iterables.getFirst(media.attributes.get("mid"), null); Preconditions.checkNotNull(id, "media has no mid"); contentMapBuilder.put(id, DescriptionTransport.of(sessionDescription, media)); } - final String groupAttribute = Iterables.getFirst(sessionDescription.attributes.get("group"), null); + final String groupAttribute = + Iterables.getFirst(sessionDescription.attributes.get("group"), null); final Group group = groupAttribute == null ? null : Group.ofSdpString(groupAttribute); return new RtpContentMap(group, contentMapBuilder.build()); } public Set getMedia() { - return Sets.newHashSet(Collections2.transform(contents.values(), input -> { - final RtpDescription rtpDescription = input == null ? null : input.description; - return rtpDescription == null ? Media.UNKNOWN : input.description.getMedia(); - })); + return Sets.newHashSet( + Collections2.transform( + contents.values(), + input -> { + final RtpDescription rtpDescription = + input == null ? null : input.description; + return rtpDescription == null + ? Media.UNKNOWN + : input.description.getMedia(); + })); } public List getNames() { @@ -90,7 +98,8 @@ public class RtpContentMap { } for (Map.Entry entry : this.contents.entrySet()) { if (entry.getValue().description == null) { - throw new IllegalStateException(String.format("%s is lacking content description", entry.getKey())); + throw new IllegalStateException( + String.format("%s is lacking content description", entry.getKey())); } } } @@ -106,15 +115,24 @@ public class RtpContentMap { for (Map.Entry entry : this.contents.entrySet()) { final IceUdpTransportInfo transport = entry.getValue().transport; final IceUdpTransportInfo.Fingerprint fingerprint = transport.getFingerprint(); - if (fingerprint == null || Strings.isNullOrEmpty(fingerprint.getContent()) || Strings.isNullOrEmpty(fingerprint.getHash())) { - throw new SecurityException(String.format("Use of DTLS-SRTP (XEP-0320) is required for content %s", entry.getKey())); + if (fingerprint == null + || Strings.isNullOrEmpty(fingerprint.getContent()) + || Strings.isNullOrEmpty(fingerprint.getHash())) { + throw new SecurityException( + String.format( + "Use of DTLS-SRTP (XEP-0320) is required for content %s", + entry.getKey())); } final IceUdpTransportInfo.Setup setup = fingerprint.getSetup(); if (setup == null) { - throw new SecurityException(String.format("Use of DTLS-SRTP (XEP-0320) is required for content %s but missing setup attribute", entry.getKey())); + throw new SecurityException( + String.format( + "Use of DTLS-SRTP (XEP-0320) is required for content %s but missing setup attribute", + entry.getKey())); } if (requireActPass && setup != IceUdpTransportInfo.Setup.ACTPASS) { - throw new SecurityException("Initiator needs to offer ACTPASS as setup for DTLS-SRTP (XEP-0320)"); + throw new SecurityException( + "Initiator needs to offer ACTPASS as setup for DTLS-SRTP (XEP-0320)"); } } } @@ -135,41 +153,66 @@ public class RtpContentMap { return jinglePacket; } - RtpContentMap transportInfo(final String contentName, final IceUdpTransportInfo.Candidate candidate) { + RtpContentMap transportInfo( + final String contentName, final IceUdpTransportInfo.Candidate candidate) { final RtpContentMap.DescriptionTransport descriptionTransport = contents.get(contentName); - final IceUdpTransportInfo transportInfo = descriptionTransport == null ? null : descriptionTransport.transport; + final IceUdpTransportInfo transportInfo = + descriptionTransport == null ? null : descriptionTransport.transport; if (transportInfo == null) { - throw new IllegalArgumentException("Unable to find transport info for content name " + contentName); + throw new IllegalArgumentException( + "Unable to find transport info for content name " + contentName); } final IceUdpTransportInfo newTransportInfo = transportInfo.cloneWrapper(); newTransportInfo.addChild(candidate); - return new RtpContentMap(null, ImmutableMap.of(contentName, new DescriptionTransport(null, newTransportInfo))); + return new RtpContentMap( + null, + ImmutableMap.of(contentName, new DescriptionTransport(null, newTransportInfo))); } RtpContentMap transportInfo() { return new RtpContentMap( null, - Maps.transformValues(contents, dt -> new DescriptionTransport(null, dt.transport.cloneWrapper())) - ); + Maps.transformValues( + contents, + dt -> new DescriptionTransport(null, dt.transport.cloneWrapper()))); } - public IceUdpTransportInfo.Credentials getCredentials() { - final Set allCredentials = ImmutableSet.copyOf(Collections2.transform( - contents.values(), - dt -> dt.transport.getCredentials() - )); - final IceUdpTransportInfo.Credentials credentials = Iterables.getFirst(allCredentials, null); + public IceUdpTransportInfo.Credentials getDistinctCredentials() { + final Set allCredentials = getCredentials(); + final IceUdpTransportInfo.Credentials credentials = + Iterables.getFirst(allCredentials, null); if (allCredentials.size() == 1 && credentials != null) { return credentials; } throw new IllegalStateException("Content map does not have distinct credentials"); } + public Set getCredentials() { + final Set credentials = + ImmutableSet.copyOf( + Collections2.transform( + contents.values(), dt -> dt.transport.getCredentials())); + if (credentials.isEmpty()) { + throw new IllegalStateException("Content map does not have any credentials"); + } + return credentials; + } + + public IceUdpTransportInfo.Credentials getCredentials(final String contentName) { + final DescriptionTransport descriptionTransport = this.contents.get(contentName); + if (descriptionTransport == null) { + throw new IllegalArgumentException( + String.format( + "Unable to find transport info for content name %s", contentName)); + } + return descriptionTransport.transport.getCredentials(); + } + public IceUdpTransportInfo.Setup getDtlsSetup() { - final Set setups = ImmutableSet.copyOf(Collections2.transform( - contents.values(), - dt -> dt.transport.getFingerprint().getSetup() - )); + final Set setups = + ImmutableSet.copyOf( + Collections2.transform( + contents.values(), dt -> dt.transport.getFingerprint().getSetup())); final IceUdpTransportInfo.Setup setup = Iterables.getFirst(setups, null); if (setups.size() == 1 && setup != null) { return setup; @@ -185,13 +228,18 @@ public class RtpContentMap { return count == 0; } - public RtpContentMap modifiedCredentials(IceUdpTransportInfo.Credentials credentials, final IceUdpTransportInfo.Setup setup) { - final ImmutableMap.Builder contentMapBuilder = new ImmutableMap.Builder<>(); + public RtpContentMap modifiedCredentials( + IceUdpTransportInfo.Credentials credentials, final IceUdpTransportInfo.Setup setup) { + final ImmutableMap.Builder contentMapBuilder = + new ImmutableMap.Builder<>(); for (final Map.Entry content : contents.entrySet()) { final RtpDescription rtpDescription = content.getValue().description; IceUdpTransportInfo transportInfo = content.getValue().transport; - final IceUdpTransportInfo modifiedTransportInfo = transportInfo.modifyCredentials(credentials, setup); - contentMapBuilder.put(content.getKey(), new DescriptionTransport(rtpDescription, modifiedTransportInfo)); + final IceUdpTransportInfo modifiedTransportInfo = + transportInfo.modifyCredentials(credentials, setup); + contentMapBuilder.put( + content.getKey(), + new DescriptionTransport(rtpDescription, modifiedTransportInfo)); } return new RtpContentMap(this.group, contentMapBuilder.build()); } @@ -200,7 +248,8 @@ public class RtpContentMap { public final RtpDescription description; public final IceUdpTransportInfo transport; - public DescriptionTransport(final RtpDescription description, final IceUdpTransportInfo transport) { + public DescriptionTransport( + final RtpDescription description, final IceUdpTransportInfo transport) { this.description = description; this.transport = transport; } @@ -215,33 +264,38 @@ public class RtpContentMap { } else if (description instanceof RtpDescription) { rtpDescription = (RtpDescription) description; } else { - throw new UnsupportedApplicationException("Content does not contain rtp description"); + throw new UnsupportedApplicationException( + "Content does not contain rtp description"); } if (transportInfo instanceof IceUdpTransportInfo) { iceUdpTransportInfo = (IceUdpTransportInfo) transportInfo; } else { - throw new UnsupportedTransportException("Content does not contain ICE-UDP transport"); + throw new UnsupportedTransportException( + "Content does not contain ICE-UDP transport"); } return new DescriptionTransport( - rtpDescription, - OmemoVerifiedIceUdpTransportInfo.upgrade(iceUdpTransportInfo) - ); + rtpDescription, OmemoVerifiedIceUdpTransportInfo.upgrade(iceUdpTransportInfo)); } - public static DescriptionTransport of(final SessionDescription sessionDescription, final SessionDescription.Media media) { + public static DescriptionTransport of( + final SessionDescription sessionDescription, final SessionDescription.Media media) { final RtpDescription rtpDescription = RtpDescription.of(sessionDescription, media); - final IceUdpTransportInfo transportInfo = IceUdpTransportInfo.of(sessionDescription, media); + final IceUdpTransportInfo transportInfo = + IceUdpTransportInfo.of(sessionDescription, media); return new DescriptionTransport(rtpDescription, transportInfo); } public static Map of(final Map contents) { - return ImmutableMap.copyOf(Maps.transformValues(contents, new Function() { - @NullableDecl - @Override - public DescriptionTransport apply(@NullableDecl Content content) { - return content == null ? null : of(content); - } - })); + return ImmutableMap.copyOf( + Maps.transformValues( + contents, + new Function() { + @NullableDecl + @Override + public DescriptionTransport apply(@NullableDecl Content content) { + return content == null ? null : of(content); + } + })); } } 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 45260cafb..ee8d12b70 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,5 +1,7 @@ package eu.siacs.conversations.xmpp.jingle.stanzas; +import androidx.annotation.NonNull; + import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Objects; @@ -123,6 +125,7 @@ public class IceUdpTransportInfo extends GenericTransportInfo { } @Override + @NonNull public String toString() { return MoreObjects.toStringHelper(this) .add("ufrag", ufrag)