#2432 Fix: error when decrypting pgpmime with inline image

Core/source/core/att.ts

@@ -3,6 +3,7 @@
 'use strict';
 
 import { Buf } from './buf';
+import { Str } from './common';
 
 type Att$treatAs = 'publicKey' | 'privateKey' | 'encryptedMsg' | 'hidden' | 'signature' | 'encryptedFile' | 'plainFile';
 export type AttMeta = {
@@ -88,7 +89,7 @@ export class Att {
     throw new Error('Att has no data set');
   };
 
-  public treatAs = (): Att$treatAs => {
+  public treatAs = (attachments: Att[], isBodyEmpty = false): Att$treatAs => {
     if (this.treatAsValue) {
       // pre-set
       return this.treatAsValue;
@@ -99,6 +100,21 @@ export class Att {
     ) {
       return 'hidden'; // PGPexch.htm.pgp is html alternative of textual body content produced by PGP Desktop and GPG4o
     } else if (this.name === 'signature.asc' || this.type === 'application/pgp-signature') {
+      // this may be a signature for an attachment following these patterns:
+      // sample.name.sig for sample.name.pgp #3448
+      // or sample.name.sig for sample.name
+      if (attachments.length > 1) {
+        const nameWithoutExtension = Str.getFilenameWithoutExtension(this.name);
+        if (
+          attachments.some(
+            a =>
+              a !== this &&
+              (a.name === nameWithoutExtension || Str.getFilenameWithoutExtension(a.name) === nameWithoutExtension),
+          )
+        ) {
+          return 'hidden';
+        }
+      }
       return 'signature';
     } else if (!this.name && !this.type.startsWith('image/')) {
       // this.name may be '' or undefined - catch either
@@ -110,6 +126,9 @@ export class Att {
       ['message', 'msg.asc', 'message.asc', 'encrypted.asc', 'encrypted.eml.pgp', 'Message.pgp'].includes(this.name)
     ) {
       return 'encryptedMsg';
+    } else if (this.name === 'message' && isBodyEmpty) {
+      // treat message as encryptedMsg when empty body for the 'message' attachment
+      return 'encryptedMsg';
     } else if (this.name.match(/(\.pgp$)|(\.gpg$)|(\.[a-zA-Z0-9]{3,4}\.asc$)/g)) {
       // ends with one of .gpg, .pgp, .???.asc, .????.asc
       return 'encryptedFile';

Core/source/core/common.ts

@@ -53,6 +53,10 @@ export class Str {
     return str.replace(/^—–|—–$/gm, '-----');
   };
 
+  public static getFilenameWithoutExtension = (filename: string): string => {
+    return filename.replace(/\.[^/.]+$/, '');
+  };
+
   public static normalize = (str: string) => {
     return Str.normalizeSpaces(Str.normalizeDashes(str));
   };

Core/source/core/mime.ts

@@ -50,16 +50,14 @@ export type MimeProccesedMsg = {
 type SendingType = 'to' | 'cc' | 'bcc';
 
 export class Mime {
-  public static processDecoded = (decoded: MimeContent): MimeProccesedMsg => {
+  public static processBody = (decoded: MimeContent): MsgBlock[] => {
     const blocks: MsgBlock[] = [];
     if (decoded.text) {
-      const blocksFromTextPart = MsgBlockParser.detectBlocks(Str.normalize(decoded.text)).blocks;
+      const blocksFromTextPart = MsgBlockParser.detectBlocks(Str.normalize(decoded.text), true).blocks;
       // if there are some encryption-related blocks found in the text section,
       // which we can use, and not look at the html section
       if (
-        blocksFromTextPart.find(
-          b => b.type === 'encryptedMsg' || b.type === 'signedMsg' || b.type === 'publicKey' || b.type === 'privateKey',
-        )
+        blocksFromTextPart.find(b => ['pkcs7', 'encryptedMsg', 'signedMsg', 'publicKey', 'privateKey'].includes(b.type))
       ) {
         // because the html most likely containt the same thing,
         // just harder to parse pgp sections cause it's html
@@ -74,21 +72,42 @@ export class Mime {
     } else if (decoded.html) {
       blocks.push(MsgBlock.fromContent('plainHtml', decoded.html));
     }
+    return blocks;
+  };
+
+  public static isBodyEmpty = ({ text, html }: MimeContent) => {
+    return Mime.isBodyTextEmpty(text) && Mime.isBodyTextEmpty(html);
+  };
+
+  public static isBodyTextEmpty = (text: string | undefined) => {
+    return !(text && !/^(\r)?(\n)?$/.test(text));
+  };
+
+  public static processAttachments = (bodyBlocks: MsgBlock[], decoded: MimeContent): MimeProccesedMsg => {
+    const attachmentBlocks: MsgBlock[] = [];
+    const signatureAttachments: Att[] = [];
     for (const file of decoded.atts) {
-      const treatAs = file.treatAs();
+      let treatAs = file.treatAs(decoded.atts, Mime.isBodyEmpty(decoded));
+      if (['needChunk', 'maybePgp'].includes(treatAs)) {
+        // todo: attachments from MimeContent always have data set (so 'needChunk' should never happen),
+        // and we can perform whatever analysis is needed based on the actual data,
+        // but we don't want to reference MsgUtil and OpenPGP.js from this class,
+        // so I suggest to move this method to MessageRenderer for further refactoring
+        treatAs = 'encryptedMsg'; // publicKey?
+      }
       if (treatAs === 'encryptedMsg') {
         const armored = PgpArmor.clip(file.getData().toUtfStr());
         if (armored) {
-          blocks.push(MsgBlock.fromContent('encryptedMsg', armored));
+          attachmentBlocks.push(MsgBlock.fromContent('encryptedMsg', armored));
         }
       } else if (treatAs === 'signature') {
-        decoded.signature = decoded.signature || file.getData().toUtfStr();
+        signatureAttachments.push(file);
       } else if (treatAs === 'publicKey') {
-        blocks.push(...MsgBlockParser.detectBlocks(file.getData().toUtfStr()).blocks);
+        attachmentBlocks.push(...MsgBlockParser.detectBlocks(file.getData().toUtfStr(), true).blocks);
       } else if (treatAs === 'privateKey') {
-        blocks.push(...MsgBlockParser.detectBlocks(file.getData().toUtfStr()).blocks);
+        attachmentBlocks.push(...MsgBlockParser.detectBlocks(file.getData().toUtfStr(), true).blocks);
       } else if (treatAs === 'encryptedFile') {
-        blocks.push(
+        attachmentBlocks.push(
           MsgBlock.fromAtt('encryptedAtt', '', {
             name: file.name,
             type: file.type,
@@ -97,7 +116,7 @@ export class Mime {
           }),
         );
       } else if (treatAs === 'plainFile') {
-        blocks.push(
+        attachmentBlocks.push(
           MsgBlock.fromAtt('plainAtt', '', {
             name: file.name,
             type: file.type,
@@ -109,7 +128,25 @@ export class Mime {
         );
       }
     }
-    if (decoded.signature) {
+    if (signatureAttachments.length) {
+      // todo: if multiple signatures, figure out which fits what
+      // attachments from MimeContent always have data set
+      const signature = signatureAttachments[0].getData().toUtfStr();
+      if (
+        ![...bodyBlocks, ...attachmentBlocks].some(block =>
+          ['plainText', 'plainHtml', 'signedMsg'].includes(block.type),
+        )
+      ) {
+        // signed an empty message
+        attachmentBlocks.push(new MsgBlock('signedMsg', '', true, signature));
+      }
+    }
+    const blocks = [...bodyBlocks, ...attachmentBlocks];
+    if (
+      decoded.signature &&
+      decoded.signature.includes(PgpArmor.ARMOR_HEADER_DICT.signature.begin) &&
+      decoded.signature.includes(String(PgpArmor.ARMOR_HEADER_DICT.signature.end))
+    ) {
       for (const block of blocks) {
         if (block.type === 'plainText') {
           block.type = 'signedMsg';
@@ -141,6 +178,11 @@ export class Mime {
     };
   };
 
+  public static processDecoded = (decoded: MimeContent): MimeProccesedMsg => {
+    const blocks = Mime.processBody(decoded);
+    return Mime.processAttachments(blocks, decoded);
+  };
+
   public static process = async (mimeMsg: Uint8Array): Promise<MimeProccesedMsg> => {
     const decoded = await Mime.decode(mimeMsg);
     return Mime.processDecoded(decoded);

Core/source/core/msg-block-parser.ts

@@ -19,12 +19,12 @@ export class MsgBlockParser {
   // eslint-disable-next-line @typescript-eslint/naming-convention
   private static ARMOR_HEADER_MAX_LENGTH = 50;
 
-  public static detectBlocks = (origText: string) => {
+  public static detectBlocks = (origText: string, completeOnly?: boolean) => {
     const blocks: MsgBlock[] = [];
     const normalized = Str.normalize(origText);
     let startAt = 0;
     while (true) {
-      const r = MsgBlockParser.detectBlockNext(normalized, startAt);
+      const r = MsgBlockParser.detectBlockNext(normalized, startAt, completeOnly);
       if (r.found) {
         blocks.push(...r.found);
       }
@@ -76,7 +76,7 @@ export class MsgBlockParser {
       blocks.push(); // escaped mime text as html
     }
     for (const att of decoded.atts) {
-      if (att.treatAs() === 'publicKey') {
+      if (att.treatAs(decoded.atts) === 'publicKey') {
         await MsgBlockParser.pushArmoredPubkeysToBlocks([att.getData().toUtfStr()], blocks);
       } else {
         const block = MsgBlock.fromAtt('decryptedAtt', '', {
@@ -92,26 +92,30 @@ export class MsgBlockParser {
     return { blocks, subject: decoded.subject, isRichText };
   };
 
-  private static detectBlockNext = (origText: string, startAt: number) => {
+  private static detectBlockNext = (origText: string, startAt: number, completeOnly?: boolean) => {
+    const armorHdrTypes = Object.keys(PgpArmor.ARMOR_HEADER_DICT) as ReplaceableMsgBlockType[];
     const result: { found: MsgBlock[]; continueAt?: number } = { found: [] as MsgBlock[] };
     const begin = origText.indexOf(PgpArmor.headers('null').begin, startAt);
     if (begin !== -1) {
       // found
-      const potentialBeginHeader = origText.substring(begin, begin + MsgBlockParser.ARMOR_HEADER_MAX_LENGTH);
-      for (const xType of Object.keys(PgpArmor.ARMOR_HEADER_DICT)) {
-        const type = xType as ReplaceableMsgBlockType;
-        const blockHeaderDef = PgpArmor.ARMOR_HEADER_DICT[type];
+      const potentialBeginHeader = origText.substr(begin, MsgBlockParser.ARMOR_HEADER_MAX_LENGTH);
+      for (const armorHdrType of armorHdrTypes) {
+        const blockHeaderDef = PgpArmor.ARMOR_HEADER_DICT[armorHdrType];
         if (blockHeaderDef.replace) {
           const indexOfConfirmedBegin = potentialBeginHeader.indexOf(blockHeaderDef.begin);
-          if (
-            indexOfConfirmedBegin === 0 ||
-            (type === 'encryptedMsgLink' && indexOfConfirmedBegin >= 0 && indexOfConfirmedBegin < 15)
-          ) {
-            // identified beginning of a specific block
+          if (indexOfConfirmedBegin === 0) {
+            let potentialTextBeforeBlockBegun = '';
             if (begin > startAt) {
-              const potentialTextBeforeBlockBegun = origText.substring(startAt, begin).trim();
-              if (potentialTextBeforeBlockBegun) {
-                result.found.push(MsgBlock.fromContent('plainText', potentialTextBeforeBlockBegun));
+              potentialTextBeforeBlockBegun = origText.substring(startAt, begin);
+              if (!potentialTextBeforeBlockBegun.endsWith('\n')) {
+                // only replace blocks if they begin on their own line
+                // contains deliberate block: `-----BEGIN PGP PUBLIC KEY BLOCK-----\n...`
+                // contains deliberate block: `Hello\n-----BEGIN PGP PUBLIC KEY BLOCK-----\n...`
+                // just plaintext (accidental block): `Hello -----BEGIN PGP PUBLIC KEY BLOCK-----\n...`
+                continue; // block treated as plaintext, not on dedicated line - considered accidental
+                // this will actually cause potential deliberate blocks that follow accidental block to be ignored
+                // but if the message already contains accidental (not on dedicated line) blocks,
+                // it's probably a good thing to ignore the rest
               }
             }
             let endIndex = -1;
@@ -128,34 +132,33 @@ export class MsgBlockParser {
                 foundBlockEndHeaderLength = matchEnd[0].length;
               }
             }
-            if (endIndex !== -1) {
-              // identified end of the same block
-              if (type !== 'encryptedMsgLink') {
+            if (endIndex !== -1 || !completeOnly) {
+              // flush the preceding plainText
+              potentialTextBeforeBlockBegun = potentialTextBeforeBlockBegun.trim();
+              if (potentialTextBeforeBlockBegun) {
+                result.found.push(MsgBlock.fromContent('plainText', potentialTextBeforeBlockBegun));
+              }
+              if (endIndex !== -1) {
+                // identified end of the same block
                 result.found.push(
-                  MsgBlock.fromContent(type, origText.substring(begin, endIndex + foundBlockEndHeaderLength).trim()),
+                  MsgBlock.fromContent(
+                    armorHdrType,
+                    origText.substring(begin, endIndex + foundBlockEndHeaderLength).trim(),
+                  ),
                 );
+                result.continueAt = endIndex + foundBlockEndHeaderLength;
               } else {
-                const pwdMsgFullText = origText.substring(begin, endIndex + foundBlockEndHeaderLength).trim();
-                const pwdMsgShortIdMatch = pwdMsgFullText.match(/[a-zA-Z0-9]{10}$/);
-                if (pwdMsgShortIdMatch) {
-                  result.found.push(MsgBlock.fromContent(type, pwdMsgShortIdMatch[0]));
-                } else {
-                  result.found.push(MsgBlock.fromContent('plainText', pwdMsgFullText));
-                }
+                result.found.push(MsgBlock.fromContent(armorHdrType, origText.substr(begin), true));
               }
-              result.continueAt = endIndex + foundBlockEndHeaderLength;
-            } else {
-              // corresponding end not found
-              result.found.push(MsgBlock.fromContent(type, origText.substring(begin), true));
+              break;
             }
-            break;
           }
         }
       }
     }
     if (origText && !result.found.length) {
       // didn't find any blocks, but input is non-empty
-      const potentialText = origText.substring(startAt).trim();
+      const potentialText = origText.substr(startAt).trim();
       if (potentialText) {
         result.found.push(MsgBlock.fromContent('plainText', potentialText));
       }

Core/source/mobile-interface/endpoints.ts

@@ -312,7 +312,7 @@ export class Endpoints {
     const { atts } = ValidateInput.parseAttachmentType(uncheckedReq);
     const parsedAtts = atts.map(attData => {
       const att = new Att(attData);
-      return { id: att.id, treatAs: att.treatAs() };
+      return { id: att.id, treatAs: att.treatAs([att]) };
     });
     return fmtRes({ atts: parsedAtts });
   };

Core/source/test.ts

@@ -771,6 +771,20 @@ test('parseDecryptMsg compat mime-email-plain', async t => {
   t.pass();
 });
 
+test('parseDecryptMsg parse PGP/MIME message with inline image', async t => {
+  const { keys } = getKeypairs('flowcrypt.compatibility2');
+  const { pubKeys } = getKeypairs('roma');
+  const { json: decryptJson } = await endpoints.parseDecryptMsg({ keys, isMime: true, verificationPubkeys: pubKeys }, [
+    await getCompatAsset('mime-email-encrypted-inline-image'),
+  ]);
+  expect(decryptJson).to.deep.equal({
+    text: '[neom-gBCMAENwknA-unsplash.jpg]',
+    replyType: 'encrypted',
+    subject: 'PGP/MIME message with inline image for flowcrypt-ios #2432',
+  });
+  t.pass();
+});
+
 test('parseDecryptMsg compat mime-email-plain-iso-2201-jp', async t => {
   const { keys } = getKeypairs('rsa1');
   const { data: blocks, json: decryptJson } = await endpoints.parseDecryptMsg({ keys, isMime: true }, [