Closed
Bug 119418
Opened 23 years ago
Closed 22 years ago
S/MIME opaque signed message with attachments displayed incorrectly.
Categories
(MailNews Core :: Security: S/MIME, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
psm2.2
People
(Reporter: daniel, Assigned: KaiE)
References
Details
(Keywords: crash, Whiteboard: [adt1] [fixed on trunk])
Attachments
(6 files, 5 obsolete files)
I generated an opaquely signed e-mail message with two attachments from Outlook 2002 version 10.3513.3501 SP-1 and viewed the message in Mozilla 0.9.7 (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.7) Gecko/20011221). Two problems: 1. The "Attachments:" list on the right shows three attachments where the third, "Part 1.1" is incorrect. 2. The message's security status shows "<Encrypted>" when it should be "<Signed>" I'll attach a screen shot and the original RFC 822 message. Note that I manually had to change the messages Content-type: from "application/pkcs7-mime" to "application/x-pkcs7-mine". I've created a separate defect report for that. Cheers, Daniel
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
Reporter | ||
Comment 3•23 years ago
|
||
I traced through the code and figured out why there is a third (Part 1.1) attachment. The function mimemoz2.cpp>>CountTotalMimeAttachments() is including the multipart/mixed contents (from the cryptographic signature construction) as an attachment, rather than skipping the cryptographic wrapping layer and going inside to the wrapped multipart/alternative inner content. Stack trace follows. In this case the attachment count is actually 4, not 2. I didn't look further to see if the defect was in the attachment counting algorithm (which performs memory allocations of attachment data) or the BuildAttachmentList() function which seems to have its own algorithm for skipping over inner content. One other thing to watch out for. While poking around inside the MimeContainer objects I noticed that if MimeContainer->children is 0 that MimeContainer->nchildren can be arbitrary values, as if it never gets initialized to 0 when the MimeContainer is first created. I hope this helps. CountTotalMimeAttachments(MimeContainer * 0x01fc4030) line 220 MimeGetAttachmentList(MimeObject * 0x01f974d0, const char * 0x01f97248, nsMsgAttachmentData * * 0x0012fd48) line 518 + 9 bytes mime_display_stream_complete(_nsMIMESession * 0x01f975d0) line 900 + 19 bytes nsStreamConverter::OnStopRequest(nsStreamConverter * const 0x01f96540, nsIRequest * 0x00000000, nsISupports * 0x00000000, unsigned int 0x00000000) line 1030 + 13 bytes ReceiveSMIMEMessage() line 252 + 40 bytes
Comment 4•23 years ago
|
||
->PSM S/MIME
Assignee: mstoltz → ssaux
Component: Security: General → S/MIME
Product: MailNews → PSM
QA Contact: junruh → alam
Version: other → 2.2
Comment 5•23 years ago
|
||
The first problem is already taken care by bug 115169.
Assignee | ||
Comment 7•23 years ago
|
||
I'm confirming the bug. The mime parser indeed handles this as an encrypted message, although it is not.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 8•23 years ago
|
||
RFC2311/2633 say, the mime types x-pkcs7-mime and pkcs7-mime are the same thing. However, we are currently looking only for one type. While looking for both mime types is an easy fix, it doesn't fix this bug - as you discovered by modifying the raw message. Currently our decryption engine seems to assume, it is an encrypted message when the x-pkcs7-mime type is seen. But RFC2311/2633 clearly define, that it can be also an encoded signed message, or even a message with certs only. We need to extend our parser.
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•23 years ago
|
||
*** Bug 117159 has been marked as a duplicate of this bug. ***
Comment 10•23 years ago
|
||
David, you want to read this bug.
Comment 11•23 years ago
|
||
While looking for the second mime type won't fix the display of attachments, it will at least make such emails readable. Would there be any problem with getting at least that part resolved? Right now, reading encrypted mail from some Outlook users requires a rollback to Communicator 4.79.
Comment 12•22 years ago
|
||
S/MIME bugs are automatically nsbeta1 candidates. (this is a bulk update - there may be some adjustment of the list).
Keywords: nsbeta1
Comment 13•22 years ago
|
||
kai, please decide on the nsbeta1+ status of this bug.
Comment 14•22 years ago
|
||
I fully second comment #11. I encounter messages I can't see in Mozilla because the type is pkcs7-mime instead of x-pkcs7-mime every day. If the severity of this bug is not high enough to treat it fast, I will open a second bug for this specific issue. Apparently, the code below is supposed to treat both cases as equal but it doesn't work : /mailnews/mime/cthandlers/smimestub/nsSMIMEStubFactory.cpp 117 { "MIME SMIMEStubed Mail Handler", NS_SMIME_CONTENT_TYPE_HANDLER_CID, "@mozilla.org/mimecth;1?type=application/x-pkcs7-mime", 118 nsSMimeMimeContentTypeHandlerConstructor, }, 119 120 { "MIME SMIMEStubed Mail Handler", NS_SMIME_CONTENT_TYPE_HANDLER_CID, "@mozilla.org/mimecth;1?type=application/pkcs7-mime", 121 nsSMimeMimeContentTypeHandlerConstructor, } Both /netwerk/mime/public/nsMimeTypes.h, line 85 #define APPLICATION_XPKCS7_MIME "application/x-pkcs7-mime" and /mailnews/mime/cthandlers/smimestub/nsSMIMEStub.h, line 46 #define SMIME_CONTENT_TYPE "application/x-pkcs7-mime" define a constant with the same value.
Comment 15•22 years ago
|
||
I did not see the end of the comment of the bug reporter : "I've created a separate defect report for that." This was bug 117159. But it's been marqued as a duplicate of this one bug. It had a detailled and very interesting report of where exactly the code is wrong. Please either solve this problem as part of 119418, or reopen 117159. Also this bug blocks bug 105774 .
Blocks: 105774
Assignee | ||
Comment 16•22 years ago
|
||
*** Bug 127819 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 17•22 years ago
|
||
*** Bug 105774 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 18•22 years ago
|
||
Stephane, my opinion is, this is definitively nsbeta1+, even if it needs new parser logic to be written.
Comment 19•22 years ago
|
||
As to the fact that Moz displays status as encrypted on the opaque signed messages, there is additional issue. If you click on the <encrypted> the mozilla will crash (M0.9.9 Linux). No talkback window appears (shame).
Comment 20•22 years ago
|
||
Now (Linux nightly 2002032717) it doesn't crash but it still displays encrypted status (key symbol) and doesn't display signed status (pen symbol). Even when I receive signed and encrypted message it displays that it is encrypted but not signed. I think that it should be addressed as soon as possible. We would like to help because we would like to use nsICMS and related interfaces in our mozilla based project but by looking at them we've found that they aren't completed (missing result checking and so on).
Keywords: mozilla1.0
Comment 21•22 years ago
|
||
So there are probably two problems: 1. application/pkcs7-mime isn't treated the same as application/x-pkcs7-mime 2. The opaque message isn't parsed well in either of the two cases. The x-pkcs7-mime messages are readable, but the signature isn't verified and the message is incorrectly identified as encrypted. I'll investigate the codepaths in the second problem later.
Updated•22 years ago
|
QA Contact: alam → carosendahl
Assignee | ||
Comment 22•22 years ago
|
||
I feared we need more mime parser logic, but now I think this does not seem to be necessary. WRT mime, it might be sufficient to extend the portions of mimemcms and mimcms code, that look at the status of the CMS message. With this patch, the attachments are no longer displayed as encrypted, but are correctly indicated as signed. However, we don't have code that verifies the signature, therefore the application currently indicates an invalid signature - nsCMSMessage::VerifySignature is not yet implemented. In addition we need to export one more function from NSS, see patch.
Assignee | ||
Comment 23•22 years ago
|
||
Verifying the signature was easier than I thought. I just cloned function VerifyDetachedSignature, but remove the call to manually set the digest. I tested this patch with sample messages, that I got from the reported problems, see duplicates of this bug. The verification reasons make sense, it succesfully verified a message where I trust the signer's root, does not verify where the root is not trusted, and I see a broken signature, if I manually modify a byte in raw opaque message. The sender's S/Mime cert arrive in self's encryption database. The only problem I see, the "message security info" window is unable to display the sender's cert, because nsCMS::GetSignerCert does not succeed. But I'd say this is a separate bug. I suggest we take this patch, because it adds supports for displaying message sent using the opaque type, and it fixes the issue that messages are wrongly displayed as encrypted.
Attachment #77793 -
Attachment is obsolete: true
Assignee | ||
Comment 24•22 years ago
|
||
Nominating [adt1], because it's a security issue and we have a patch.
Whiteboard: [adt1]
Comment 25•22 years ago
|
||
Comment on attachment 78267 [details] [diff] [review] Updated patch, now verifies signature Patch looks good to me. r=ddrinan.
Attachment #78267 -
Flags: review+
Comment 26•22 years ago
|
||
it's fine with me too. R=ducarroz
Comment 27•22 years ago
|
||
r=darin for the nsMimeTypes.h change
Assignee | ||
Comment 28•22 years ago
|
||
Alec, can you please superreview?
Comment 29•22 years ago
|
||
Assignee | ||
Comment 30•22 years ago
|
||
Comment on attachment 78267 [details] [diff] [review] Updated patch, now verifies signature Please hold off superreviewing, I found a problem with my patch. Sorry.
Attachment #78267 -
Attachment is obsolete: true
Assignee | ||
Comment 31•22 years ago
|
||
Sorry that I asked for review before I did excessive testing. I crashed with my patch trying to dereference a null pointer, if I did not have the decryption key for an encrypted message. I carefully rewrote the new logic to avoid that risk. After updating the patch, and testing all kind of messages and key existance scenarios, I think the patch is now correct. Unchanged are the portions to netwerk, so Darin's r= is still valid.
Attachment #78433 -
Attachment is obsolete: true
Assignee | ||
Comment 32•22 years ago
|
||
David, Jean-Francois, while I now have to ask you for review again, I'm trying to minimize the overhead. I was working on bug 122721 in parallel and that bug only requires a small change, and I have included it with this new patch. Unchanged are the previous changes to nsCMS.cpp, no need to review again. But I implemented two more functions in nsCMS.cpp, so please review nsCMSMessage::GetSignerEmailAddress nsCMSMessage::GetSignerCommonName They should be correct, because they mostly use logic that is already in use in GetSignerCert. Please review the changes to file mimecms.cpp again, it changed completely. In mimecms.cpp, the code to compare email header and certificate no longer retrieves the cert's common name, that is not necessary. In both mimemcms and mimcms, I fixed another bug, the actual and max wanted nesting levels used the wrong comparison operator... This did not cause a problem yet, because the comparison included the == case, and the called function checks the nesting level, too, and does it correctly. I updated my previous change to NSS with Wan-Teh's patch, and I give r=kaie on that change to NSS, because it works for me.
Comment 33•22 years ago
|
||
Comment on attachment 78482 [details] [diff] [review] Updated patch, has wtc's patch, has fix for 122721 Looks fine to me. I have just one request: can you share the common part of functions nsCMSMessage::GetSignerEmailAddress & nsCMSMessage::GetSignerCommonName instead of duplicating the code?
Assignee | ||
Comment 34•22 years ago
|
||
> Looks fine to me. I have just one request: > can you share the common part of functions nsCMSMessage::GetSignerEmailAddress > & nsCMSMessage::GetSignerCommonName instead of duplicating the code? Good idea. The same code was even used in additional locations, I made them all call the same common function. I also created a shared implementation for the signature verification, as the new function was mostly identical to the existing one.
Attachment #78482 -
Attachment is obsolete: true
Comment 35•22 years ago
|
||
Sorry but to correctly resolve the bug it would also be necessary to fix the NSS functions which look for the cert in the opaque signed data. Because even with the latest patch if you don't have the sender's certificate in the Other people database it shows broken pen and that the message couldn't be verified. (Unknown problems....1028) This would certainly fix the GetSignerCert too. I think it shouldn't be too hard to fix it...investigating.
Assignee | ||
Comment 36•22 years ago
|
||
I agree this patch is not the end of the story. I already mentioned the problem in comment #23, and it is not yet fixed. But this patch fixes the most serious issue, in my opinion. It makes sure that messages, that have only been signed, will not be incorrectly labeled as being encrypted. I think this kind of correctness is a number 1 priority, to make sure people trust the application, and we don't make the people feel safe, where they should not feel safe. This patch is the best I currently have, and because of the limited time until 1.0, I'd rather land this patch than leaving the current situation. Fixing the ability to verify the signature can be done in the next step.
Assignee | ||
Comment 37•22 years ago
|
||
We have an additional option, for discussion. In one of the duplicates of this bug I said, because we currently only display messages having application/x-pkcs7-mime, we avoid displaying opaque signed messages, and are thereby avoiding displaying messages as being encrypted, although they are only signed. While that seems to be true for messages sent from recent versions of OE (which uses application/pkcs7-mime), I have learned that my assumption is not 100% true. In the newsgroups I saw complaints, that Mozilla is already displaying signed messages as being encrypted. I draw the conclusion that there must be some S/Mime clients, that do send opaque signed message using application/x-pkcs7-mime, which cause the wrong display. As you noted, this patch is not yet perfect, as it has problems verifying some messages - although the worst that happens, an error is shown that the signature is invalid. The option we have: If we wanted to avoid confusion, we could leave support for application/pkcs7-mime still disabled for now, and only use the portions of the fix that make the encryption bug go away. That would mean, opaque signed messages from other recent S/Mime applications are still not displayed at all with Mozilla, and we avoid displaying incorrect signature status, until we have implemented that completely. That's just another option we have, but personally I'd say, ignore this comment and let's use the patch.
Assignee | ||
Comment 38•22 years ago
|
||
I have provided test builds with this patch, for both Linux and Windows, downloadable at http://www.kuix.de/mozilla/bug119418/ . Please help testing and give feedback here in the bug, to raise chances that this fix will become accepted for inclusion with 1.0. We need to confirm: - no crashes when using S/Mime - no regressions with S/Mime messages sent from Mozilla to Mozilla - messages sent from OE are readable with Mozilla (if you can, please test various versions of OE, old and new) - signed only S/Mime messages from OE are never indicated as being encrypted, although they might be displayed as having an invalid signature. (please test messages with and without attachments) Thanks!
Comment 39•22 years ago
|
||
Comment on attachment 78522 [details] [diff] [review] Updated patch, does what ducarroz requested r=ddrinan
Attachment #78522 -
Flags: review+
Reporter | ||
Comment 40•22 years ago
|
||
I ran Kai's test build through its paces, using the following test matrix: + Incoming messages: clear signed, opaque signed, clear signed and encrypted, opaque signed and encrypted, encrypted. All the previous with attachments. + Outgoing messages: same set as incoming messages. Problems ======== I found two problems. 1. The signatory is not displayed for opaque signed message, only clear signed messages. The same is true for encrypted and signed (clear or opaque). I've attached two screen shots, opaqueSigned.jpg and clearSigned.jpg that show the differences. 2. To test encryption I imported a PKCS #12 key pair from a Windows XP box. The import succeeded but the key pair was labeled "Other People's" rather than one of mine. See the pkcs12Import.jpg image for a screen shot. The screen shot shows that the private key is present since I successfully decrypted an incoming encrypted message. I could not test outgoing S/MIME messages because I could not configure a default signing and encryption certificate. See the security.jpg image. Note the disabled buttons. I'm guessing this is due to the errant PKCS #12 import? I hope this helps. Daniel
Reporter | ||
Comment 41•22 years ago
|
||
Reporter | ||
Comment 42•22 years ago
|
||
Reporter | ||
Comment 43•22 years ago
|
||
Assignee | ||
Comment 44•22 years ago
|
||
Daniel, thanks for your help! (I noticed that server with test builds was offline, it should be back online now. Ping me if you can't access.) > 1. The signatory is not displayed for opaque signed message, only > clear signed messages. The same is true for encrypted and signed > (clear or opaque). I've attached two screen shots, opaqueSigned.jpg > and clearSigned.jpg that show the differences. The difference I see is, in one message you are able to view the details of the signer cert, in the other message, you are not. This is a known issue, not yet solved, and must be addressed with a separate bug, I'll open it. But in both cases, you seem to be able to successfully read the message, that's good. > 2. To test encryption I imported a PKCS #12 key pair from a > Windows XP box. The import succeeded but the key pair was labeled > "Other People's" rather than one of mine. See the pkcs12Import.jpg > image for a screen shot. > > The screen shot shows that the private key is present since I > successfully decrypted an incoming encrypted message. > > I could not test outgoing S/MIME messages because I could not > configure a default signing and encryption certificate. See > the security.jpg image. Note the disabled buttons. I'm guessing > this is due to the errant PKCS #12 import? That's interesting. Indeed, it is a bug that your cert appears in the wrong tab. I didn't tell you, but this test build actually includes another patch, the one in bug 128586. I think what you see is an effect caused by that bug/patch, but not a problem with the patch from this bug. You do not have other p12 files for testing, do you? If these are the only issues you found, that makes me confident that the patch works. If anybody else can help testing, please do.
Assignee | ||
Comment 45•22 years ago
|
||
Alec, now it's time to ask again, can you please superreview the patch? I filed bug 136791, to track the new issue, that extracting certs from opaque signed messages does not work yet.
Comment 46•22 years ago
|
||
I've verified that clear and opaque signed messages, some combined with encrytpion, are viewable within mozilla. I've also verified replies and forwarding of these messages are correct in form in both moz and OE. - Attachments (and list) show up correctly - Messages appear as signed when signed amd sign/encrypted (icons are correct) Other bugs are in separate defect reports.
Comment 47•22 years ago
|
||
I've verified various possibilities (but only with messages from Mozilla and Outlook 2000) and it works fine with the latest patch except the bug 136791. I've found and reported another bug 136814, but this one isn't connected to the patch.
Assignee | ||
Comment 48•22 years ago
|
||
I found the solution for bug 136791! The previous patch tried to retrieve the cert from the NSS data structure, before we called the signature verification code. Simply moving that call after the verification step, and bug 136791 is fixed. The reason is that the verification signature code includes a call to import the certificates from the message. After that has been executed, retrieving the signer's cert from the NSS data structures succeeds.
Attachment #78522 -
Attachment is obsolete: true
Comment 49•22 years ago
|
||
Good work! I've just tried your patch and it works fine in all circumstances I've tried the previous patch with and the certificate is now viewable.
Assignee | ||
Comment 50•22 years ago
|
||
I'm currently uploading an updated test build to http://www.kuix.de/mozilla/bug119418/ . It should be up in about 20 minutes. It will contain the latest patch from this bug. It will no longer contain the patch for bug 128586.
Comment 51•22 years ago
|
||
Can you land this on the trunk today, if possible. The sooner the better.
Keywords: nsdogfood
Comment 52•22 years ago
|
||
Comment on attachment 78697 [details] [diff] [review] Updated patch, slightly rearranged code sr=alecf
Attachment #78697 -
Flags: superreview+
Assignee | ||
Comment 53•22 years ago
|
||
Checked in to the trunk. Leaving open for branch landing.
Comment 54•22 years ago
|
||
If this is fixed on the trunk, then you can mark it as fixed. Once it has landed on the 1.0 branch, pls add the keyword fixed1.0.0, so we know it made it there, too.
Whiteboard: [adt1] → [adt1] [fixed on trunk]
Comment 55•22 years ago
|
||
Please update this bug when it has been tested.
Assignee | ||
Comment 56•22 years ago
|
||
Marking fixed, needs to be verified, in preparation for branch landing.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 57•22 years ago
|
||
*** Bug 135536 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 58•22 years ago
|
||
*** Bug 105474 has been marked as a duplicate of this bug. ***
Comment 59•22 years ago
|
||
Verified fixed with 2002041503. I will include the message I used for the test as an attachment as I could not see any usable one in the previous attachments. Encrypted messages from OE are also OK. But I'm sightly annoyed by the patch. It implements a function nsCMSMessage::GetSignerEmailAddress that assumes that a certificate can only have one email adress. This is not true, and a certificate can have several adresses that will be stored in the alternative name extension. Whilst it's not very much used yet, they are use scenario for certificates where it's very useful to have a client mail application that suppports this.
Assignee | ||
Comment 60•22 years ago
|
||
Thanks for verifying. > It implements a function nsCMSMessage::GetSignerEmailAddress that assumes that a > certificate can only have one email adress. > This is not true, and a certificate can have several adresses that will be > stored in the alternative name extension. > > Whilst it's not very much used yet, they are use scenario for certificates where > it's very useful to have a client mail application that suppports this. Right, I agree, and I would appreciate that feature too. But right now, we only support the first email address listed in the cert. There is already bug 50823 which tracks that feature request. I think it is a separate task to implement bug 50823, because it might need many changes across the security codebase - but that's just a guess.
Keywords: adt1.0.0+
Comment 61•22 years ago
|
||
Agree with Kai's last comment - the multiple email address support (Thawte for instance) is a separate bug that needs to be resolved most likely in a point release. Verified fixed in the 0416 trunk build. Signed/encrypted, opaque/non-opaque combinatorics used to confirm correct functionality. Please add 'fixed' to the keyword list after this defect lands on the branch.
Status: RESOLVED → VERIFIED
Comment 62•22 years ago
|
||
only the adt can add adt1.0.0+ on a bug. pls wait for our approval, should come shortly. thanks!
Assignee | ||
Comment 63•22 years ago
|
||
> only the adt can add adt1.0.0+ on a bug. pls wait for our approval, should come
> shortly. thanks!
Argh sorry, my intention was to add the keyword with the plus :(
Assignee | ||
Comment 64•22 years ago
|
||
Please ignore the previous comment. What I really wanted to say was: "Actually, my intention was to add the keyword WITHOUT the plus." To summarize, Jaime, thanks for correcting my mistake! (comment proofread this time)
Comment 65•22 years ago
|
||
adt1.0.0+ (on ADT's beahlf) for checkin into the 1.0 branch. Pls check this in today, and add fixed1.0.0. Once, QA has verified the fix on the branch, pls add the verified 1.0.0 keyword. thanks again kaie!
Comment 66•22 years ago
|
||
Comment on attachment 78697 [details] [diff] [review] Updated patch, slightly rearranged code a=rjesup@wgate.com for branch checkin. Marking previously gibven r='s
Attachment #78697 -
Flags: review+
Attachment #78697 -
Flags: approval+
Assignee | ||
Comment 68•22 years ago
|
||
*** Bug 133213 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Keywords: fixed1.0.0 → verified1.0.0
You need to log in
before you can comment on or make changes to this bug.
Description
•