Closed Bug 1558595 Opened 1 year ago Closed 1 year ago

S/MIME signature shown for incorrect message

Categories

(Thunderbird :: Security, defect)

defect
Not set
normal

Tracking

(thunderbird68 fixed, thunderbird69 fixed)

RESOLVED FIXED
Thunderbird 69.0
Tracking Status
thunderbird68 --- fixed
thunderbird69 --- fixed

People

(Reporter: kenny.ashford.bug.moz, Assigned: KaiE)

References

Details

(Keywords: csectype-spoof, sec-low)

Attachments

(1 file, 8 obsolete files)

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.

Attached patch fix-signature-bug.diff (obsolete) — Splinter Review

fix

Duplicate of this bug: 1518047

Thank you for the bug report and the patch.

Status: UNCONFIRMED → NEW
Ever confirmed: true

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.

Flags: needinfo?(vseerror)

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

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?
Attachment #9071376 - Flags: review-

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.

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?

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.

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?

Flags: needinfo?(vseerror)

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

Attached patch 1558595-v3.patch (obsolete) — Splinter Review

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.

Assignee: nobody → kaie
Attachment #9072706 - Flags: review?(mkmelin+mozilla)
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
Attachment #9072706 - Flags: review?(mkmelin+mozilla)

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.

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.

(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.

@@ +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

Attached patch 1558595-v4.patch (obsolete) — Splinter Review
Attachment #9071376 - Attachment is obsolete: true
Attachment #9072706 - Attachment is obsolete: true
Attachment #9072891 - Flags: review?(mkmelin+mozilla)
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)
Attachment #9072891 - Flags: review?(mkmelin+mozilla)

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.

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.

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.

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

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

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.

Attachment #9072891 - Attachment is obsolete: true
Attached patch 1558595-v5.patch (obsolete) — Splinter Review

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.

Attachment #9073196 - Flags: review?(mkmelin+mozilla)
Attached patch 1558595-v6.patch (obsolete) — Splinter Review

There was unnecessary code in getSyntheticID, removed in v6.

Attachment #9073196 - Attachment is obsolete: true
Attachment #9073196 - Flags: review?(mkmelin+mozilla)
Attachment #9073197 - Flags: review?(mkmelin+mozilla)

(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.

(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.

Status: NEW → ASSIGNED

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

Attached patch 1558595-v8.patch (obsolete) — Splinter Review
Attachment #9073197 - Attachment is obsolete: true
Attachment #9073197 - Flags: review?(mkmelin+mozilla)
Attachment #9073405 - Flags: review?(mkmelin+mozilla)
Attached patch 1558595-v9.patch (obsolete) — Splinter Review

fix line wraps

Attachment #9073405 - Attachment is obsolete: true
Attachment #9073405 - Flags: review?(mkmelin+mozilla)
Attachment #9073406 - Flags: review?(mkmelin+mozilla)
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
Attachment #9073406 - Flags: review?(mkmelin+mozilla) → review+

(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

Attached patch 1558595-v10.patch (obsolete) — Splinter Review
Attachment #9073406 - Attachment is obsolete: true
Attachment #9073510 - Flags: review+
Attachment #9073510 - Attachment is obsolete: true
Attachment #9073511 - Flags: review+

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

Keywords: checkin-needed
Comment on attachment 9073511 [details] [diff] [review]
1558595-commit.patch

I suggest to uplift for correctness.
Attachment #9073511 - Flags: approval-comm-beta?
Comment on attachment 9073511 [details] [diff] [review]
1558595-commit.patch

OK, thanks.
Attachment #9073511 - Flags: approval-comm-beta? → approval-comm-beta+

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

it passed.

Target Milestone: --- → Thunderbird 69.0
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Group: mail-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.