1558595 - S/MIME signature shown for incorrect message

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Kenny Ashford]

User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

Received S/MIME message with signature. Click on S/MIME message. Immediately click on other message that isn't signed.

Actual results:

Sometimes the little signature envelope is shown for the message that isn't signed.

Expected results:

Thunderbird should never show the little signature envelope for messages that aren't signed.

Comment 1

[Kenny Ashford]

Attachment: fix-signature-bug.diff

fix

Comment 3

[Kai Engert (:KaiE:)]

Thank you for the bug report and the patch.

Comment 4

[Kai Engert (:KaiE:)]

I see a potential issue with this patch. It assumes that message always have a message ID. If a message doesn't have one, it will not display signature or encryption status.

Magnus, Wayne, do you know if we can assume that a message always has a message ID?

There's code that attempts to obtain a message ID from a parent message part. I guess that makes sense for nested messages, if a message is multipart, the encrypted part is the first subpart, and potentially the signed part is another level inside the message. That code is repeated multiple times, it would be good to move it to a shared function.

Comment 5

[Magnus Melin [:mkmelin]]

We can't assume a message has a message id.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 9071376 [details] [diff] [review]
fix-signature-bug.diff

Then unfortunately we cannot take this patch as it is.

We need a message identifier that's unique and always present.
Should we try to use the message ID, and if it's missing, we synthesize an ID based on from/to/subject/timestamp?
Other ideas?

Comment 7

[Magnus Melin [:mkmelin]]

I guess we can rely on the message being unique enough. If we're only checking if it's changed. Maybe we can also require/assume signed/encrypted messages need to have an id.

Comment 8

[Kai Engert (:KaiE:)]

Magnus, if we cannot assume that a message has a message ID, why is it OK to assume that a signed/encrypted message has a message ID?

Comment 9

[Magnus Melin [:mkmelin]]

I think messages without ids are very likely not to get delivered. It's usually generated mails from poorly written software (e.g. mails from various web forums). I think if you care enough to implement sign or encrypt, you are going to have the message id there too.

Comment 10

[Kai Engert (:KaiE:)]

Maybe it would be good to have a fallback, if there isn't a message ID.
I'm worried about crafted messages that we might use in test cases. I think it would be confusing if we don't show a status for those.

How about we fall back to use the concatenation of subject+from+to+cc+date and use that as a replacement identifier?

Comment 11

[Magnus Melin [:mkmelin]]

There's no guarantee any of those are set either, but I guess you can assume as much ;)

One thing we always have is the nsIMsgDBHdr.messageKey

Comment 12

[Kai Engert (:KaiE:)]

Attachment: 1558595-v3.patch

Thanks Magnus, that's very helpful!

I'm attaching an improved version of the contributed patch, which addresses my change requests, and queries and compares the messageKey that Magnus suggested.

I've kept the messageID string for now, and require both to match. Magnus, if you think that's redundant, because we really always have the messageKey, then we can remove the messageID parts of the code.

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9072706 [details] [diff] [review]
1558595-v3.patch

Review of attachment 9072706 [details] [diff] [review]:
-----------------------------------------------------------------

I guess the messageid can be useful at least for debugging purposes. Potentially make it optional(?) and put it last in the function signature.

Please also test that it works for an .eml.

::: mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js
@@ +32,5 @@
>    maxWantedNesting() {
>      return 1;
>    },
>  
> +  getTrimmedMsgID(aMessageID) {

instead of cleaning up here, can we just do that in MimeFindMessageID

@@ +56,5 @@
> +    let trimmedID = this.getTrimmedMsgID(aMessageID);
> +    let selectedMessageHdr = gFolderDisplay.selectedMessage.QueryInterface(Ci.nsIMsgDBHdr)
> +
> +    if (gFolderDisplay.selectedMessage.messageId != trimmedID
> +        || selectedMessageHdr.messageKey != aMessageKey) {

we put || at the end of the line

@@ +64,5 @@
>      gSignatureStatus = aSignatureStatus;
>      gSignerCert = aSignerCert;
>  
>      gSMIMEContainer.collapsed = false;
>      gSignedUINode.collapsed = false;

Should we make sure to set these to true for error conditions? Maybe they already are ensure to be that.

::: mailnews/mime/src/mimecms.cpp
@@ +458,5 @@
>        nsCOMPtr<nsISupports> securityInfo;
>        channel->GetURI(getter_AddRefs(uri));
>        if (uri) {
> +        nsCOMPtr<nsIMsgDBHdr> msgHdr;
> +        MimeGetMsgHdrForURI(uri, getter_AddRefs(msgHdr));

needs rv checking. 
note also that .eml messages opened in the standalone msg window won't have a msgHdr (though you can obtain a fake one if necessary)

@@ +639,5 @@
>        MimeCMSGetFromSender(data->self, from_addr, from_name, sender_addr,
>                             sender_name);
>  
> +      nsCString idStr;
> +      MimeFindMessageID(data->self, idStr);

might want to write it as 
(void) MimeFindMessageID(data->self, idStr); 

... when you don't intend to care if the id was set or not

::: mailnews/mime/src/mimemoz2.cpp
@@ +1780,5 @@
> +  }
> +  if (!msgId) {
> +    return false;
> +  }
> +  outString = msgId;

Surprised this assignment works.

::: mailnews/mime/src/mimemoz2.h
@@ +187,5 @@
>  extern "C" char *MimeGetStringByID(int32_t stringID);
>  extern "C" char *MimeGetStringByName(const char16_t *stringName);
>  
> +extern "C" bool MimeFindMessageID(MimeObject *obj, nsCString &outString);
> +extern "C" void MimeGetMsgHdrForURI(nsIURI *uri, nsIMsgDBHdr **aMsgHdr);

should probably both return nsresult

Comment 14

[Kai Engert (:KaiE:)]

Magnus, two issues:

• messageKey isn't a global unique ID, apparently it's only "unique inside a folder". That's insufficient as an identifier alone. The user could click between two local folders, each containing only one message, and the ID will be "1" in both scenarios, and we show the incorrect status.

• when opening a message from an .eml file, then gFolderDisplay.selectedMessage.QueryInterface fails with "QueryInterface is not a function". The object has very few attributes, compared to the selectMessage object when clicking a message in a folder.

Comment 15

[Kai Engert (:KaiE:)]

I tried to use multiple attributes in combination, but the challenge is, we can only use attributes that are available from both the MIME processing code, and also from inside the JS code for the currently selected message. I couldn't find a way to obtain the MIME headers from selected message object. Also, the comparison is easy only for attributes that use the same encoding on both sides.

If the subject is UTF-8 encoded, then the JS side gets different values based on how we open the message. If we open it from a folder, the subject is raw. If we open it from eml, then it's decoded. So I've excluded the subject for now.

Date is tricky, because the MIME code only has access to the header string (unless we identify a parser and decoder to numeric value), and for gFolderDisplay.selectedMessage we only have a numeric presentation. I found jsmime.headerparser.parseDateHeader, so I could pass over the raw header string from MIME, then parse it. But I don't know if that will always work for complex messages. If the Date: header is missing, will the JS code use some other header to produce the numeric value?

Also, the value contained in the JS object is factor 1000 larger then we I get from Date(jsmime.headerparser.parseDateHeader).getTime(). (13 digits vs 16 digits.) Can I assume that the last 3 digits of the JS attribute will always be 000?

Usually, if one side has an attribute, but the other side doesn't, then we conclude it's different. An exception is the messageKey, if one side doesn't have id, we must ignore the other side's messageKey.

From JS, I think we must probably compare the incoming attributes with the attributes that we can obtain through gFolderDisplay.selectedMessage.

Comment 16

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #13)

• getTrimmedMsgID(aMessageID) {

instead of cleaning up here, can we just do that in MimeFindMessageID

I did that, but then I found it's insufficient. I sometimes get the brackets for the selected message when opening as eml. I think we should trim on both sides.

we put || at the end of the line

ok

• outString = msgId;

Surprised this assignment works.

In C++ you can redefine how the "=" operator works, by defining a method called "operator=". That's what the Mozilla string class does, for parameter "const char*", which will call strlen, allocate and then copy.

Comment 17

[Kai Engert (:KaiE:)]

@@ +64,5 @@

 gSignatureStatus = aSignatureStatus;
 gSignerCert = aSignerCert;

 gSMIMEContainer.collapsed = false;
 gSignedUINode.collapsed = false;

Should we make sure to set these to true for error conditions? Maybe they
already are ensure to be that.

Shouldn't be necessary.
Whenever a new message gets selected, the display is reset to the "not S/MIME" state.

If we get a notification that doesn't belong to the shown message, the display has already been reset, and we shouldn't need to do that.

   if (uri) {

•    nsCOMPtr<nsIMsgDBHdr> msgHdr;
  

•    MimeGetMsgHdrForURI(uri, getter_AddRefs(msgHdr));
  

needs rv checking.

OK

•  MimeFindMessageID(data->self, idStr);
  

might want to write it as
(void) MimeFindMessageID(data->self, idStr);

... when you don't intend to care if the id was set or not

ok

+extern "C" bool MimeFindMessageID(MimeObject *obj, nsCString &outString);
+extern "C" void MimeGetMsgHdrForURI(nsIURI *uri, nsIMsgDBHdr **aMsgHdr);

should probably both return nsresult

ok

Comment 18

[Kai Engert (:KaiE:)]

Attachment: 1558595-v4.patch

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 9072891 [details] [diff] [review]
1558595-v4.patch

Review of attachment 9072891 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like overcomplicating things. Let's think about the original issue here. It's a timing issue right? That in some edge cases the notification can arrive after you already switched the message.

Some other thoughts:
 - for .eml messages I doubt it's an issue at all. The notifications are all for the same message, same window. So I don't think the wrong notification can happen. Just need to make sure it doesn't break. 
 - yes messageKey is only unique per folder, but it's an always increasing number (no reassignments), so for our use case it seems it should be unique enough. The chances that you would have two messages with missing ids, in two different folders, with the same messageKey, and that you would switch between those folders, between exactly those two messages, quickly enough (we're likely talking about a few 100ms maybe), and that the notifications would happen to in that case arrive in in the wrong order... seems super fetched.

So, I don't think you need the SyntheticID thing. 
You should be able to trust that selectedMessage.messageId is already without the <> or that's a bug elsewhere (but for the two messages we'd compare, I would assume both have the same bug then and just comparing should be fine)

Comment 20

[Kai Engert (:KaiE:)]

Magnus, I'd prefer a solution, that works in the following simple scenario:

• two folders
• each folder has a message without message ID.

This can happen when crafting messages for testing, copying each message to their own folder, messageKey will be 1 for both messages.
We shouldn't show wrong information in this simple scenario.

Relying only on messageID and messageKey is insufficient to distinguish the messages.

Comment 21

[Kai Engert (:KaiE:)]

I've discovered another detail. If the message doesn't have a message ID, then the gFolderDisplay.selectedMessage.messageID will be a string like "md5:(base64-encoded)".

The MIME code currently sets it to an empty string (which is sent for the status call). I guess we cannot easily synthesize that from inside the MIME code, we'd have to wait for the complete streaming of the whole message, and delay the S/MIME status call until we are done and have calculated the MD5. Seems difficult.

Unless we synthsize it inside the MIME code, a messageID comparison in the UI JS will always get a mismatch for messages with an empty messageID.

This turns out be much harder than I thought.

Comment 22

[Kai Engert (:KaiE:)]

If the selected messageID on the JS side starts with "md5:" we could decide to treat that as "empty".

The remaining tricky scenario is, if messageKey matches (could be a real match, or caused by different folders), and both messageIDs are considered empty, what do we do?

Not showing S/MIME information for messages without ID is misleading.

If we show the S/MIME status, then I can still reproduce the bug. (One folder has big S/MIME message which takes a little to process, other folder has plain text message. Switch folder quickly, and plain message gets S/MIME status display.)

To avoid showing false information, we'd have to use additional information for comparison, like the synthesizing I've suggested.

Comment 23

[Magnus Melin [:mkmelin]]

Hmm, if we have a messaageKey we also have a folder. So messageKey+folder is unique at least. We could compare that.?

Comment 24

[Kai Engert (:KaiE:)]

Folder isn't unique if we have multiple accounts.

I compared the URI that we have inside MIME and gFolderDisplay.selectedMessageUris[0], look very different.

MIME:
mailbox:///home/user/.thunderbird/[profilepath]/Mail/Local%20Folders/folder4?number=1

gFolderDisplay.selectedMessageUris[0]:
mailbox-message://nobody@Local%20Folders/folder4#1

Comment 25

[Kai Engert (:KaiE:)]

Magnus, I think demanding that {from+to+cc+date} is different on empty messageID is reasonable.

I think that "seems overcomplicated" shouldn't be a argument for rejecting the solution. We're required to be certain when we deal with security status, and if that takes complicated code, so be it.

We already have the code for that approach from the synth patch, and searching for another solution will take again more work. Given that S/MIME status is tied to "from" anyway, it seems reasonable to include that into comparison. And comparing the date shouldn't hurt either.

Comment 26

[Kai Engert (:KaiE:)]

Attachment: 1558595-v5.patch

With this revision of the patch:

• we'll skip comparing if the messageKey is absent,
  because then we conclude display is happening in a dedicated window

• if the messageKey is available, we'll conclude it's different,
  if the keys are different

Everything else is fallback for less common cases.

If the message has a messageID header, then we'll use only that for
comparison.

If no message ID header, then we'll fall back to from/to/cc.
If that's still an empty string, we have too little information,
and we don't show status.

If the synth ID mismatches, we don't show status.

Only if those match again, we'll try to look at the date.
If date is missing, we accept the from/to/cc match and show the status.

Then the last fallback is the comparison of the header date.

Comment 27

[Kai Engert (:KaiE:)]

Attachment: 1558595-v6.patch

There was unnecessary code in getSyntheticID, removed in v6.

Comment 28

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:kaie:) from comment #25)

I think that "seems overcomplicated" shouldn't be a argument for rejecting
the solution. We're required to be certain when we deal with security
status, and if that takes complicated code, so be it.

I'm not rejecting it because it's complicated. I'm rejecting it because it seems overcomplicated and therefore prone to additional problems down the line.

Comment 29

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:kaie:) from comment #24)

