More rigid detection of multipart/signed and /encrypted structures

k9mail-library/src/main/java/com/fsck/k9/mail/internet/MimeBodyPart.java

@@ -30,10 +30,10 @@ public class MimeBodyPart extends BodyPart {
         this(body, null);
     }
 
-    public MimeBodyPart(Body body, String mimeType) throws MessagingException {
+    public MimeBodyPart(Body body, String contentType) throws MessagingException {
         mHeader = new MimeHeader();
-        if (mimeType != null) {
+        if (contentType != null) {
-            addHeader(MimeHeader.HEADER_CONTENT_TYPE, mimeType);
+            addHeader(MimeHeader.HEADER_CONTENT_TYPE, contentType);
         }
         MimeMessageHelper.setBody(this, body);
     }

k9mail/src/main/java/com/fsck/k9/crypto/MessageDecryptVerifier.java

@@ -18,6 +18,7 @@ import com.fsck.k9.mail.Multipart;
 import com.fsck.k9.mail.Part;
 import com.fsck.k9.mail.internet.MessageExtractor;
 import com.fsck.k9.mail.internet.MimeBodyPart;
+import com.fsck.k9.mail.internet.MimeMultipart;
 import com.fsck.k9.mail.internet.MimeUtility;
 import com.fsck.k9.mailstore.CryptoResultAnnotation;
 import com.fsck.k9.ui.crypto.MessageCryptoAnnotations;
@@ -214,24 +215,55 @@ public class MessageDecryptVerifier {
     }
 
     private static boolean isPartMultipartSigned(Part part) {
-        return isSameMimeType(part.getMimeType(), MULTIPART_SIGNED);
+        if (!isSameMimeType(part.getMimeType(), MULTIPART_SIGNED)) {
+            return false;
+        }
+        if (! (part.getBody() instanceof MimeMultipart)) {
+            return false;
+        }
+        MimeMultipart mimeMultipart = (MimeMultipart) part.getBody();
+        if (mimeMultipart.getCount() != 2) {
+            return false;
+        }
+
+        String protocolParameter = MimeUtility.getHeaderParameter(part.getContentType(), PROTOCOL_PARAMETER);
+        BodyPart signatureBodyPart = mimeMultipart.getBodyPart(1);
+        return isSameMimeType(protocolParameter, signatureBodyPart.getMimeType());
     }
 
     private static boolean isPartMultipartEncrypted(Part part) {
-        return isSameMimeType(part.getMimeType(), MULTIPART_ENCRYPTED);
+        if (!isSameMimeType(part.getMimeType(), MULTIPART_ENCRYPTED)) {
+            return false;
+        }
+        if (! (part.getBody() instanceof MimeMultipart)) {
+            return false;
+        }
+        MimeMultipart mimeMultipart = (MimeMultipart) part.getBody();
+        if (mimeMultipart.getCount() != 2) {
+            return false;
+        }
+
+        String protocolParameter = MimeUtility.getHeaderParameter(part.getContentType(), PROTOCOL_PARAMETER);
+        BodyPart signatureBodyPart = mimeMultipart.getBodyPart(0);
+        return isSameMimeType(protocolParameter, signatureBodyPart.getMimeType());
     }
 
-    // TODO also guess by mime-type of contained part?
+    public static boolean isMultipartEncryptedOpenPgpProtocol(Part part) {
-    public static boolean isPgpMimeEncryptedOrSignedPart(Part part) {
+        if (!isSameMimeType(part.getMimeType(), MULTIPART_ENCRYPTED)) {
-        String contentType = part.getContentType();
+            throw new IllegalArgumentException("Part is not multipart/encrypted!");
-        String protocolParameter = MimeUtility.getHeaderParameter(contentType, PROTOCOL_PARAMETER);
+        }
 
-        boolean isPgpEncrypted = isSameMimeType(part.getMimeType(), MULTIPART_ENCRYPTED) &&
+        String protocolParameter = MimeUtility.getHeaderParameter(part.getContentType(), PROTOCOL_PARAMETER);
-                APPLICATION_PGP_ENCRYPTED.equalsIgnoreCase(protocolParameter);
+        return APPLICATION_PGP_ENCRYPTED.equalsIgnoreCase(protocolParameter);
-        boolean isPgpSigned = isSameMimeType(part.getMimeType(), MULTIPART_SIGNED) &&
+    }
-                APPLICATION_PGP_SIGNATURE.equalsIgnoreCase(protocolParameter);
 
-        return isPgpEncrypted || isPgpSigned;
+    public static boolean isMultipartSignedOpenPgpProtocol(Part part) {
+        if (!isSameMimeType(part.getMimeType(), MULTIPART_SIGNED)) {
+            throw new IllegalArgumentException("Part is not multipart/signed!");
+        }
+
+        String protocolParameter = MimeUtility.getHeaderParameter(part.getContentType(), PROTOCOL_PARAMETER);
+        return APPLICATION_PGP_SIGNATURE.equalsIgnoreCase(protocolParameter);
     }
 
     @VisibleForTesting

k9mail/src/main/java/com/fsck/k9/ui/crypto/MessageCryptoHelper.java

@@ -154,7 +154,7 @@ public class MessageCryptoHelper {
                 addErrorAnnotation(part, CryptoError.OPENPGP_ENCRYPTED_BUT_INCOMPLETE, MessageHelper.createEmptyPart());
                 continue;
             }
-            if (MessageDecryptVerifier.isPgpMimeEncryptedOrSignedPart(part)) {
+            if (MessageDecryptVerifier.isMultipartEncryptedOpenPgpProtocol(part)) {
                 CryptoPart cryptoPart = new CryptoPart(CryptoPartType.PGP_ENCRYPTED, part);
                 partsToProcess.add(cryptoPart);
                 continue;
@@ -170,7 +170,7 @@ public class MessageCryptoHelper {
                 addErrorAnnotation(part, CryptoError.OPENPGP_SIGNED_BUT_INCOMPLETE, replacementPart);
                 continue;
             }
-            if (MessageDecryptVerifier.isPgpMimeEncryptedOrSignedPart(part)) {
+            if (MessageDecryptVerifier.isMultipartSignedOpenPgpProtocol(part)) {
                 CryptoPart cryptoPart = new CryptoPart(CryptoPartType.PGP_SIGNED, part);
                 partsToProcess.add(cryptoPart);
                 continue;

k9mail/src/test/java/com/fsck/k9/crypto/MessageDecryptVerifierTest.java

@@ -29,13 +29,12 @@ import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.mock;
 
 
+@SuppressWarnings("WeakerAccess")
 @RunWith(K9RobolectricTestRunner.class)
 @Config(manifest = Config.NONE)
 public class MessageDecryptVerifierTest {
-    private static final String MIME_TYPE_MULTIPART_ENCRYPTED = "multipart/encrypted";
+    MessageCryptoAnnotations messageCryptoAnnotations = mock(MessageCryptoAnnotations.class);
-    private MessageCryptoAnnotations messageCryptoAnnotations = mock(MessageCryptoAnnotations.class);
+    static final String PGP_INLINE_DATA = "" +
-    private static final String PROTCOL_PGP_ENCRYPTED = "application/pgp-encrypted";
-    private static final String PGP_INLINE_DATA = "" +
             "-----BEGIN PGP MESSAGE-----\n" +
             "Header: Value\n" +
             "\n" +
@@ -59,8 +58,8 @@ public class MessageDecryptVerifierTest {
         List<Part> outputExtraParts = new ArrayList<>();
         BodyPart pgpInlinePart = bodypart("text/plain", PGP_INLINE_DATA);
         Message message = messageFromBody(
-                multipart("alternative",
+                multipart("alternative", null,
-                        pgpInlinePart,
+                    pgpInlinePart,
                         bodypart("text/html")
                 )
         );
@@ -75,7 +74,7 @@ public class MessageDecryptVerifierTest {
         List<Part> outputExtraParts = new ArrayList<>();
         BodyPart pgpInlinePart = bodypart("text/plain", PGP_INLINE_DATA);
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
                         pgpInlinePart,
                         bodypart("application/octet-stream")
                 )
@@ -92,8 +91,8 @@ public class MessageDecryptVerifierTest {
         List<Part> outputExtraParts = new ArrayList<>();
         BodyPart pgpInlinePart = bodypart("text/plain", PGP_INLINE_DATA);
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
-                        multipart("alternative",
+                        multipart("alternative", null,
                             pgpInlinePart,
                             bodypart("text/html")
                         ),
@@ -110,7 +109,7 @@ public class MessageDecryptVerifierTest {
     public void findPrimaryCryptoPart_withEmptyMultipartAlternative_shouldReturnNull() throws Exception {
         List<Part> outputExtraParts = new ArrayList<>();
         Message message = messageFromBody(
-                multipart("alternative")
+                multipart("alternative", null)
         );
 
         Part cryptoPart = MessageDecryptVerifier.findPrimaryEncryptedOrSignedPart(message, outputExtraParts);
@@ -122,7 +121,7 @@ public class MessageDecryptVerifierTest {
     public void findPrimaryCryptoPart_withEmptyMultipartMixed_shouldReturnNull() throws Exception {
         List<Part> outputExtraParts = new ArrayList<>();
         Message message = messageFromBody(
-                multipart("mixed")
+                multipart("mixed", null)
         );
 
         Part cryptoPart = MessageDecryptVerifier.findPrimaryEncryptedOrSignedPart(message, outputExtraParts);
@@ -135,8 +134,8 @@ public class MessageDecryptVerifierTest {
             throws Exception {
         List<Part> outputExtraParts = new ArrayList<>();
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
-                        multipart("alternative")
+                        multipart("alternative", null)
                 )
         );
 
@@ -164,53 +163,10 @@ public class MessageDecryptVerifierTest {
         assertEquals(0, encryptedParts.size());
     }
 
-    @Test
-    public void findEncryptedPartsShouldReturnEmptyEncryptedPart() throws Exception {
-        MimeMessage message = new MimeMessage();
-        MimeMultipart multipartEncrypted = MimeMultipart.newInstance();
-        multipartEncrypted.setSubType("encrypted");
-        MimeMessageHelper.setBody(message, multipartEncrypted);
-        setContentTypeWithProtocol(message, MIME_TYPE_MULTIPART_ENCRYPTED, PROTCOL_PGP_ENCRYPTED);
-
-        List<Part> encryptedParts = MessageDecryptVerifier.findEncryptedParts(message);
-
-        assertEquals(1, encryptedParts.size());
-        assertSame(message, encryptedParts.get(0));
-    }
-
-    @Test
-    public void findEncryptedPartsShouldReturnMultipleEncryptedParts() throws Exception {
-        MimeMessage message = new MimeMessage();
-        MimeMultipart multipartMixed = MimeMultipart.newInstance();
-        multipartMixed.setSubType("mixed");
-        MimeMessageHelper.setBody(message, multipartMixed);
-
-        MimeMultipart multipartEncryptedOne = MimeMultipart.newInstance();
-        multipartEncryptedOne.setSubType("encrypted");
-        MimeBodyPart bodyPartOne = new MimeBodyPart(multipartEncryptedOne);
-        setContentTypeWithProtocol(bodyPartOne, MIME_TYPE_MULTIPART_ENCRYPTED, PROTCOL_PGP_ENCRYPTED);
-        multipartMixed.addBodyPart(bodyPartOne);
-
-        MimeBodyPart bodyPartTwo = new MimeBodyPart(null, "text/plain");
-        multipartMixed.addBodyPart(bodyPartTwo);
-
-        MimeMultipart multipartEncryptedThree = MimeMultipart.newInstance();
-        multipartEncryptedThree.setSubType("encrypted");
-        MimeBodyPart bodyPartThree = new MimeBodyPart(multipartEncryptedThree);
-        setContentTypeWithProtocol(bodyPartThree, MIME_TYPE_MULTIPART_ENCRYPTED, PROTCOL_PGP_ENCRYPTED);
-        multipartMixed.addBodyPart(bodyPartThree);
-
-        List<Part> encryptedParts = MessageDecryptVerifier.findEncryptedParts(message);
-
-        assertEquals(2, encryptedParts.size());
-        assertSame(bodyPartOne, encryptedParts.get(0));
-        assertSame(bodyPartThree, encryptedParts.get(1));
-    }
-
     @Test
     public void findEncrypted__withMultipartEncrypted__shouldReturnRoot() throws Exception {
         Message message = messageFromBody(
-                multipart("encrypted",
+                multipart("encrypted", "application/pgp-encrypted",
                         bodypart("application/pgp-encrypted"),
                         bodypart("application/octet-stream")
                 )
@@ -222,11 +178,65 @@ public class MessageDecryptVerifierTest {
         assertSame(message, encryptedParts.get(0));
     }
 
+    @Test
+    public void findEncrypted__withBadProtocol__shouldReturnEmpty() throws Exception {
+        Message message = messageFromBody(
+                multipart("encrypted", "application/not-pgp-encrypted",
+                        bodypart("application/pgp-encrypted"),
+                        bodypart("application/octet-stream")
+                )
+        );
+
+        List<Part> encryptedParts = MessageDecryptVerifier.findEncryptedParts(message);
+
+        assertTrue(encryptedParts.isEmpty());
+    }
+
+    @Test
+    public void findEncrypted__withEmptyProtocol__shouldReturnEmpty() throws Exception {
+        Message message = messageFromBody(
+                multipart("encrypted", null,
+                        bodypart("application/pgp-encrypted"),
+                        bodypart("application/octet-stream")
+                )
+        );
+
+        List<Part> encryptedParts = MessageCryptoStructureDetector.findMultipartEncryptedParts(message);
+
+        assertTrue(encryptedParts.isEmpty());
+    }
+
+    @Test
+    public void findEncrypted__withMissingEncryptedBody__shouldReturnEmpty() throws Exception {
+        Message message = messageFromBody(
+                multipart("encrypted", "application/pgp-encrypted",
+                        bodypart("application/pgp-encrypted")
+                )
+        );
+
+        List<Part> encryptedParts = MessageDecryptVerifier.findEncryptedParts(message);
+
+        assertTrue(encryptedParts.isEmpty());
+    }
+
+    @Test
+    public void findEncrypted__withBadStructure__shouldReturnEmpty() throws Exception {
+        Message message = messageFromBody(
+                multipart("encrypted", "application/pgp-encrypted",
+                        bodypart("application/octet-stream")
+                )
+        );
+
+        List<Part> encryptedParts = MessageDecryptVerifier.findEncryptedParts(message);
+
+        assertTrue(encryptedParts.isEmpty());
+    }
+
     @Test
     public void findEncrypted__withMultipartMixedSubEncrypted__shouldReturnRoot() throws Exception {
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
-                        multipart("encrypted",
+                        multipart("encrypted", "application/pgp-encrypted",
                             bodypart("application/pgp-encrypted"),
                             bodypart("application/octet-stream")
                         )
@@ -243,12 +253,12 @@ public class MessageDecryptVerifierTest {
     public void findEncrypted__withMultipartMixedSubEncryptedAndEncrypted__shouldReturnBoth()
             throws Exception {
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
-                        multipart("encrypted",
+                        multipart("encrypted", "application/pgp-encrypted",
                                 bodypart("application/pgp-encrypted"),
                                 bodypart("application/octet-stream")
                         ),
-                        multipart("encrypted",
+                        multipart("encrypted", "application/pgp-encrypted",
                                 bodypart("application/pgp-encrypted"),
                                 bodypart("application/octet-stream")
                         )
@@ -265,9 +275,9 @@ public class MessageDecryptVerifierTest {
     @Test
     public void findEncrypted__withMultipartMixedSubTextAndEncrypted__shouldReturnEncrypted() throws Exception {
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
                         bodypart("text/plain"),
-                        multipart("encrypted",
+                        multipart("encrypted", "application/pgp-encrypted",
                                 bodypart("application/pgp-encrypted"),
                                 bodypart("application/octet-stream")
                         )
@@ -283,8 +293,8 @@ public class MessageDecryptVerifierTest {
     @Test
     public void findEncrypted__withMultipartMixedSubEncryptedAndText__shouldReturnEncrypted() throws Exception {
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
-                        multipart("encrypted",
+                        multipart("encrypted", "application/pgp-encrypted",
                                 bodypart("application/pgp-encrypted"),
                                 bodypart("application/octet-stream")
                         ),
@@ -301,7 +311,7 @@ public class MessageDecryptVerifierTest {
     @Test
     public void findSigned__withSimpleMultipartSigned__shouldReturnRoot() throws Exception {
         Message message = messageFromBody(
-                multipart("signed",
+                multipart("signed", "application/pgp-signature",
                         bodypart("text/plain"),
                         bodypart("application/pgp-signature")
                 )
@@ -313,11 +323,53 @@ public class MessageDecryptVerifierTest {
         assertSame(message, signedParts.get(0));
     }
 
+    @Test
+    public void findSigned__withBadProtocol__shouldReturnRoot() throws Exception {
+        Message message = messageFromBody(
+                multipart("signed", "application/not-pgp-signature",
+                        bodypart("text/plain"),
+                        bodypart("application/pgp-signature")
+                )
+        );
+
+        List<Part> signedParts = MessageDecryptVerifier.findSignedParts(message, messageCryptoAnnotations);
+
+        assertTrue(signedParts.isEmpty());
+    }
+
+    @Test
+    public void findSigned__withEmptyProtocol__shouldReturnRoot() throws Exception {
+        Message message = messageFromBody(
+                multipart("signed", null,
+                        bodypart("text/plain"),
+                        bodypart("application/pgp-signature")
+                )
+        );
+
+        List<Part> signedParts = MessageCryptoStructureDetector
+                .findMultipartSignedParts(message, messageCryptoAnnotations);
+
+        assertTrue(signedParts.isEmpty());
+    }
+
+    @Test
+    public void findSigned__withMissingSignature__shouldReturnEmpty() throws Exception {
+        Message message = messageFromBody(
+                multipart("signed", "application/pgp-signature",
+                        bodypart("text/plain")
+                )
+        );
+
+        List<Part> signedParts = MessageDecryptVerifier.findSignedParts(message, messageCryptoAnnotations);
+
+        assertTrue(signedParts.isEmpty());
+    }
+
     @Test
     public void findSigned__withComplexMultipartSigned__shouldReturnRoot() throws Exception {
         Message message = messageFromBody(
-                multipart("signed",
+                multipart("signed", "application/pgp-signature",
-                        multipart("mixed",
+                        multipart("mixed", null,
                                 bodypart("text/plain"),
                                 bodypart("application/pdf")
                         ),
@@ -334,10 +386,10 @@ public class MessageDecryptVerifierTest {
     @Test
     public void findEncrypted__withMultipartMixedSubSigned__shouldReturnSigned() throws Exception {
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
-                    multipart("signed",
+                        multipart("signed", "application/pgp-signature",
-                            bodypart("text/plain"),
+                                bodypart("text/plain"),
-                            bodypart("application/pgp-signature")
+                                bodypart("application/pgp-signature")
                     )
                 )
         );
@@ -351,8 +403,8 @@ public class MessageDecryptVerifierTest {
     @Test
     public void findEncrypted__withMultipartMixedSubSignedAndText__shouldReturnSigned() throws Exception {
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
-                        multipart("signed",
+                        multipart("signed", "application/pgp-signature",
                                 bodypart("text/plain"),
                                 bodypart("application/pgp-signature")
                         ),
@@ -369,9 +421,9 @@ public class MessageDecryptVerifierTest {
     @Test
     public void findEncrypted__withMultipartMixedSubTextAndSigned__shouldReturnSigned() throws Exception {
         Message message = messageFromBody(
-                multipart("mixed",
+                multipart("mixed", null,
                         bodypart("text/plain"),
-                        multipart("signed",
+                        multipart("signed", "application/pgp-signature",
                                 bodypart("text/plain"),
                                 bodypart("application/pgp-signature")
                         )
@@ -467,16 +519,24 @@ public class MessageDecryptVerifierTest {
     MimeMessage messageFromBody(BodyPart bodyPart) throws MessagingException {
         MimeMessage message = new MimeMessage();
         MimeMessageHelper.setBody(message, bodyPart.getBody());
+        if (bodyPart.getContentType() != null) {
+            message.setHeader("Content-Type", bodyPart.getContentType());
+        }
         return message;
     }
 
-    MimeBodyPart multipart(String type, BodyPart... subParts) throws MessagingException {
+    MimeBodyPart multipart(String type, String protocol, BodyPart... subParts) throws MessagingException {
         MimeMultipart multiPart = MimeMultipart.newInstance();
         multiPart.setSubType(type);
         for (BodyPart subPart : subParts) {
             multiPart.addBodyPart(subPart);
         }
-        return new MimeBodyPart(multiPart);
+        MimeBodyPart mimeBodyPart = new MimeBodyPart(multiPart);
+        if (protocol != null) {
+            mimeBodyPart.setHeader(MimeHeader.HEADER_CONTENT_TYPE,
+                    mimeBodyPart.getContentType() + "; protocol=\"" + protocol + "\"");
+        }
+        return mimeBodyPart;
     }
 
     BodyPart bodypart(String type) throws MessagingException {
@@ -488,17 +548,11 @@ public class MessageDecryptVerifierTest {
         return new MimeBodyPart(textBody, type);
     }
 
-    public static Part getPart(Part searchRootPart, int... indexes) {
+    static Part getPart(Part searchRootPart, int... indexes) {
         Part part = searchRootPart;
         for (int index : indexes) {
             part = ((Multipart) part.getBody()).getBodyPart(index);
         }
         return part;
     }
-
-    //TODO: Find a cleaner way to do this
-    private static void setContentTypeWithProtocol(Part part, String mimeType, String protocol)
-            throws MessagingException {
-        part.setHeader(MimeHeader.HEADER_CONTENT_TYPE, mimeType + "; protocol=\"" + protocol + "\"");
-    }
 }