329990 - Unwind S/Mime signature verification from UI thread

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Kai Engert [:KaiE:]]

This bug depends on the changes in bug 111384.

Bug 111384 is about making OCSP work with proxies.
This is done by no longer the minimal http client contained in NSS, but use Necko for certificate verification.

This causes a deadlock, when OCSP is enabled, and rendering an S/Mime message, that has an OCSP responder service specificed.

The reason is that MIME rendering happens on the UI thread, and while we're doing that, it's not possible to call back into Necko.

I will attach a patch that unwinds S/Mime signature verification from message rendering. At the time we render the message, all data is collected that will be required to verify the signature. An asynchronous request gets started to run the verification on a separate thread (introduced in bug 111384). Once the verification is ready, the signature status will be set in the UI.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

I investigated what variables are required for delayed S/Mime signature verification.

In the old days of Netscape Communicator, signature and encryption status information was shown inside the message body area.
Links were added to a "security advisor" that could be used to access the message crypto details.

As we are now displaying that information in the mail application chrome, that code is no longer necessary.

I decided to remove this code, because it is mixed with message processing and accesses the MIME data variables. But I don't want to keep around the MIME objects until verification. It's not necessary.

In order to simplify the code required to fix this bug, this patch removes a lot of old code.

I will not yet request review, but wait until bug 111384 is reviewed.

Comment 2

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Now that I checked in the whitespace patch bug 329989, here is a clean patch against CVS.

Comment 3

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 214980 [details] [diff] [review]
Patch v3

I found a problem with this patch, when looking at nested messages in a separate window, where the outer message has a valid signature, it reports that signature for the inner (nested) message, even though the inner message does not have a signature.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Re: comment 4:  is that actually wrong?
(Note: this is not intended to be a rhetorical question.)
If we're examining a subset of a signed message, 
is it wrong to say that the subset was signed?
Clearly the window that shows the subset should be clear about WHO signed it.

Comment 6

[Kai Engert [:KaiE:]]

If the outer message is from A and signed by A, and the inner message was sent by B, and when viewing the message B in its own window (without indication it is an attached message), it is clearly wrong to say that message is signed by A.

Comment 7

[Kai Engert [:KaiE:]]

Note that as of today (without the patch), while message B is shown in its own window, where B was not signed, we do not show a signature. The patch should not change that behaviour.

Comment 8

[Kai Engert [:KaiE:]]

The problem I mentioned in comment 4 will be addressed in bug 332212.

Comment 9

[Kai Engert [:KaiE:]]

Attachment: Patch v4

Now that I received reviews for bug 111384, I'd like to ask for your review of this required follow up fix.
This patch includes the fix for the issue mentioned in comment 4, and explained in more detail in bug 332212.

Can you please have a look?
Thanks a lot.

Comment 10

[Scott MacGregor]

Comment on attachment 216842 [details] [diff] [review]
Patch v4

some comments:

+  if (!mHeaderSink)
+    return NS_OK;

should be 
NS_ENSURE_TRUE(mHeaderSink, NS_OK);

+  if (!aVerifiedMessage)
+    return NS_ERROR_FAILURE;

NS_ENSURE_TRUE(aVerifiedMessage, NS_ERROR_FAILURE)

ditto for

+  if (!msg)
+    return NS_ERROR_FAILURE;

We typically avoid putting braces around single line statements like:

+    if (NS_ERROR_MODULE_SECURITY == NS_ERROR_GET_MODULE(aVerificationResultCode)) {
+      signature_status = NS_ERROR_GET_CODE(aVerificationResultCode);
+    }

And we put the opening brace on the next line, but if what you are doing is the prevailing pattern for the smime libmime code, then feel free to leave it the way it is. 

in void MimeCMSGetFromSender there seems to be an extra set of braces around a block of code at the top of the function.

again 

can you double check the ref counting for listener in MimeCMSRequestAsyncSignatureVerification to make sure it doesn't get leaked.

thanks kaie!

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Patch v5

Thanks Scott.
I made the changes to use the NS_ENSURE_TRUE macro.

I removed several braces after if clauses with a single line statement.

I moved several opening braces to a separate line.

I removed both sets of unnecessary extra brackets.

> can you double check the ref counting for listener in
> MimeCMSRequestAsyncSignatureVerification to make sure it doesn't get leaked.

Thanks for asking me to check again, it does get leaked!
I fixed it by using nsRefPtr.

Comment 12

[Scott MacGregor]

Comment on attachment 216906 [details] [diff] [review]
Patch v5

thanks for making those changes.

Comment 13

[Kai Engert [:KaiE:]]

checked in

Checking in mimecms.cpp;
/cvsroot/mozilla/mailnews/mime/src/mimecms.cpp,v  <--  mimecms.cpp
new revision: 1.27; previous revision: 1.26
done
Checking in mimecms.h;
/cvsroot/mozilla/mailnews/mime/src/mimecms.h,v  <--  mimecms.h
new revision: 1.6; previous revision: 1.5
done
Checking in mimei.cpp;
/cvsroot/mozilla/mailnews/mime/src/mimei.cpp,v  <--  mimei.cpp
new revision: 1.79; previous revision: 1.78
done
Checking in mimei.h;
/cvsroot/mozilla/mailnews/mime/src/mimei.h,v  <--  mimei.h
new revision: 1.21; previous revision: 1.20
done
Checking in mimemcms.cpp;
/cvsroot/mozilla/mailnews/mime/src/mimemcms.cpp,v  <--  mimemcms.cpp
new revision: 1.23; previous revision: 1.22
done
Checking in mimemcms.h;
/cvsroot/mozilla/mailnews/mime/src/mimemcms.h,v  <--  mimemcms.h
new revision: 1.6; previous revision: 1.5
done

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 216906 [details] [diff] [review]
Patch v5

branch=shaver