209995 - Incorrect S/Mime status shown in UI

Status: VERIFIED FIXED

(MailNews Core :: Security, defect)

Description

[Kai Engert (:KaiE:)]

I saw this problem with Mozilla 1.3, but I very much suspect the problem is
present on latest Mozilla 1.4, too.

To see this problem, you must have to do the following:
- have Junk Mail Controls enabled for your account
- be on a slow network connection, like a 56K modem dialup
- have several new messages in your inbox, like >= 50

When you connect to your mailbox, Mozilla Mail will download headers of all new
messages. The user can not do anything during that phase but must wait. Once
header downloading is finished, Mail updates the view and shows headers for all
new messages.

At this time:
- the user is able to click on messages to read them
- in parallel the Junk Mail logic downloads messages to classify them, which can
take some minutes

I clicked a mail from bugzilla, which is of course not S/Mime signed.
Moments later I recognized a "valid signature icon"!!!
I clicked the icon, and the message info said "valid signature from Bob Lord".
I discarded this information dialog, and now saw an invalid signature icon,
clicked it, and saw "invalid signature from Yumaris"!

I believe this problem comes from the way the S/Mime UI update has been
implemented in the past, and that is still in use.

Whenever a message is getting processed, it seems not to make a difference
whether the processing's intention is for display, or for some background
processing (like junk classification).

As a result, whenever a message is processed, the code that updates the UI gets
triggered, and pushes the information to the UI.

Comment 1

[Kai Engert (:KaiE:)]

I don't yet have a fix, but here are some first explanations about the involved
code:

Updating the UI happens in
  mailnews/extensions/smime/resources/content/msgHdrViewSMIMEOverlay.js

The code hooks into gMessageListeners from base mailnews
  (see file mailnews/base/resources/content/msgHdrViewOverlay.js)
that is supposed to notifiy the S/Mime code whenever when another message is
being displayed. When this gets triggered, we reset the UI to "not encrypted /
not signed".

Whenever an S/Mime message gets processed, for whatever reason, the mail mime
code handling in files
  mailnews/mime/src/mimemcms.cpp
  mailnews/mime/src/mimecms.cpp
is traversed.

The code determines the signature/encryption status of the message, and tries to
find out whether the current message belongs to a message window.
It uses this code:

  mime_stream_data *msd = (mime_stream_data *)
(data->self->options->stream_closure);
  if (msd)
  {
    nsIChannel *channel = msd->channel;  // note the lack of ref counting...
    if (channel)
    {
      nsCOMPtr<nsIURI> uri;
      nsCOMPtr<nsIMsgWindow> msgWindow;
      nsCOMPtr<nsIMsgHeaderSink> headerSink;
      nsCOMPtr<nsIMsgMailNewsUrl> msgurl;
      nsCOMPtr<nsISupports> securityInfo;
      channel->GetURI(getter_AddRefs(uri));
      if (uri)
        msgurl = do_QueryInterface(uri);
      if (msgurl)
        msgurl->GetMsgWindow(getter_AddRefs(msgWindow));
      if (msgWindow)
        msgWindow->GetMsgHeaderSink(getter_AddRefs(headerSink));
      if (headerSink)
        headerSink->GetSecurityInfo(getter_AddRefs(securityInfo));
      if (securityInfo)
        data->smimeHeaderSink = do_QueryInterface(securityInfo);
    } // if channel
  } // if msd

If an associated window for the current message processing channel is found, the
crypto UI portion of the message window will be remembered (smimeHeaderSink).

Once processing of the message is done, calls to the message window's
smimeHeaderSink will happen, either or both of "signedStatus" /
"encrpytionStatus", and the UI will be updated.

I don't understand why this problem is only seen sometimes.


I think a fix should extend the processing in the mime library, in files
mimecms.cpp and mimemcms.cpp

Before querying the channel for the window, another test should be made, trying
to find out the intention of the current message processing action.

If the processing intention is "display in window", then proceed with our
current logic.

If the processing intention is "do background processing without displaying",
then we should skip the attempt to obtain a window, and remember that we don't
want to update the UI for this transaction.


I don't know the mail code well enough to say how we could distinguish the
intention of processing.

Comment 2

[Kai Engert (:KaiE:)]

cc'ing Marina who was able to confirm the problem.

Comment 3

[Kai Engert (:KaiE:)]

Now I can reproduce this easily.

To make sure you can test this by sending yourself email, prepare your profile:
- quit Mozilla
- move your training.dat from your profile to a safe place
- enable mail junk controls
- remove the checkbox from 
  "do not mark messages as junk mail if the sender is in my address book".
  For me, this enabled junk checking of messages you send to yourself.

Now to see the bug:
- Click on a signed message in your inbox, the message display will display a
signature icon.
- Send yourself a standard message, that is not signed.
- Check mail.
- see the message gets delivered to your inbox (but do not click it).

