Bug 1926810 - Fail mbox reads if offset into mbox file doesn't start at a "From " separator. r=mkmelin

This patch makes the Mbox reader much more fussy. Previously, it'd just start
reading from where it was told to, and ignored a missing "From " separator
line at the beginning of a message.
This patch makes it much more strict, throwing a Read() error if that "From "
line is not there.

Differential Revision: https://phabricator.services.mozilla.com/D227315

Depends on D227314

--HG--
extra : rebase_source : eadc9b10c9174402154e60c8ba7dc7319825038a
extra : amend_source : dbcc189026d6824a1f01aace9ce0b6f4ce231a25

mailnews/base/public/msgCore.h

@@ -171,6 +171,9 @@ class nsIMsgFolder;
 
 #define NS_MSG_ERROR_UNEXPECTED_SIZE NS_MSG_GENERATE_FAILURE(35)
 
+// Mbox message doesn't start with "From " separator line.
+#define NS_MSG_ERROR_MBOX_MALFORMED NS_MSG_GENERATE_FAILURE(36)
+
 /* Error codes for message compose are defined in
    compose\src\nsMsgComposeStringBundle.h. Message compose use the same error
    code space as other mailnews modules. To avoid any conflict, values between

mailnews/base/src/MboxMsgInputStream.cpp

@@ -62,9 +62,16 @@ class MboxParser {
    * via Drain().
    */
   bool IsFinished() const {
-    return Available() == 0 && (mState == eEOF || mState == eMessageComplete);
+    return Available() == 0 && (mState == eEOF || mState == eMessageComplete ||
+                                mState == eMalformed);
   }
 
