161488 - forwarding encrypted mail as attachments should be first deciphered

Status: VERIFIED FIXED

(MailNews Core :: Security: S/MIME, defect, P2)

Description

[Kai Engert (:KaiE:)]

When forwarding an encrypted message as attachment, the attached message should
get decrypted before it is attached.

This is necessary to allow new recipients to be able to read the attached objects.

See also bug 141730, which already cared for the reply to and forward inline
scenarios.

Jean-Francois said, this bug will be more difficult to implement (than 141730),
because when forwarding as attachment, the messages is not piped through mime.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: Patch v1

This patch was created as teamwork by Jean-Francois and me, with 90% of the
ideas coming from JF.

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 105568 [details] [diff] [review]
Patch v1

r=kaie on the portions that were created by JF. In addition I have tested the
patch and it seems to work for me.

JF, could you review the portions that came from me, so we have a fully
reviewed patch?

Comment 3

[Jean-Francois Ducarroz]

Comment on attachment 105568 [details] [diff] [review]
Patch v1

m_mime_parser doesn't have to be a member variable but that's fine with me.
However, it's important to hold a grop on m_converter_channel. Maybe in the
future MIME should own the channel but that's another story.
I have tested the patch to be sure we are not leaking, everything seems fine.
R=ducarroz

Comment 4

[(not reading, please use [e-mail] instead)]

Comment on attachment 105568 [details] [diff] [review]
Patch v1

1)

please don't add the static cid.  (can fix it in nsMsgCreate.cpp and
sMsgQuote.cpp, too?  If we can get rid of all at once, it will prevent copy and
paste.)

instead, use the contract id (NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID) and
use do_CreateInstance().


2)

+  if (!msd->options->decrypt_p)
   msd->options->write_html_p	       = PR_TRUE;

is write_html_p initialized before this?

I assume we don't want:

msd->options->write_html_p = !msd->options->decrypt_p;

because that would override write_htmp_p in certain cases to be false, when it
should be true

3)

+      rv = nsComponentManager::CreateInstance(kStreamConverterCID, 
+					       NULL,
NS_GET_IID(nsIStreamConverter), 
+					       (void **)
getter_AddRefs(m_mime_parser)); 
+
+      if (NS_FAILED(rv) || !m_mime_parser)
+      {
+	 if (NS_SUCCEEDED(rv))
+	   rv =  NS_ERROR_UNEXPECTED;
+	 goto done;
+      }

switch to do_CreateInstance().	it's not going to succeeded and yet not give
you the instance.

so just do:

m_mime_parser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID,
&rv);
if (NS_FAILED(rv))
  goto done;

or:



4)

+      nsCOMPtr<nsIStreamListener> convertedListener =
do_QueryInterface(m_mime_parser);
+      if (!convertedListener)
+      {
+	 if (NS_SUCCEEDED(rv))
+	   rv =  NS_ERROR_UNEXPECTED;
+	 goto done;
+      }

do:

+      nsCOMPtr<nsIStreamListener> convertedListener =
do_QueryInterface(m_mime_parser, &rv);
+      if (NS_FAILED(rv))
+	 goto done;

6)

+      rv = NS_NewInputStreamChannel(getter_AddRefs(m_converter_channel), aURL,
nsnull,
+				     NS_LITERAL_CSTRING(""),
NS_LITERAL_CSTRING(""), -1);
+      if (NS_FAILED(m_mime_parser->AsyncConvertData(
+		    NS_LITERAL_STRING("message/rfc822").get(),
+		    NS_LITERAL_STRING("message/rfc822").get(),
+		    strListener, m_converter_channel)))
+      {
+	 if (NS_SUCCEEDED(rv))
+	   rv =  NS_ERROR_UNEXPECTED;
+	 goto done;
+      }

why not:

+      rv = NS_NewInputStreamChannel(getter_AddRefs(m_converter_channel), aURL,
nsnull,
+				     NS_LITERAL_CSTRING(""),
NS_LITERAL_CSTRING(""), -1);
+      if (NS_FAILED(rv))
+	 goto done;
+
+      rv = m_mime_parser->AsyncConvertData(
+		    NS_LITERAL_STRING("message/rfc822").get(),
+		    NS_LITERAL_STRING("message/rfc822").get(),
+		    strListener, m_converter_channel);
+      if (NS_FAILED(rv))
+	 goto done;

Comment 5

[Kai Engert (:KaiE:)]

Attachment: Patch v2

> 1)
> 
> please don't add the static cid.  (can fix it in nsMsgCreate.cpp and
> sMsgQuote.cpp, too?  If we can get rid of all at once, it will prevent copy
and
> paste.)
> 
> instead, use the contract id (NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID)
and
> use do_CreateInstance().