Folder isn't unique if we have multiple accounts.

I compared the URI that we have inside MIME and
gFolderDisplay.selectedMessageUris[0], look very different.

MIME:
mailbox:///home/user/.thunderbird/[profilepath]/Mail/Local%20Folders/
folder4?number=1

gFolderDisplay.selectedMessageUris[0]:
mailbox-message://nobody@Local%20Folders/folder4#1

You need to get the neckourl

neckoURLForMessageURI("mailbox-message://nobody@Local%20Folders/folder4#1") will give you what mime knows about.
Then you have the uri, and the messageKey so that should be sufficient for comparisons.

Comment 30

[Magnus Melin [:mkmelin]]

https://searchfox.org/comm-central/rev/d504627d43a55b97fdb25bd121c4d7b2ab41e74e/mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js#22

Comment 31

[Kai Engert (:KaiE:)]

Magnus, that's great information. This allows for a simple solution.

Comment 32

[Kai Engert (:KaiE:)]

Attachment: 1558595-v8.patch

Comment 33

[Kai Engert (:KaiE:)]

Attachment: 1558595-v9.patch

fix line wraps

Comment 34

[Magnus Melin [:mkmelin]]

Comment on attachment 9073406 [details] [diff] [review]
1558595-v9.patch

Review of attachment 9073406 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thx! r=mkmelin

