be safer when processing parts in AttachmentDownloadJob

Closes #4166
// FREEBIE

src/org/thoughtcrime/securesms/crypto/MasterCipher.java

@@ -17,6 +17,7 @@
  */
 package org.thoughtcrime.securesms.crypto;
 
+import android.support.annotation.NonNull;
 import android.util.Log;
 
 import org.thoughtcrime.securesms.util.Base64;
@@ -92,7 +93,7 @@ public class MasterCipher {
     }
   }
 	
-  public byte[] decryptBytes(byte[] decodedBody) throws InvalidMessageException {
+  public byte[] decryptBytes(@NonNull byte[] decodedBody) throws InvalidMessageException {
     try {
       Mac mac              = getMac(masterSecret.getMacKey());
       byte[] encryptedBody = verifyMacBody(mac, decodedBody);
@@ -103,7 +104,7 @@ public class MasterCipher {
       return encrypted;
     } catch (GeneralSecurityException ge) {
       throw new InvalidMessageException(ge);
-    }		
+    }
   }
 	
   public byte[] encryptBytes(byte[] body) {
@@ -153,7 +154,11 @@ public class MasterCipher {
     return Base64.encodeBytes(encryptedAndMacBody);
   }
 	
-  private byte[] verifyMacBody(Mac hmac, byte[] encryptedAndMac) throws InvalidMessageException {		
+  private byte[] verifyMacBody(@NonNull Mac hmac, @NonNull byte[] encryptedAndMac) throws InvalidMessageException {
+    if (encryptedAndMac.length < hmac.getMacLength()) {
+      throw new InvalidMessageException("length(encrypted body + MAC) < length(MAC)");
+    }
+
     byte[] encrypted = new byte[encryptedAndMac.length - hmac.getMacLength()];
     System.arraycopy(encryptedAndMac, 0, encrypted, 0, encrypted.length);
 		

src/org/thoughtcrime/securesms/jobs/AttachmentDownloadJob.java

@@ -15,6 +15,7 @@ import org.thoughtcrime.securesms.jobs.requirements.MasterSecretRequirement;
 import org.thoughtcrime.securesms.jobs.requirements.MediaNetworkRequirement;
 import org.thoughtcrime.securesms.notifications.MessageNotifier;
 import org.thoughtcrime.securesms.util.Util;
+import org.thoughtcrime.securesms.util.VisibleForTesting;
 import org.whispersystems.jobqueue.JobParameters;
 import org.whispersystems.jobqueue.requirements.NetworkRequirement;
 import org.whispersystems.libaxolotl.InvalidMessageException;
@@ -122,10 +123,16 @@ public class AttachmentDownloadJob extends MasterSecretJob implements Injectable
     }
   }
 
-  private TextSecureAttachmentPointer createAttachmentPointer(MasterSecret masterSecret, PduPart part)
+  @VisibleForTesting
+  TextSecureAttachmentPointer createAttachmentPointer(MasterSecret masterSecret, PduPart part)
       throws InvalidPartException
   {
-    if (part.getContentLocation() == null) throw new InvalidPartException("null content location");
+    if (part.getContentLocation() == null || part.getContentLocation().length == 0) {
+      throw new InvalidPartException("empty content id");
+    }
+    if (part.getContentDisposition() == null || part.getContentDisposition().length == 0) {
+      throw new InvalidPartException("empty encrypted key");
+    }
 
     try {
       AsymmetricMasterSecret asymmetricMasterSecret = MasterSecretUtil.getAsymmetricMasterSecret(context, masterSecret);
@@ -164,7 +171,7 @@ public class AttachmentDownloadJob extends MasterSecretJob implements Injectable
     }
   }
 
-  private static class InvalidPartException extends Exception {
+  @VisibleForTesting static class InvalidPartException extends Exception {
     public InvalidPartException(String s) {super(s);}
     public InvalidPartException(Exception e) {super(e);}
   }

test/unitTest/java/org/thoughtcrime/securesms/BaseUnitTest.java

@@ -1,5 +1,6 @@
 package org.thoughtcrime.securesms;
 
+import android.content.Context;
 import android.os.Handler;
 import android.os.Looper;
 import android.util.Log;
@@ -11,15 +12,25 @@ import org.mockito.stubbing.Answer;
 import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
+import org.thoughtcrime.securesms.crypto.MasterSecret;
+
+import javax.crypto.spec.SecretKeySpec;
 
 import static org.mockito.Matchers.anyString;
+import static org.powermock.api.mockito.PowerMockito.mock;
 import static org.powermock.api.mockito.PowerMockito.mockStatic;
 
 @RunWith(PowerMockRunner.class)
-@PrepareForTest( { Log.class, Handler.class, Looper.class })
+@PrepareForTest({ Log.class, Handler.class, Looper.class })
 public abstract class BaseUnitTest {
+  protected Context      context;
+  protected MasterSecret masterSecret;
+
   @Before
   public void setUp() throws Exception {
+    context      = mock(Context.class);
+    masterSecret = new MasterSecret(new SecretKeySpec(new byte[16], "AES"),
+                                    new SecretKeySpec(new byte[16], "HmacSHA1"));
     mockStatic(Looper.class);
     mockStatic(Log.class);
     mockStatic(Handler.class);

test/unitTest/java/org/thoughtcrime/securesms/crypto/MasterCipherTest.java

@@ -0,0 +1,24 @@
+package org.thoughtcrime.securesms.crypto;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.powermock.core.classloader.annotations.PowerMockIgnore;
+import org.thoughtcrime.securesms.BaseUnitTest;
+import org.whispersystems.libaxolotl.InvalidMessageException;
+
+@PowerMockIgnore("javax.crypto.*")
+public class MasterCipherTest extends BaseUnitTest {
+  private MasterCipher masterCipher;
+
+  @Before
+  @Override
+  public void setUp() throws Exception {
+    super.setUp();
+    masterCipher = new MasterCipher(masterSecret);
+  }
+
+  @Test(expected = InvalidMessageException.class)
+  public void testEncryptBytesWithZeroBody() throws Exception {
+    masterCipher.decryptBytes(new byte[]{});
+  }
+}

test/unitTest/java/org/thoughtcrime/securesms/jobs/AttachmentDownloadJobTest.java

@@ -0,0 +1,39 @@
+package org.thoughtcrime.securesms.jobs;
+
+import android.content.Context;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.thoughtcrime.securesms.BaseUnitTest;
+import org.thoughtcrime.securesms.database.PartDatabase.PartId;
+import org.thoughtcrime.securesms.jobs.AttachmentDownloadJob.InvalidPartException;
+import org.thoughtcrime.securesms.util.Util;
+
+import ws.com.google.android.mms.pdu.PduPart;
+
+import static org.powermock.api.mockito.PowerMockito.mock;
+
+public class AttachmentDownloadJobTest extends BaseUnitTest {
+  private AttachmentDownloadJob job;
+
+  @Before
+  @Override
+  public void setUp() throws Exception {
+    super.setUp();
+    job = new AttachmentDownloadJob(mock(Context.class), 1L, new PartId(1L, 1L));
+  }
+
+  @Test(expected = InvalidPartException.class)
+  public void testCreateAttachmentPointerInvalidId() throws Exception {
+    PduPart part = new PduPart();
+    part.setContentDisposition(Util.toIsoBytes("a long and acceptable valid key like we all want"));
+    job.createAttachmentPointer(masterSecret, part);
+  }
+
+  @Test(expected = InvalidPartException.class)
+  public void testCreateAttachmentPointerInvalidKey() throws Exception {
+    PduPart part = new PduPart();
+    part.setContentDisposition(Util.toIsoBytes("1"));
+    job.createAttachmentPointer(masterSecret, part);
+  }
+}