From aecb771ab58436c41ced5c613d3feaf789457b41 Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Sun, 8 Mar 2020 13:13:15 +0100 Subject: [PATCH] use 16 byte IVs for http upload files larger than 768KiB MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ever since Android 9+ switched to Conscrypt we can no longer efficiently encrypt (and decrypt) large files with AES-GCM. We did’t notice this before because when using 16 byte IVs even modern Androids will fall back to bouncy castle. However the 'bug'/'feature' in Conscrypt surfaced when we switched over to 12 byte IVs (which uses Conscrypt on Android 9+) Switching back entirely to 16 byte IVs is undesirable as this would break compatibility with Monal. So we end up with a weird compromise where we use 12 byte for normale plain text OMEMO messages and 'small' files where the inefficiencies aren’t a problem. The result of this commit is that Monal won’t be able to receive our files larger than 768KiB. However the alternative is that Conversations would always OOM when attempting to send larger files (where large depends on the available RAM.) fixes #3653 --- build.gradle | 4 ++-- src/main/java/eu/siacs/conversations/Config.java | 1 - .../crypto/axolotl/AxolotlService.java | 6 +++--- .../crypto/axolotl/XmppAxolotlMessage.java | 11 +++++------ .../conversations/entities/DownloadableFile.java | 4 ++++ .../conversations/http/HttpUploadConnection.java | 13 +++++++++++-- 6 files changed, 25 insertions(+), 14 deletions(-) diff --git a/build.gradle b/build.gradle index ead92e284e213e21bcc9aad460e94d7bfc17868b..f9f73f18ffc8ad9cb08fbe17b5da676898b06c20 100644 --- a/build.gradle +++ b/build.gradle @@ -90,8 +90,8 @@ android { defaultConfig { minSdkVersion 16 targetSdkVersion 28 - versionCode 364 - versionName "2.7.0" + versionCode 365 + versionName "2.7.1" archivesBaseName += "-$versionName" applicationId "eu.siacs.conversations" resValue "string", "applicationId", applicationId diff --git a/src/main/java/eu/siacs/conversations/Config.java b/src/main/java/eu/siacs/conversations/Config.java index e131a4a329b1c748ab6f9d12d527b8f7f761b971..777953735f3e057c70a43fa1b66692373d8d69a0 100644 --- a/src/main/java/eu/siacs/conversations/Config.java +++ b/src/main/java/eu/siacs/conversations/Config.java @@ -100,7 +100,6 @@ public final class Config { public static final boolean REMOVE_BROKEN_DEVICES = false; public static final boolean OMEMO_PADDING = false; public static final boolean PUT_AUTH_TAG_INTO_KEY = true; - public static final boolean TWELVE_BYTE_IV = false; public static final boolean USE_BOOKMARKS2 = false; diff --git a/src/main/java/eu/siacs/conversations/crypto/axolotl/AxolotlService.java b/src/main/java/eu/siacs/conversations/crypto/axolotl/AxolotlService.java index e6775558662bca14c4550b1ab9f5da872eb78920..906080d06b3a2a7512d407f10ce4f05143355ab5 100644 --- a/src/main/java/eu/siacs/conversations/crypto/axolotl/AxolotlService.java +++ b/src/main/java/eu/siacs/conversations/crypto/axolotl/AxolotlService.java @@ -1157,7 +1157,7 @@ public class AxolotlService implements OnAdvancedStreamFeaturesLoaded { @Nullable public XmppAxolotlMessage encrypt(Message message) { - final XmppAxolotlMessage axolotlMessage = new XmppAxolotlMessage(account.getJid().asBareJid(), getOwnDeviceId()); + final XmppAxolotlMessage axolotlMessage = new XmppAxolotlMessage(account.getJid().asBareJid(), getOwnDeviceId(), true); final String content; if (message.hasFileOnRemoteHost()) { content = message.getFileParams().url.toString(); @@ -1201,7 +1201,7 @@ public class AxolotlService implements OnAdvancedStreamFeaturesLoaded { executor.execute(new Runnable() { @Override public void run() { - final XmppAxolotlMessage axolotlMessage = new XmppAxolotlMessage(account.getJid().asBareJid(), getOwnDeviceId()); + final XmppAxolotlMessage axolotlMessage = new XmppAxolotlMessage(account.getJid().asBareJid(), getOwnDeviceId(), false); if (buildHeader(axolotlMessage, conversation)) { onMessageCreatedCallback.run(axolotlMessage); } else { @@ -1362,7 +1362,7 @@ public class AxolotlService implements OnAdvancedStreamFeaturesLoaded { } private void completeSession(XmppAxolotlSession session) { - final XmppAxolotlMessage axolotlMessage = new XmppAxolotlMessage(account.getJid().asBareJid(), getOwnDeviceId()); + final XmppAxolotlMessage axolotlMessage = new XmppAxolotlMessage(account.getJid().asBareJid(), getOwnDeviceId(), true); axolotlMessage.addDevice(session, true); try { final Jid jid = Jid.of(session.getRemoteAddress().getName()); diff --git a/src/main/java/eu/siacs/conversations/crypto/axolotl/XmppAxolotlMessage.java b/src/main/java/eu/siacs/conversations/crypto/axolotl/XmppAxolotlMessage.java index 13082deac5c2a0b185e6bcc238106e3a57d7d584..e2a9a2da2e7eb3cb407b36803f28cb2e760a41a5 100644 --- a/src/main/java/eu/siacs/conversations/crypto/axolotl/XmppAxolotlMessage.java +++ b/src/main/java/eu/siacs/conversations/crypto/axolotl/XmppAxolotlMessage.java @@ -85,11 +85,11 @@ public class XmppAxolotlMessage { } } - XmppAxolotlMessage(Jid from, int sourceDeviceId) { + XmppAxolotlMessage(Jid from, int sourceDeviceId, final boolean twelveByteIv) { this.from = from; this.sourceDeviceId = sourceDeviceId; this.keys = new ArrayList<>(); - this.iv = generateIv(); + this.iv = generateIv(twelveByteIv); this.innerKey = generateKey(); } @@ -115,14 +115,13 @@ public class XmppAxolotlMessage { generator.init(128); return generator.generateKey().getEncoded(); } catch (NoSuchAlgorithmException e) { - Log.e(Config.LOGTAG, e.getMessage()); - return null; + throw new IllegalStateException(e); } } - private static byte[] generateIv() { + private static byte[] generateIv(final boolean twelveByteIv) { final SecureRandom random = new SecureRandom(); - byte[] iv = new byte[Config.TWELVE_BYTE_IV ? 12 : 16]; + byte[] iv = new byte[twelveByteIv ? 12 : 16]; random.nextBytes(iv); return iv; } diff --git a/src/main/java/eu/siacs/conversations/entities/DownloadableFile.java b/src/main/java/eu/siacs/conversations/entities/DownloadableFile.java index c88af6f950ecb70f02609c73f4f8f154027d6ef7..c0b3512a35c8342f85ff6c83991318b4290508e2 100644 --- a/src/main/java/eu/siacs/conversations/entities/DownloadableFile.java +++ b/src/main/java/eu/siacs/conversations/entities/DownloadableFile.java @@ -1,7 +1,10 @@ package eu.siacs.conversations.entities; +import android.util.Log; + import java.io.File; +import eu.siacs.conversations.Config; import eu.siacs.conversations.utils.MimeUtils; public class DownloadableFile extends File { @@ -66,6 +69,7 @@ public class DownloadableFile extends File { this.iv = new byte[]{ 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0xf }; System.arraycopy(keyIvCombo, 0, aeskey, 0, 32); } + Log.d(Config.LOGTAG,"using "+this.iv.length+"-byte IV for file transmission"); } public void setKey(byte[] key) { diff --git a/src/main/java/eu/siacs/conversations/http/HttpUploadConnection.java b/src/main/java/eu/siacs/conversations/http/HttpUploadConnection.java index 08fae1bc5f54beba63e2cfc26e3fccfc1d456306..ea893924f03e7d081b80389b66cf3b54a1b3226d 100644 --- a/src/main/java/eu/siacs/conversations/http/HttpUploadConnection.java +++ b/src/main/java/eu/siacs/conversations/http/HttpUploadConnection.java @@ -105,11 +105,20 @@ public class HttpUploadConnection implements Transferable { } else { this.mime = this.file.getMimeType(); } + final long originalFileSize = file.getSize(); this.delayed = delay; if (Config.ENCRYPT_ON_HTTP_UPLOADED || message.getEncryption() == Message.ENCRYPTION_AXOLOTL || message.getEncryption() == Message.ENCRYPTION_OTR) { - this.key = new byte[Config.TWELVE_BYTE_IV ? 44 : 48]; + //ok, this is going to sound super crazy but on Android 9+ a 12 byte IV will use the + //internal conscrypt library (provided by the OS) instead of bounce castle, while 16 bytes + //will still 'fallback' to bounce castle even on Android 9+ because conscrypt doesnt + //have support for anything but 12. + //For large files conscrypt has extremely bad performance; so why not always use 16 you ask? + //well the ecosystem was moving and some clients like Monal *only* support 16 + //so the result of this code is that we can only send 'small' files to Monal. + //'small' was relatively arbitrarily choose and correlates to roughly 'small' compressed images + this.key = new byte[originalFileSize <= 786432 ? 44 : 48]; mXmppConnectionService.getRNG().nextBytes(this.key); this.file.setKeyAndIv(this.key); } @@ -128,7 +137,7 @@ public class HttpUploadConnection implements Transferable { md5 = null; } - this.file.setExpectedSize(file.getSize() + (file.getKey() != null ? 16 : 0)); + this.file.setExpectedSize(originalFileSize + (file.getKey() != null ? 16 : 0)); message.resetFileParams(); this.mSlotRequester.request(method, account, file, mime, md5, new SlotRequester.OnSlotRequested() { @Override