diff --git a/smack-integration-test/src/main/java/org/jivesoftware/smackx/omemo/ReadOnlyDeviceIntegrationTest.java b/smack-integration-test/src/main/java/org/jivesoftware/smackx/omemo/ReadOnlyDeviceIntegrationTest.java new file mode 100644 index 000000000..69d611ea4 --- /dev/null +++ b/smack-integration-test/src/main/java/org/jivesoftware/smackx/omemo/ReadOnlyDeviceIntegrationTest.java @@ -0,0 +1,72 @@ +/** + * + * Copyright 2018 Paul Schaub + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.jivesoftware.smackx.omemo; + +import static junit.framework.TestCase.assertEquals; +import static junit.framework.TestCase.assertFalse; +import static junit.framework.TestCase.assertTrue; + +import org.jivesoftware.smack.SmackException; +import org.jivesoftware.smack.XMPPException; +import org.jivesoftware.smackx.omemo.exceptions.CryptoFailedException; +import org.jivesoftware.smackx.omemo.exceptions.ReadOnlyDeviceException; +import org.jivesoftware.smackx.omemo.exceptions.UndecidedOmemoIdentityException; + +import org.igniterealtime.smack.inttest.SmackIntegrationTest; +import org.igniterealtime.smack.inttest.SmackIntegrationTestEnvironment; +import org.igniterealtime.smack.inttest.TestNotPossibleException; + +public class ReadOnlyDeviceIntegrationTest extends AbstractTwoUsersOmemoIntegrationTest { + + public ReadOnlyDeviceIntegrationTest(SmackIntegrationTestEnvironment environment) throws XMPPException.XMPPErrorException, SmackException.NotConnectedException, InterruptedException, SmackException.NoResponseException, TestNotPossibleException { + super(environment); + } + + @SmackIntegrationTest + public void test() throws InterruptedException, SmackException.NoResponseException, SmackException.NotLoggedInException, SmackException.NotConnectedException, CryptoFailedException, UndecidedOmemoIdentityException { + boolean prevIgnoreReadOnlyConf = OmemoConfiguration.getIgnoreReadOnlyDevices(); + int prevMaxMessageCounter = OmemoConfiguration.getMaxReadOnlyMessageCount(); + + OmemoConfiguration.setIgnoreReadOnlyDevices(true); + // Set the maxReadOnlyMessageCount to ridiculously low threshold of 5. + // This means that Alice will be able to encrypt 5 messages for Bob, while the 6th will not be encrypted for Bob. + OmemoConfiguration.setMaxReadOnlyMessageCount(5); + + // Reset counter to begin test + alice.getOmemoService().getOmemoStoreBackend().storeOmemoMessageCounter(alice.getOwnDevice(), bob.getOwnDevice(), 0); + + // Since the max threshold is 5, we must be able to encrypt 5 messages for Bob. + for (int i = 0; i < 5; i++) { + assertEquals(i, alice.getOmemoService().getOmemoStoreBackend().loadOmemoMessageCounter(alice.getOwnDevice(), bob.getOwnDevice())); + OmemoMessage.Sent message = alice.encrypt(bob.getOwnJid(), "Hello World!"); + assertFalse(message.getSkippedDevices().containsKey(bob.getOwnDevice())); + } + + // Now the message counter must be too high and Bobs device must be skipped. + OmemoMessage.Sent message = alice.encrypt(bob.getOwnJid(), "Hello World!"); + Throwable exception = message.getSkippedDevices().get(bob.getOwnDevice()); + assertTrue(exception instanceof ReadOnlyDeviceException); + assertEquals(bob.getOwnDevice(), ((ReadOnlyDeviceException) exception).getDevice()); + + // Reset the message counter + alice.getOmemoService().getOmemoStoreBackend().storeOmemoMessageCounter(alice.getOwnDevice(), bob.getOwnDevice(), 0); + + // Reset the configuration to previous values + OmemoConfiguration.setMaxReadOnlyMessageCount(prevMaxMessageCounter); + OmemoConfiguration.setIgnoreReadOnlyDevices(prevIgnoreReadOnlyConf); + } +} diff --git a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/CachingOmemoStore.java b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/CachingOmemoStore.java index ec4b23a46..7ab0bcde6 100644 --- a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/CachingOmemoStore.java +++ b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/CachingOmemoStore.java @@ -140,6 +140,30 @@ public class CachingOmemoStore lastDeviceIdPublicationDates = new HashMap<>(); private final HashMap deviceLists = new HashMap<>(); private Date lastRenewalDate = null; + private final HashMap messageCounters = new HashMap<>(); } } diff --git a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/FileBasedOmemoStore.java b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/FileBasedOmemoStore.java index 4ecefcda7..bab399c35 100644 --- a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/FileBasedOmemoStore.java +++ b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/FileBasedOmemoStore.java @@ -24,6 +24,7 @@ import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.FileOutputStream; import java.io.IOException; +import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.HashSet; @@ -353,6 +354,24 @@ public abstract class FileBasedOmemoStore integers = readIntegers(messageCounterFile); + + if (integers == null || integers.isEmpty()) { + return 0; + } + + return integers.iterator().next(); + } + @Override public OmemoCachedDeviceList loadCachedDeviceList(OmemoDevice userDevice, BareJid contact) { OmemoCachedDeviceList cachedDeviceList = new OmemoCachedDeviceList(); @@ -732,6 +751,7 @@ public abstract class FileBasedOmemoStoreBlog Post explaining the danger of read-only devices. TODO: Add URL */ - private static boolean IGNORE_STALE_DEVICES = true; - private static int IGNORE_STALE_DEVICE_AFTER_HOURS = 24 * 7; //One week - - public static void setIgnoreStaleDevices(boolean ignore) { - IGNORE_STALE_DEVICES = ignore; + public static void setIgnoreReadOnlyDevices(boolean ignore) { + IGNORE_READ_ONLY_DEVICES = ignore; } - public static boolean getIgnoreStaleDevices() { - return IGNORE_STALE_DEVICES; + /** + * Return true, if the client should stop encrypting messages to a read-only device. + * + * @return true if read-only devices should get ignored after a certain amount of unanswered messages. + * @see Blog Post explaining the danger of read-only devices. TODO: Add URL + */ + public static boolean getIgnoreReadOnlyDevices() { + return IGNORE_READ_ONLY_DEVICES; } - public static void setIgnoreStaleDevicesAfterHours(int hours) { - if (hours <= 0) { - throw new IllegalArgumentException("Hours must be greater than 0."); + /** + * Set the maximum amount of messages that the client is allowed to send to a read-only device without getting a + * response. Once the message counter of a device reaches that value, the client will stop encrypting messages for + * the device (given that {@link #getIgnoreReadOnlyDevices()} is true). + * This threshold is used to prevent read-only devices from weakening forward secrecy. + * + * @param maxReadOnlyMessageCount maximum number of allowed messages to a read-only device. + * @see Blog Post explaining the danger of read-only devices. TODO: Add URL + */ + public static void setMaxReadOnlyMessageCount(int maxReadOnlyMessageCount) { + if (maxReadOnlyMessageCount <= 0) { + throw new IllegalArgumentException("maxReadOnlyMessageCount MUST be greater than 0."); } - IGNORE_STALE_DEVICE_AFTER_HOURS = hours; + MAX_READ_ONLY_MESSAGE_COUNT = maxReadOnlyMessageCount; } - public static int getIgnoreStaleDevicesAfterHours() { - return IGNORE_STALE_DEVICE_AFTER_HOURS; + /** + * Get the maximum amount of messages that the client is allowed to send to a read-only device without getting a + * response. Once the message counter of a device reaches that value, the client will stop encrypting messages for + * the device (given that {@link #getIgnoreReadOnlyDevices()} is true). + * This threshold is used to prevent read-only devices from weakening forward secrecy. + * + * @return maximum number of allowed messages to a read-only device. + * @see Blog Post explaining the danger of read-only devices. TODO: Add URL + */ + public static int getMaxReadOnlyMessageCount() { + return MAX_READ_ONLY_MESSAGE_COUNT; } /** diff --git a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoService.java b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoService.java index d29d3339c..27f775f3e 100644 --- a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoService.java +++ b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoService.java @@ -43,7 +43,6 @@ import javax.crypto.NoSuchPaddingException; import org.jivesoftware.smack.SmackException; import org.jivesoftware.smack.XMPPConnection; import org.jivesoftware.smack.XMPPException; - import org.jivesoftware.smack.packet.Message; import org.jivesoftware.smack.packet.Stanza; import org.jivesoftware.smack.packet.StanzaError; @@ -62,7 +61,7 @@ import org.jivesoftware.smackx.omemo.exceptions.CorruptedOmemoKeyException; import org.jivesoftware.smackx.omemo.exceptions.CryptoFailedException; import org.jivesoftware.smackx.omemo.exceptions.NoIdentityKeyException; import org.jivesoftware.smackx.omemo.exceptions.NoRawSessionException; -import org.jivesoftware.smackx.omemo.exceptions.StaleDeviceException; +import org.jivesoftware.smackx.omemo.exceptions.ReadOnlyDeviceException; import org.jivesoftware.smackx.omemo.exceptions.UndecidedOmemoIdentityException; import org.jivesoftware.smackx.omemo.exceptions.UntrustedOmemoIdentityException; import org.jivesoftware.smackx.omemo.internal.CipherAndAuthTag; @@ -323,6 +322,8 @@ public abstract class OmemoService= OmemoConfiguration.getMaxReadOnlyMessageCount(); - boolean stale = isStale(userDevice, contactsDevice, lastPublicationDate, OmemoConfiguration.getIgnoreStaleDevicesAfterHours()); - stale &= isStale(userDevice, contactsDevice, lastMessageDate, OmemoConfiguration.getIgnoreStaleDevicesAfterHours()); + if (readOnly) { + LOGGER.log(Level.FINE, "Device " + contactsDevice + " seems to be read-only (We sent " + + messageCounter + " messages without getting a reply back (max allowed is " + + OmemoConfiguration.getMaxReadOnlyMessageCount() + "). Ignoring the device."); + skippedRecipients.put(contactsDevice, new ReadOnlyDeviceException(contactsDevice)); - if (stale) { - LOGGER.log(Level.FINE, "Device " + contactsDevice + " seems to be stale (last message received " - + lastMessageDate + ", last publication of deviceId: " + lastPublicationDate + "). Ignore it."); - skippedRecipients.put(contactsDevice, new StaleDeviceException(contactsDevice, lastMessageDate, lastPublicationDate)); + // Skip this device and handle next device continue; } } @@ -428,6 +421,10 @@ public abstract class OmemoService