Incorrect S/Mime status shown in UI

VERIFIED FIXED in mozilla1.4final

Status

MailNews Core
Security
VERIFIED FIXED
15 years ago
10 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Trunk
mozilla1.4final
x86
All
Bug Flags:
blocking1.4 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

4.19 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
(not reading, please use seth@sspitzer.org instead)
: superreview+
(not reading, please use seth@sspitzer.org instead)
: approval1.4+
Details | Diff | Splinter Review
3.19 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
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.
(Assignee)

Comment 2

15 years ago
cc'ing Marina who was able to confirm the problem.
(Assignee)

Comment 3

15 years ago
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

15 years ago
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.
(Assignee)

Comment 5

15 years ago
Setting it to all platforms, since you saw it on Windows, I see it on Linux.
OS: Linux → All
(Assignee)

Comment 6

15 years ago
Created attachment 126078 [details] [diff] [review]
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.
(Assignee)

Comment 7

15 years ago
> 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.

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
Assignee: sspitzer → kaie
(Assignee)

Comment 9

15 years ago
Created attachment 126083 [details] [diff] [review]
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.
Attachment #126078 - Attachment is obsolete: true
(Assignee)

Comment 10

15 years ago
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.
(Assignee)

Comment 11

15 years ago
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.
(Assignee)

Comment 12

15 years ago
Created attachment 126084 [details] [diff] [review]
Patch v3

Sorry, I attached the wrong file.
Attachment #126083 - Attachment is obsolete: true
(Assignee)

Comment 13

15 years ago
Created attachment 126087 [details] [diff] [review]
Patch v4

Cleaning up and adding a comment.
Attachment #126084 - Attachment is obsolete: true
(Assignee)

Comment 14

15 years ago
Created attachment 126089 [details] [diff] [review]
Patch v4 - ignoring whitespace - demonstrating how small the patch is
we want this for 1.4 final
Status: NEW → ASSIGNED
Flags: blocking1.4+
Target Milestone: --- → mozilla1.4final
Comment on attachment 126087 [details] [diff] [review]
Patch v4

r/sr=sspitzer
a=asa,sspitzer,adt for 1.4
Attachment #126087 - Flags: superreview+
Attachment #126087 - Flags: review+
Attachment #126087 - Flags: approval1.4+
(Assignee)

Comment 17

15 years ago
Patch landed on both 1.4 branch after rc2 and trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Keywords: fixed1.4
Resolution: --- → FIXED
(Assignee)

Comment 18

15 years ago
Marina, since you were able to reproduce this bug, could you help us and be the
QA for this bug? Thanks!
QA Contact: junruh → marina
(Assignee)

Comment 19

15 years ago
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.

Updated

15 years ago
Keywords: nsbeta1+

Comment 20

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

Comment 21

15 years ago
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

15 years ago
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

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

Marking verified, verified1.4.
Status: RESOLVED → VERIFIED
Keywords: fixed1.4 → verified1.4
(Assignee)

Updated

15 years ago
Group: security
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.