Actual behaviour:
  The message signature icon goes away.
  The signed message, that is still displayed in your message pane,
  is now reported as being unsigned.

Expected behaviour:
  The previously shown signature status should not change.


The same bug works in the reverse.
The is, if you look at a standard message, while no signature icons are shown,
and you then fetch a signed message from the server, you'll suddenly see the
signature icon.

Comment 4

[marina]

That's the scenario where i was able to repro this problem: i had the checkbox
"do not mark messages as junk mail if the sender is in my address book"
unchecked. Also is it for Linux only? I am seeing it on Windows.

Comment 5

[Kai Engert (:KaiE:)]

Setting it to all platforms, since you saw it on Windows, I see it on Linux.

Comment 6

[Kai Engert (:KaiE:)]

Attachment: Tentative Patch v1

This patch fixes the problem for me, but I don't understand whether it will
cause side effects.

The idea is to not pass in a window handle at all, when loading a message for
junk mail classificiation.

A better fix would be: Add an additional flag somewhere, that is set when doing
filtering / junk mail processing, and make only the S/Mime code check for this
flag.


I wonder if this bug is restricted to junk mail, or whether message filtering
could also trigger it. I suspect it could. This would require a bigger patch
than this.

Comment 7

[Kai Engert (:KaiE:)]

> I wonder if this bug is restricted to junk mail, or whether message filtering
> could also trigger it. I suspect it could. This would require a bigger patch
> than this.

Luckily, message filtering does not trigger this bug.

I have been playing with the attached patch and do not see a problem yet, but I
don't really know what I should test, what could potentially be affected from
not passing a message window handle to junk mail fetching any more.

Comment 8

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

talking to kaie over aim, and he is working on a differt fix, which uses the
header=filter query string value that we append in the junk mail / filter case.

the fix would be limited to mozilla\mailnews\mime\src\mimecms.cpp and
mozilla\mailnews\mime\src\mimemcms.cpp

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v2

This is a better patch that only changes S/Mime code, nothing else.

Thanks for Scott for an initial idea, and for Seth for assistance with this
patch, and confirming this approach is good.

Comment 10

[Kai Engert (:KaiE:)]

This patch relies on the fact that Junk Mail classification adds an additional
"filter" header to the data we are processing, which allows us to only change
the S/Mime code add a special behaviour when this kind of stream is processed.

Comment 11

[Kai Engert (:KaiE:)]

I tested the patch, it works for me.

I had added tracing printf statements to verify we arrive in the correct code
paths when reading and junk mail classification processing.

It also worked for me with messages that were both signed and encrypted.

Comment 12

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Sorry, I attached the wrong file.

Comment 13

[Kai Engert (:KaiE:)]

Attachment: Patch v4

Cleaning up and adding a comment.

Comment 14

[Kai Engert (:KaiE:)]

Attachment: Patch v4 - ignoring whitespace - demonstrating how small the patch is

Comment 15

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

we want this for 1.4 final

Comment 16

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

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

r/sr=sspitzer
a=asa,sspitzer,adt for 1.4

Comment 17

[Kai Engert (:KaiE:)]

Patch landed on both 1.4 branch after rc2 and trunk.

Comment 18

[Kai Engert (:KaiE:)]

Marina, since you were able to reproduce this bug, could you help us and be the
QA for this bug? Thanks!

Comment 19

[Kai Engert (:KaiE:)]

Before checking in, I had tested with several different combinations, all looked
fine for me.

I had tested "looking at plain, fetching signed, encrypted, signed+encrypted",
and "looking at signed, fetching plain", worked fine.

I also tested forwardning a signed message, where the outer message is plain.
This also works. But this shouldn't be a problem at all, because we ignore the
S/Mime properties of nested elements anyway.

However, opening a forwarded signed message in a new message window, by double
clicking the attached message, still works correctly.

I think we should be fine.

Comment 20

[marina]

i don't see this problem happening any more under same circumstances, used
20030620-1.4 build on WinXP

Comment 21

[Charles Rosendahl]

20030620-1.4 Win2k
Reproduced by on 6-18 branch using comment #3 and:
- after doing the setup Kai mentions
- go to a folder with a variety of signed/encrypted/unsigned messages and run
"tools->run junk mail controls on folder"  You whirl through the icons in the UI.

Now trying 20030620-1.4 results next comment.

Comment 22

[Charles Rosendahl]

20030620-1.4 Branch Win2k
Using the test case in comment #21, the behaviour is correct in this build.

Using a folder containing a variety of messages (signed, signed/encrypted,
unsigned, invalid, questionable signature, etc.) Running the same test through
the 6/18 build spun the icons in the UI.

On the 6/20 build, junk mail filtering does not alter the PSM UI.

Next Comment:  Mac 10.2

Comment 23

[Charles Rosendahl]

Also verified that it does not happen on the 6/19 and 6/20 1.4 OSX 10.2 builds.

Marking verified, verified1.4.