+  /**
+   * Returns true if the parser has decided the mbox is malformed and refuses
+   * to proceed (e.g. Missing a "From " line at the beginning).
+   */
+  bool IsMalformed() const { return mState == eMalformed; }
+
   /**
    * Returns true when the end of the mbox has been reached (and the last
    * message has been completely read out via Drain()).
@@ -113,7 +120,8 @@ class MboxParser {
     while (true) {
       // If a message is complete (or the mbox is finished), then
       // we stall.
-      if (mState == eMessageComplete || mState == eEOF) {
+      if (mState == eMessageComplete || mState == eEOF ||
+          mState == eMalformed) {
         break;
       }
 
@@ -212,6 +220,7 @@ class MboxParser {
     eEmitQuoting,
     eEmitBodyLine,
     eMessageComplete,  // Message is complete (or ended prematurely).
+    eMalformed,        // Error. No initial "From " line was found.
     eEOF,              // End of mbox.
   } mState{eExpectFromLine};
 
@@ -221,17 +230,11 @@ class MboxParser {
   // the mbox file has been reached.
   span handle(span data) {
     {
-      const char* stateName[] = {"eExpectFromLine",
-                                 "eDiscardFromLine",
-                                 "eExpectHeaderLine",
-                                 "eEmitHeaderLine",
-                                 "eEmitSeparator",
-                                 "eExpectBodyLine",
-                                 "eCountQuoting",
-                                 "eEmitQuoting",
-                                 "eEmitBodyLine",
-                                 "eMessageComplete",
-                                 "eEOF"};
+      const char* stateName[] = {
+          "eExpectFromLine",  "eDiscardFromLine", "eExpectHeaderLine",
+          "eEmitHeaderLine",  "eEmitSeparator",   "eExpectBodyLine",
+          "eCountQuoting",    "eEmitQuoting",     "eEmitBodyLine",
+          "eMessageComplete", "eMalformed",       "eEOF"};
       MOZ_LOG(gMboxLog, LogLevel::Verbose,
               ("MboxParser - handle %s (%zu bytes: '%s')", stateName[mState],
                data.Length(),
@@ -258,6 +261,8 @@ class MboxParser {
         return handle_eEmitBodyLine(data);
       case eMessageComplete:
         return handle_eMessageComplete(data);
+      case eMalformed:
+        return handle_eMalformed(data);
       case eEOF:
         return handle_eEOF(data);
       default:
@@ -339,10 +344,9 @@ class MboxParser {
       }
       mState = eDiscardFromLine;
     } else {
-      MOZ_LOG(gMboxLog, LogLevel::Warning,
+      MOZ_LOG(gMboxLog, LogLevel::Error,
               ("MboxParser - Missing 'From ' separator"));
-      // Just jump straight to header phase.
-      mState = eExpectHeaderLine;
+      mState = eMalformed;
     }
     return data;
   }
@@ -521,6 +525,9 @@ class MboxParser {
     return data;
   }
 
+  // Halt parsing, So this is a no-op.
+  span handle_eMalformed(span data) { return data; }
+
   // All done, so this is a no-op.
   span handle_eEOF(span data) {
     MOZ_ASSERT(data.IsEmpty());
@@ -871,6 +878,10 @@ nsresult MboxMsgInputStream::PumpData() {
     mUnused -= consumed;
   }
 
+  if (mParser->IsMalformed()) {
+    return NS_MSG_ERROR_MBOX_MALFORMED;
+  }
+
   return NS_OK;
 }
 

mailnews/base/test/gtest/Helpers.cpp

@@ -12,13 +12,16 @@
 
 namespace testing {
 
-void ExtractFromMbox(nsACString const& mbox, nsTArray<nsCString>& msgs,
-                     size_t readSize) {
+// Parses all the messages in mbox returning them as an array.
+nsresult ExtractFromMbox(nsACString const& mbox, nsTArray<nsCString>& msgs,
+                         size_t readSize) {
   // Open stream for raw mbox.
   nsCOMPtr<nsIInputStream> raw;
   nsresult rv = NS_NewByteInputStream(getter_AddRefs(raw), mozilla::Span(mbox),
                                       NS_ASSIGNMENT_COPY);
-  ASSERT_TRUE(NS_SUCCEEDED(rv));
+  if (NS_FAILED(rv)) {
+    return rv;
+  }
 
   msgs.Clear();
   // Wrap with MboxMsgInputStream and read single message.
@@ -27,7 +30,9 @@ void ExtractFromMbox(nsACString const& mbox, nsTArray<nsCString>& msgs,
   while (true) {
     nsAutoCString got;
     rv = Slurp(rdr, readSize, got);
-    ASSERT_TRUE(NS_SUCCEEDED(rv));
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
     // Corner case: suppress dud message for empty mbox file.
     if (!rdr->IsNullMessage()) {
       msgs.AppendElement(got);
@@ -35,11 +40,14 @@ void ExtractFromMbox(nsACString const& mbox, nsTArray<nsCString>& msgs,
     // Try and reuse the MboxMsgInputStream for the next message.
     bool more;
     rv = rdr->Continue(more);
-    ASSERT_TRUE(NS_SUCCEEDED(rv));
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
     if (!more) {
       break;
     }
   }
+  return NS_OK;
 }
 
 // Read all the data out of a stream into a string, reading readSize
@@ -50,7 +58,9 @@ nsresult Slurp(nsIInputStream* src, size_t readSize, nsACString& out) {
   while (true) {
     uint32_t n;
     nsresult rv = src->Read(readbuf.Elements(), readbuf.Length(), &n);
-    NS_ENSURE_SUCCESS(rv, rv);
+    if (NS_FAILED(rv)) {
+      return rv;
+    }
     if (n == 0) {
       break;  // EOF.
     }

mailnews/base/test/gtest/Helpers.h

@@ -24,8 +24,8 @@ namespace testing {
  * produce bad results at different read sizes, so this can be used to
  * shake it out by asking for, say, a single byte at a time.
  */