::: mail/extensions/smime/content/msgHdrViewSMIMEOverlay.js
@@ +34,5 @@
>    },
>  
> +  isStatusForSelectedMessage(aMsgNeckoURL) {
> +    try {
> +      let selKey = gFolderDisplay.selectedMessage.QueryInterface(Ci.nsIMsgDBHdr).messageKey;

I think you could instead also just do an 
if (gFolderDisplay.selectedMessage.folder) {
  return true;
}

... since .emls won't have a folder

Comment 35

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #34)

I think you could instead also just do an
if (gFolderDisplay.selectedMessage.folder) {
return true;
}
... since .emls won't have a folder

Thanks. I hope there isn't another scenario in which the folder is missing.
Obviously we want !folder

Comment 36

[Kai Engert (:KaiE:)]

Attachment: 1558595-v10.patch

Comment 37

[Kai Engert (:KaiE:)]

Attachment: 1558595-commit.patch

Comment 38

[Kai Engert (:KaiE:)]

Jörg, please use the commit message from the patch. Although this is a minor issue, I've used a slightly disguising commit message.

Comment 39

[Kai Engert (:KaiE:)]

Comment on attachment 9073511 [details] [diff] [review]
1558595-commit.patch

I suggest to uplift for correctness.

Comment 40

[Jorg K (CEST = GMT+2)]

Comment on attachment 9073511 [details] [diff] [review]
1558595-commit.patch

OK, thanks.

Comment 41

[Kai Engert (:KaiE:)]

I forgot to run eslint as usual. Will do that now.

Comment 42

[Kai Engert (:KaiE:)]

it passed.

Comment 43

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/75c5d68a9a21fd977c743cdd1561dc8735f22660
Improve S/MIME status display. r=mkmelin

Comment 44

[Jorg K (CEST = GMT+2)]

TB 68 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/5e71a311323ca64540b989c263ece1f8ca0a617e

Comment 45

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/c2eb3b629b42c7315b241707bf183151444e63df (reformat)
https://hg.mozilla.org/releases/comm-beta/rev/a9eccd0153e4c2364c265230b99c02d581de1e1a (reformat)