From afb432dceea26c53683b5d1b14cbbe68693fd588 Mon Sep 17 00:00:00 2001 From: Paul Schaub Date: Wed, 10 Oct 2018 13:00:07 +0200 Subject: [PATCH 1/3] Add store methods for message counters --- .../smackx/omemo/CachingOmemoStore.java | 25 +++++++++++++++++++ .../smackx/omemo/FileBasedOmemoStore.java | 24 ++++++++++++++++++ .../jivesoftware/smackx/omemo/OmemoStore.java | 21 ++++++++++++++++ .../smackx/omemo/OmemoStoreTest.java | 7 ++++++ 4 files changed, 77 insertions(+) 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 FileBasedOmemoStore Date: Wed, 10 Oct 2018 14:05:45 +0200 Subject: [PATCH 2/3] Business logic for handling read-only devices --- .../smackx/omemo/OmemoConfiguration.java | 57 ++++++++++---- .../smackx/omemo/OmemoService.java | 76 ++++++++----------- .../exceptions/ReadOnlyDeviceException.java | 52 +++++++++++++ .../smackx/omemo/OmemoConfigurationTest.java | 14 ---- .../smackx/omemo/OmemoServiceTest.java | 7 -- 5 files changed, 123 insertions(+), 83 deletions(-) create mode 100644 smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/exceptions/ReadOnlyDeviceException.java diff --git a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoConfiguration.java b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoConfiguration.java index a529162fd..8756ef37e 100644 --- a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoConfiguration.java +++ b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoConfiguration.java @@ -23,31 +23,56 @@ package org.jivesoftware.smackx.omemo; */ public final class OmemoConfiguration { + private static boolean IGNORE_READ_ONLY_DEVICES = true; + private static int MAX_READ_ONLY_MESSAGE_COUNT = 400; + /** - * Ignore own other stale devices that we did not receive a message from for a period of time. - * Ignoring means do not encrypt messages for them. This helps to mitigate stale devices that threaten - * forward secrecy by never advancing ratchets. + * Set to true, in order to ignore read-only devices. + * + * @param ignore ignore read-only devices + * @see Blog Post explaining the danger of read-only devices. */ - 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. + */ + 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. + */ + 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. + */ + 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..f8d38706d 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 @@ -19,6 +19,9 @@ package org.jivesoftware.smackx.omemo; import static org.jivesoftware.smackx.omemo.util.OmemoConstants.Crypto.KEYLENGTH; import static org.jivesoftware.smackx.omemo.util.OmemoConstants.Crypto.KEYTYPE; +import javax.crypto.BadPaddingException; +import javax.crypto.IllegalBlockSizeException; +import javax.crypto.NoSuchPaddingException; import java.io.UnsupportedEncodingException; import java.security.InvalidAlgorithmParameterException; import java.security.InvalidKeyException; @@ -36,14 +39,10 @@ import java.util.Random; import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; -import javax.crypto.BadPaddingException; -import javax.crypto.IllegalBlockSizeException; -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 Date: Wed, 10 Oct 2018 14:48:22 +0200 Subject: [PATCH 3/3] Add integration test and fix checkstyle issues --- .../omemo/ReadOnlyDeviceIntegrationTest.java | 72 +++++++++++++++++++ .../smackx/omemo/OmemoConfiguration.java | 8 +-- .../smackx/omemo/OmemoService.java | 6 +- 3 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 smack-integration-test/src/main/java/org/jivesoftware/smackx/omemo/ReadOnlyDeviceIntegrationTest.java 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/OmemoConfiguration.java b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoConfiguration.java index 8756ef37e..c53f97dac 100644 --- a/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoConfiguration.java +++ b/smack-omemo/src/main/java/org/jivesoftware/smackx/omemo/OmemoConfiguration.java @@ -30,7 +30,7 @@ public final class OmemoConfiguration { * Set to true, in order to ignore read-only devices. * * @param ignore ignore read-only devices - * @see Blog Post explaining the danger of read-only devices. + * @see Blog Post explaining the danger of read-only devices. TODO: Add URL */ public static void setIgnoreReadOnlyDevices(boolean ignore) { IGNORE_READ_ONLY_DEVICES = ignore; @@ -40,7 +40,7 @@ public final class OmemoConfiguration { * 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. + * @see Blog Post explaining the danger of read-only devices. TODO: Add URL */ public static boolean getIgnoreReadOnlyDevices() { return IGNORE_READ_ONLY_DEVICES; @@ -53,7 +53,7 @@ public final class OmemoConfiguration { * 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. + * @see Blog Post explaining the danger of read-only devices. TODO: Add URL */ public static void setMaxReadOnlyMessageCount(int maxReadOnlyMessageCount) { if (maxReadOnlyMessageCount <= 0) { @@ -69,7 +69,7 @@ public final class OmemoConfiguration { * 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. + * @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 f8d38706d..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 @@ -19,9 +19,6 @@ package org.jivesoftware.smackx.omemo; import static org.jivesoftware.smackx.omemo.util.OmemoConstants.Crypto.KEYLENGTH; import static org.jivesoftware.smackx.omemo.util.OmemoConstants.Crypto.KEYTYPE; -import javax.crypto.BadPaddingException; -import javax.crypto.IllegalBlockSizeException; -import javax.crypto.NoSuchPaddingException; import java.io.UnsupportedEncodingException; import java.security.InvalidAlgorithmParameterException; import java.security.InvalidKeyException; @@ -39,6 +36,9 @@ import java.util.Random; import java.util.Set; import java.util.logging.Level; import java.util.logging.Logger; +import javax.crypto.BadPaddingException; +import javax.crypto.IllegalBlockSizeException; +import javax.crypto.NoSuchPaddingException; import org.jivesoftware.smack.SmackException; import org.jivesoftware.smack.XMPPConnection;