-void ExtractFromMbox(nsACString const& mbox, nsTArray<nsCString>& msgs,
-                     size_t readSize = 4096);
+nsresult ExtractFromMbox(nsACString const& mbox, nsTArray<nsCString>& msgs,
+                         size_t readSize = 4096);
 
 /**
  * Slurp just reads the src stream until EOF, returning the data in

mailnews/base/test/gtest/TestMsgMboxRead.cpp

@@ -41,7 +41,8 @@ static void dumpCase(testing::MboxCase const& t,
 // readSize lets the caller set the size of the stream Read() calls.
 static void runInputStreamCase(testing::MboxCase const& t, size_t readSize) {
   nsTArray<nsCString> msgs;
-  testing::ExtractFromMbox(t.mbox, msgs, readSize);
+  nsresult rv = testing::ExtractFromMbox(t.mbox, msgs, readSize);
+  ASSERT_TRUE(NS_SUCCEEDED(rv));
 
   // Display test data + results before asserting to make troubleshooting
   // simpler.
@@ -154,3 +155,17 @@ TEST(TestMsgMboxRead, Ambiguities)
     }
   }
 }
+
+// Test to handle missing From line.
+// Not too exhaustive (more in xpcshell tests).
+// Just make sure that a missing "From " line causes an error.
+TEST(TestMsgMboxRead, MissingFromLine)
+{
+  nsAutoCString borkedMbox("blah blah blah");
+
+  for (size_t s : {1, 3, 4096}) {
+    nsTArray<nsCString> msgs;
+    nsresult rv = testing::ExtractFromMbox(borkedMbox, msgs, s);
+    ASSERT_EQ(rv, NS_MSG_ERROR_MBOX_MALFORMED);
+  }
+}

mailnews/imap/test/unit/test_downloadOffline.js

@@ -59,3 +59,41 @@ add_task(async function verifyDownloaded() {
     await streamListener.promise;
   }
 });
+
+/**
+ * For mbox, make sure that offline messages fail if the storeTokens
+ * don't point to the beginning of a message.
+ */
+add_task(
+  {
+    skip_if: () => IMAPPump.inbox.msgStore.storeType != "mbox",
+  },
+  async function checkBadStoreTokens() {
+    const inbox = IMAPPump.inbox;
+
+    // Corrupt the storeTokens by adding 3.
+    for (const msg of inbox.messages) {
+      const offset = Number(msg.storeToken) + 3;
+      msg.storeToken = offset.toString();
+    }
+
+    // Make sure message reading fails.
+    const NS_MSG_ERROR_MBOX_MALFORMED = 0x80550024;
+    for (const msg of inbox.messages) {
+      const streamListener = new PromiseTestUtils.PromiseStreamListener();
+      const uri = inbox.getUriForMsg(msg);
+      const service = MailServices.messageServiceFromURI(uri);
+      try {
+        service.streamMessage(uri, streamListener, null, null, false, "", true);
+        await streamListener.promise;
+        Assert.ok(false, "Bad storeToken should cause error.");
+      } catch (e) {
+        Assert.equal(
+          e,
+          NS_MSG_ERROR_MBOX_MALFORMED,
+          "Bad storeToken causes NS_MSG_ERROR_MBOX_MALFORMED for mbox"
+        );
+      }
+    }
+  }
+);

mailnews/local/test/unit/test_mboxMalformed.js

@@ -10,6 +10,9 @@
 const { PromiseTestUtils } = ChromeUtils.importESModule(
   "resource://testing-common/mailnews/PromiseTestUtils.sys.mjs"
 );
+const { MessageGenerator } = ChromeUtils.importESModule(
+  "resource://testing-common/mailnews/MessageGenerator.sys.mjs"
+);
 
 // Force mbox mailstore.
 Services.prefs.setCharPref(
@@ -60,3 +63,45 @@ add_task(async function test_unescapedMbox() {
   // Clear up.
   localAccountUtils.clearAll();
 });
+
+/**
+ * Test that reading from bad offset fails.
+ */
+add_task(async function test_badStoreTokens() {
+  localAccountUtils.loadLocalMailAccount();
+  const inbox = localAccountUtils.inboxFolder;
+
+  // Add some messages to inbox.
+  const generator = new MessageGenerator();
+  inbox.addMessageBatch(
+    generator.makeMessages({ count: 10 }).map(m => m.toMessageString())
+  );
+
+  // Corrupt the storeTokens by adding 3
+  for (const msg of inbox.messages) {
+    const offset = Number(msg.storeToken) + 3;
+    msg.storeToken = offset.toString();
+  }
+
+  // Check that message reads fail.
+  const NS_MSG_ERROR_MBOX_MALFORMED = 0x80550024;
+  for (const msg of inbox.messages) {
+    const streamListener = new PromiseTestUtils.PromiseStreamListener();
+    const uri = inbox.getUriForMsg(msg);
+    const service = MailServices.messageServiceFromURI(uri);
+
+    try {
+      service.streamMessage(uri, streamListener, null, null, false, "", true);
+      await streamListener.promise;
+    } catch (e) {
+      Assert.equal(
+        e,
+        NS_MSG_ERROR_MBOX_MALFORMED,
+        "Bad read causes NS_MSG_ERROR_MBOX_MALFORMED"
+      );
+    }
+  }
+
+  // Clear up.
+  localAccountUtils.clearAll();
+});