Bug 1240290

Tag #1273

1 files changed, 107 insertions, 41 deletions

diff --git a/mailnews/mime/src/mimecms.cpp b/mailnews/mime/src/mimecms.cpp
index 9289c0c33..6f7455c0a 100644
--- a/mailnews/mime/src/mimecms.cpp
+++ b/mailnews/mime/src/mimecms.cpp
@@ -8,6 +8,7 @@
 #include "nsICMSMessageErrors.h"
 #include "nsICMSDecoder.h"
 #include "mimecms.h"
+#include "mimemcms.h"
 #include "mimemsig.h"
 #include "nspr.h"
 #include "mimemsg.h"
@@ -28,6 +29,9 @@
 
 using namespace mozilla::mailnews;
 
+// The name "mime encrypted" is misleading, because this code is used
+// both for CMS messages that are encrypted, and also for messages that
+// aren't encrypted, but only contain a signature.
 
 #define MIME_SUPERCLASS mimeEncryptedClass
 MimeDefClass(MimeEncryptedCMS, MimeEncryptedCMSClass,
@@ -68,10 +72,11 @@ typedef struct MimeCMSdata
   bool ci_is_encrypted;
   char *sender_addr;
   bool decoding_failed;
+  bool skip_content;
   uint32_t decoded_bytes;
   MimeObject *self;
-  bool parent_is_encrypted_p;
-  bool parent_holds_stamp_p;
+  bool any_parent_is_encrypted_p;
+  bool any_parent_is_signed_p;
   nsCOMPtr<nsIMsgSMIMEHeaderSink> smimeHeaderSink;
   nsCString url;
 
@@ -81,12 +86,11 @@ typedef struct MimeCMSdata
   ci_is_encrypted(false),
   sender_addr(nullptr),
   decoding_failed(false),
+  skip_content(false),
   decoded_bytes(0),
   self(nullptr),
-  parent_is_encrypted_p(false),
-  parent_holds_stamp_p(false)
-  {
-  }
+  any_parent_is_encrypted_p(false),
+  any_parent_is_signed_p(false) {}
 
   ~MimeCMSdata()
   {
@@ -140,6 +144,46 @@ bool MimeEncryptedCMS_encrypted_p (MimeObject *obj)
   return false;
 }
 
+bool MimeEncOrMP_CMS_signed_p(MimeObject *obj) {
+  bool is_signed;
+
+  if (!obj) return false;
+  if (mime_typep(obj, (MimeObjectClass *)&mimeMultipartSignedCMSClass)) {
+    return true;
+  }
+  if (mime_typep(obj, (MimeObjectClass *)&mimeEncryptedCMSClass)) {
+    MimeEncrypted *enc = (MimeEncrypted *)obj;
+    MimeCMSdata *data = (MimeCMSdata *)enc->crypto_closure;
+    if (!data || !data->content_info) return false;
+    data->content_info->ContentIsSigned(&is_signed);
+    return is_signed;
+  }
+  return false;
+}
+
+bool MimeAnyParentCMSEncrypted(MimeObject *obj)
+{
+  MimeObject *o2 = obj;
+  while (o2 && o2->parent) {
+    if (MimeEncryptedCMS_encrypted_p(o2->parent)) {
+      return true;
+    }
+    o2 = o2->parent;
+  }
+  return false;
+}
+
+bool MimeAnyParentCMSSigned(MimeObject *obj)
+{
+  MimeObject *o2 = obj;
+  while (o2 && o2->parent) {
+    if (MimeEncOrMP_CMS_signed_p(o2->parent)) {
+      return true;
+    }
+    o2 = o2->parent;
+  }
+  return false;
+}
 
 bool MimeCMSHeadersAndCertsMatch(nsICMSMessage *content_info,
                                    nsIX509Cert *signerCert,
@@ -439,38 +483,37 @@ static void *MimeCMS_init(MimeObject *obj,
   data->output_fn = output_fn;
   data->output_closure = output_closure;
   PR_SetError(0, 0);
-  data->decoder_context = do_CreateInstance(NS_CMSDECODER_CONTRACTID, &rv);
-  if (NS_FAILED(rv))
-  {
-    delete data;
-    return 0;
-  }
 
-  rv = data->decoder_context->Start(MimeCMS_content_callback, data);
-  if (NS_FAILED(rv))
-  {
-    delete data;
-    return 0;
+  data->any_parent_is_signed_p = MimeAnyParentCMSSigned(obj);
+
+  if (data->any_parent_is_signed_p) {
+    // Parent is signed.
+    // We don't know yet if this child is signed or encrypted.
+    // (We'll know after decoding has completed and EOF is called.)
+    // We don't support "inner encrypt" with outer sign, because the
+    // inner encrypted part could have been produced by an attacker who
+    // stripped away a part containing the signature (S/MIME doesn't
+    // have integrity protection).
+    // A sign-then-sign encoding is confusing, too, because it could be
+    // an attempt to influence which signature is shown.
+    data->skip_content = true;
   }
 
-  // XXX Fix later XXX //
-  data->parent_holds_stamp_p =
-  (obj->parent &&
-   (mime_crypto_stamped_p(obj->parent) ||
-    mime_typep(obj->parent, (MimeObjectClass *) &mimeEncryptedClass)));
+  if (!data->skip_content) {
+    data->decoder_context = do_CreateInstance(NS_CMSDECODER_CONTRACTID, &rv);
+    if (NS_FAILED(rv)) {
+      delete data;
+      return 0;
+    }
 
-  data->parent_is_encrypted_p =
-  (obj->parent && MimeEncryptedCMS_encrypted_p (obj->parent));
+    rv = data->decoder_context->Start(MimeCMS_content_callback, data);
+    if (NS_FAILED(rv)) {
+      delete data;
+      return 0;
+    }
+  }
 
-  /* If the parent of this object is a crypto-blob, then it's the grandparent
-   who would have written out the headers and prepared for a stamp...
-   (This shit sucks.)
-   */
-  if (data->parent_is_encrypted_p &&
-    !data->parent_holds_stamp_p &&
-    obj->parent && obj->parent->parent)
-  data->parent_holds_stamp_p =
-    mime_crypto_stamped_p (obj->parent->parent);
+  data->any_parent_is_encrypted_p = MimeAnyParentCMSEncrypted(obj);
 
   mime_stream_data *msd = (mime_stream_data *) (data->self->options->stream_closure);
   if (msd)
@@ -530,9 +573,11 @@ MimeCMS_write (const char *buf, int32_t buf_size, void *closure)
 
   if (!data || !data->output_fn || !data->decoder_context) return -1;
 
-  PR_SetError(0, 0);
-  rv = data->decoder_context->Update(buf, buf_size);
-  data->decoding_failed = NS_FAILED(rv);
+  if (!data->decoding_failed && !data->skip_content) {
+    PR_SetError(0, 0);
+    rv = data->decoder_context->Update(buf, buf_size);
+    data->decoding_failed = NS_FAILED(rv);
+  }
 
   return 0;
 }
@@ -606,7 +651,12 @@ MimeCMS_eof (void *crypto_closure, bool abort_p)
   nsresult rv;
   int32_t status = nsICMSMessageErrors::SUCCESS;
 
-  if (!data || !data->output_fn || !data->decoder_context) {
+  if (!data || !data->output_fn) {
+    return -1;
+  }
+
+  if (!data->skip_content && !data->decoder_context) {
+    // If we don't skip, we should have a context.
     return -1;
   }
 
@@ -621,11 +671,12 @@ MimeCMS_eof (void *crypto_closure, bool abort_p)
    */
 
   PR_SetError(0, 0);
-  rv = data->decoder_context->Finish(getter_AddRefs(data->content_info));
-  if (NS_FAILED(rv))
-    status = nsICMSMessageErrors::GENERAL_ERROR;
+  if (!data->skip_content) {
+    rv = data->decoder_context->Finish(getter_AddRefs(data->content_info));
+    if (NS_FAILED(rv)) status = nsICMSMessageErrors::GENERAL_ERROR;
 
-  data->decoder_context = nullptr;
+    data->decoder_context = nullptr;
+  }
 
   nsCOMPtr<nsIX509Cert> certOfInterest;
 
@@ -644,6 +695,21 @@ MimeCMS_eof (void *crypto_closure, bool abort_p)
   if (data->decoding_failed)
     status = nsICMSMessageErrors::GENERAL_ERROR;
 
+  if (data->skip_content) {
+    // Skipping content means, we detected a forbidden combination
+    // of CMS objects, so let's make sure we replace the parent status
+    // with a bad status.
+    if (data->any_parent_is_signed_p) {
+      data->smimeHeaderSink->SignedStatus(aRelativeNestLevel,
+        nsICMSMessageErrors::GENERAL_ERROR, nullptr, data->url);
+    }
+    if (data->any_parent_is_encrypted_p) {
+      data->smimeHeaderSink->EncryptionStatus(aRelativeNestLevel,
+        nsICMSMessageErrors::GENERAL_ERROR, nullptr, data->url);
+    }
+    return 0;
+  }
+
   if (!data->content_info)
   {
     if (!data->decoded_bytes)