From 546dd5485c58489af34372f5218ce1e863a46e3a Mon Sep 17 00:00:00 2001 From: Simeon Morgan Date: Tue, 12 Nov 2013 12:57:47 +1100 Subject: [PATCH 1/4] Fix issue #410: DecryptingPartInputStream could return more data than requested, causing segfaults in BitmapFactory on Android 4.4. --- .../crypto/DecryptingPartInputStream.java | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java index 860f424673..5c6a4d72c2 100644 --- a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java +++ b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java @@ -24,6 +24,7 @@ import java.security.InvalidAlgorithmParameterException; import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; import java.util.Arrays; +import java.lang.System; import javax.crypto.BadPaddingException; import javax.crypto.Cipher; @@ -53,6 +54,7 @@ public class DecryptingPartInputStream extends FileInputStream { private boolean done; private long totalDataSize; private long totalRead; + private byte[] overflowBuffer; public DecryptingPartInputStream(File file, MasterSecret masterSecret) throws FileNotFoundException { super(file); @@ -122,6 +124,26 @@ public class DecryptingPartInputStream extends FileInputStream { } private int readIncremental(byte[] buffer, int offset, int length) throws IOException { + int readLength = 0; + //Use data from overflow buffer first if present + if (null != overflowBuffer) { + if (overflowBuffer.length > length) { + System.arraycopy(overflowBuffer, 0, buffer, offset, length); + overflowBuffer = Arrays.copyOfRange(overflowBuffer, length, overflowBuffer.length); + return length; + } else if (overflowBuffer.length == length) { + System.arraycopy(overflowBuffer, 0, buffer, offset, length); + overflowBuffer = null; + return length; + } else { + System.arraycopy(overflowBuffer, 0, buffer, offset, overflowBuffer.length); + readLength += overflowBuffer.length; + offset += readLength; + length -= readLength; + overflowBuffer = null; + } + } + if (length + totalRead > totalDataSize) length = (int)(totalDataSize - totalRead); @@ -131,7 +153,20 @@ public class DecryptingPartInputStream extends FileInputStream { try { mac.update(internalBuffer, 0, read); - return cipher.update(internalBuffer, 0, read, buffer, offset); + + //data retrieved using cipher.update doesn't always match cipher.getOutputSize (but should never be larger) + int outputLen = cipher.getOutputSize(read); + byte[] transientBuffer = new byte[outputLen]; + outputLen = cipher.update(internalBuffer, 0, read, transientBuffer, 0); + if (outputLen <= length) { + System.arraycopy(transientBuffer, 0, buffer, offset, outputLen); + readLength += outputLen; + } else { + System.arraycopy(transientBuffer, 0, buffer, offset, length); + overflowBuffer = Arrays.copyOfRange(transientBuffer, length, outputLen); + readLength += length; + } + return readLength; } catch (ShortBufferException e) { throw new AssertionError(e); } From 3cc6344c8b8e8e4baf723c8399a901f6d891ac13 Mon Sep 17 00:00:00 2001 From: Simeon Morgan Date: Tue, 12 Nov 2013 13:31:30 +1100 Subject: [PATCH 2/4] Optimised use of buffers when decrypting to avoid unnecessary array copying --- .../securesms/crypto/DecryptingPartInputStream.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java index 5c6a4d72c2..55549a298b 100644 --- a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java +++ b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java @@ -156,6 +156,12 @@ public class DecryptingPartInputStream extends FileInputStream { //data retrieved using cipher.update doesn't always match cipher.getOutputSize (but should never be larger) int outputLen = cipher.getOutputSize(read); + + if (outputLen < length) { + readLength += cipher.update(internalBuffer, 0, read, buffer, offset); + return readLength; + } + byte[] transientBuffer = new byte[outputLen]; outputLen = cipher.update(internalBuffer, 0, read, transientBuffer, 0); if (outputLen <= length) { From adfa3c1b106f9e3939495774909c503ed22b3243 Mon Sep 17 00:00:00 2001 From: Simeon Morgan Date: Tue, 12 Nov 2013 13:37:57 +1100 Subject: [PATCH 3/4] Optimised use of buffers when decrypting to avoid unnecessary array copying properly. --- .../securesms/crypto/DecryptingPartInputStream.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java index 55549a298b..a8fc15c552 100644 --- a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java +++ b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java @@ -157,7 +157,7 @@ public class DecryptingPartInputStream extends FileInputStream { //data retrieved using cipher.update doesn't always match cipher.getOutputSize (but should never be larger) int outputLen = cipher.getOutputSize(read); - if (outputLen < length) { + if (outputLen <= length) { readLength += cipher.update(internalBuffer, 0, read, buffer, offset); return readLength; } From 4931d7327b1f89deb9893d2e8c9ba80b08ff98fd Mon Sep 17 00:00:00 2001 From: Simeon Morgan Date: Thu, 14 Nov 2013 17:53:38 +1100 Subject: [PATCH 4/4] Switch to two-space indentation, remove inline comments. --- .../crypto/DecryptingPartInputStream.java | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java index a8fc15c552..82e7805339 100644 --- a/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java +++ b/src/org/thoughtcrime/securesms/crypto/DecryptingPartInputStream.java @@ -125,23 +125,22 @@ public class DecryptingPartInputStream extends FileInputStream { private int readIncremental(byte[] buffer, int offset, int length) throws IOException { int readLength = 0; - //Use data from overflow buffer first if present if (null != overflowBuffer) { - if (overflowBuffer.length > length) { - System.arraycopy(overflowBuffer, 0, buffer, offset, length); - overflowBuffer = Arrays.copyOfRange(overflowBuffer, length, overflowBuffer.length); - return length; - } else if (overflowBuffer.length == length) { - System.arraycopy(overflowBuffer, 0, buffer, offset, length); - overflowBuffer = null; - return length; - } else { - System.arraycopy(overflowBuffer, 0, buffer, offset, overflowBuffer.length); - readLength += overflowBuffer.length; - offset += readLength; - length -= readLength; - overflowBuffer = null; - } + if (overflowBuffer.length > length) { + System.arraycopy(overflowBuffer, 0, buffer, offset, length); + overflowBuffer = Arrays.copyOfRange(overflowBuffer, length, overflowBuffer.length); + return length; + } else if (overflowBuffer.length == length) { + System.arraycopy(overflowBuffer, 0, buffer, offset, length); + overflowBuffer = null; + return length; + } else { + System.arraycopy(overflowBuffer, 0, buffer, offset, overflowBuffer.length); + readLength += overflowBuffer.length; + offset += readLength; + length -= readLength; + overflowBuffer = null; + } } if (length + totalRead > totalDataSize) @@ -154,7 +153,6 @@ public class DecryptingPartInputStream extends FileInputStream { try { mac.update(internalBuffer, 0, read); - //data retrieved using cipher.update doesn't always match cipher.getOutputSize (but should never be larger) int outputLen = cipher.getOutputSize(read); if (outputLen <= length) {