S/MIME signature shown for incorrect message
Categories
(Thunderbird :: Security, defect)
Tracking
(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)
12.61 KB,
patch
|
KaiE
:
review+
jorgk-bmo
:
approval-comm-beta+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•5 years ago
|
||
fix
Assignee | ||
Comment 3•5 years ago
|
||
Thank you for the bug report and the patch.
Assignee | ||
Comment 4•5 years ago
|
||
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•5 years ago
|
||
We can't assume a message has a message id.
Assignee | ||
Comment 6•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 8•5 years ago
|
||
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•5 years ago
|
||
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.
Assignee | ||
Comment 10•5 years ago
|
||
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?
Updated•5 years ago
|
Comment 11•5 years ago
|
||
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
Assignee | ||
Comment 12•5 years ago
|
||
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•5 years ago
|
||
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
Assignee | ||
Comment 14•5 years ago
|
||
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.
Assignee | ||
Comment 15•5 years ago
|
||
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.
Assignee | ||
Comment 16•5 years ago
|
||
(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.
Assignee | ||
Comment 17•5 years ago
|
||
@@ +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
Assignee | ||
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
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)
Assignee | ||
Comment 20•5 years ago
|
||
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.
Assignee | ||
Comment 21•5 years ago
|
||
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.
Assignee | ||
Comment 22•5 years ago
|
||
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•5 years ago
|
||
Hmm, if we have a messaageKey we also have a folder. So messageKey+folder is unique at least. We could compare that.?
Assignee | ||
Comment 24•5 years ago
|
||
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
Assignee | ||
Comment 25•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 26•5 years ago
|
||
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.
Assignee | ||
Comment 27•5 years ago
|
||
There was unnecessary code in getSyntheticID, removed in v6.
Comment 28•5 years ago
|
||
(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•5 years ago
|
||
(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=1gFolderDisplay.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.
Updated•5 years ago
|
Comment 30•5 years ago
|
||
Assignee | ||
Comment 31•5 years ago
|
||
Magnus, that's great information. This allows for a simple solution.
Assignee | ||
Comment 32•5 years ago
|
||
Assignee | ||
Comment 33•5 years ago
|
||
fix line wraps
Comment 34•5 years ago
|
||
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
Assignee | ||
Comment 35•5 years ago
|
||
(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
Assignee | ||
Comment 36•5 years ago
|
||
Assignee | ||
Comment 37•5 years ago
|
||
Assignee | ||
Comment 38•5 years ago
|
||
Jörg, please use the commit message from the patch. Although this is a minor issue, I've used a slightly disguising commit message.
Assignee | ||
Comment 39•5 years ago
|
||
Comment on attachment 9073511 [details] [diff] [review] 1558595-commit.patch I suggest to uplift for correctness.
Comment 40•5 years ago
|
||
Comment on attachment 9073511 [details] [diff] [review] 1558595-commit.patch OK, thanks.
Assignee | ||
Comment 41•5 years ago
|
||
I forgot to run eslint as usual. Will do that now.
Assignee | ||
Comment 42•5 years ago
|
||
it passed.
Comment 43•5 years ago
|
||
https://hg.mozilla.org/comm-central/rev/75c5d68a9a21fd977c743cdd1561dc8735f22660
Improve S/MIME status display. r=mkmelin
Updated•5 years ago
|
Comment 44•5 years ago
|
||
TB 68 beta 3:
https://hg.mozilla.org/releases/comm-beta/rev/5e71a311323ca64540b989c263ece1f8ca0a617e
Updated•5 years ago
|
Comment 45•5 years ago
•
|
||
Updated•4 years ago
|
Description
•