Fixed, including some calls in MsgCreate and MsgQuote, but only where there was
an contract ID constant already available.


> 2)
> 
> +  if (!msd->options->decrypt_p)
>    msd->options->write_html_p        = PR_TRUE;
> 
> is write_html_p initialized before this?
> 
> I assume we don't want:
> 
> msd->options->write_html_p = !msd->options->decrypt_p;
> 
> because that would override write_htmp_p in certain cases to be false, when
it
> should be true

I don't know, I must leave this comment to Jean-Francois.
What do you think?


> 3)

changed


> 4)

changed


there was no 5...


> 6)

changed

Comment 6

[Kai Engert (:KaiE:)]

Jean-Francois, could you please check whether we need to address Seth's comment 2?

Comment 7

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Oops, in the previous patch, I had not correctly compiled in compose, this new
patch corrects a wrong bracket in the call to AsyncConvertData.

Comment 8

[Jean-Francois Ducarroz]

the best way to address Seth comment #2 is to reorder the initialization of
write_html_p:

Index: mimemoz2.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/mime/src/mimemoz2.cpp,v
retrieving revision 1.184
diff -u -2 -w -r1.184 mimemoz2.cpp
--- mimemoz2.cpp        4 Oct 2002 22:40:53 -0000       1.184
+++ mimemoz2.cpp        12 Nov 2002 18:18:06 -0000
@@ -1518,4 +1518,5 @@
   //
   MIME_HeaderType = MimeHeadersAll;
+  msd->options->write_html_p  = PR_TRUE;
   switch (format_out)
   {
@@ -1544,4 +1545,9 @@
     case nsMimeOutput::nsMimeMessageFilterSniffer:    // generating an output
that can be scan by a message filter
       break;
+
+    case nsMimeOutput::nsMimeMessageDecrypt:
+      msd->options->decrypt_p = PR_TRUE;
+      msd->options->write_html_p = PR_FALSE;
+      break;
   }

@@ -1581,5 +1587,4 @@

   msd->options->url = msd->url_name;
-  msd->options->write_html_p          = PR_TRUE;
   msd->options->output_init_fn        = mime_output_init_fn;

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v4

This patch replaces the changes to mimemoz2.cpp with the new code from JF.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 105988 [details] [diff] [review]
Patch v4

Carrying forward the existing reviews for the trivial changes requested by
Seth.
r=kaie on the new logic for mimemoz2.cpp, it still works for me

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 105988 [details] [diff] [review]
Patch v4

sr=sspitzer

thanks for the fix, and the code cleanup!

two nits:


1)

+      // initialize a new stream converter, that uses the strListener as its
input
+      // obtain the input stream listener from the new converter,
+      // and pass the converter's input stream listener to DisplayMessage
+
+      m_mime_parser =
do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv);
+      if (NS_FAILED(rv) || !m_mime_parser)
+      {
+	 if (NS_SUCCEEDED(rv))
+	   rv =  NS_ERROR_UNEXPECTED;
+	 goto done;
+      }

just do:

+      m_mime_parser =
do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID, &rv);
+      if (NS_FAILED(rv))
+	 goto done;


2)

+  mimeParser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID,
&rv);
   if (NS_FAILED(rv) || !mimeParser)

since you are fixing this code, can you do:
+  mimeParser = do_CreateInstance(NS_MAILNEWS_MIME_STREAM_CONVERTER_CONTRACTID,
&rv);
+  if (NS_FAILED(rv))
   {
     Release();

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v5

as requested

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 105994 [details] [diff] [review]
Patch v5

carrying forward reviews

Comment 14

[Kai Engert (:KaiE:)]

Fixed on trunk.

Comment 15

[Charles Rosendahl]

20021204 Trnk builds - OSX, Win2k, and Linux7.3 - verified.

Comment 16

[Antonio.Xu]

this fixed caused a regression.  see
http://bugzilla.mozilla.org/show_bug.cgi?id=188344

Comment 17

[Antonio.Xu]

this fixed caused a regression.  see
http://bugzilla.mozilla.org/show_bug.cgi?id=188344

Comment 18

[(not reading, please use [e-mail] instead)]

this caused another regression, see #189988

Comment 19

[Kai Engert (:KaiE:)]

And we found another regression that was caused by this bug.
A message, containing a base 64 encoded object as it's single content, e.g. a
GIF image (not a multipart message), will arrive damaged when forwarded as
attachment. Actually, the image will get forwarded in its decoded binary form.

Comment 20

[Kai Engert (:KaiE:)]

The regression that I mentioned in the previous comment has been filed as bug
194636.