386313 - ESS: Signed Receipts (RFC2634 - 2)

Status: NEW

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

Description

[BALLET BAZ Eric]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; fr; rv:1.8.1.2) Gecko/20070219 Firefox/2.0.0.2
Build Identifier: version 2.0.0.0 (20070326)

This patch implements the signed receipt functionnality (RFC2634 - 2
http://tools.ietf.org/html/rfc2634#section-2)

This is the part of the following feature list:
http://www.mozilla.org/projects/security/pki/nss/smime/

and has been developped for the milimail project :
http://www.milimail.org/milimailfr/index.php/Triple_wrapping_user

This patch is based on the sources THUNDERBIRD_2_0_0_0_RELEASE


Reproducible: Always

Comment 1

[BALLET BAZ Eric]

Attachment: Signed Receipt patch v0.1

Comment 2

[BALLET BAZ Eric]

Attachment: Screenshots showing the modified UI screens

Comment 3

[David :Bienvenu]

Cool - I think you're going to need Kaie's review for the nss stuff, and the s/mime stuff as well - is that right, Kaie? But first, I think we need a trunk patch.

For the trunk, I have similar comments as for the dsn patch - you need to use nsIFile instead of nsIFileSpec. Instead of nsXPIDL(C)String, you should just use ns(C)String, since we're getting rid of xpidlstrings.

+NS_IMETHODIMP nsMsgSMIMEComposeFields::GetSignedReceiptRequest(PRBool *_retval)
+{
+  *_retval = mSignedReceiptRequest;
+  return NS_OK;
+}

We need an NS_ENSURE_ARG_POINTER(_retval); 

+      if (smimeCompFields)
+      {
+        smimeCompFields->GetSignedReceiptRequest(aSignedReceiptRequest);
+        return NS_OK;
+      }

can this just be:

if (smimeCompFields)
  return smimeCompFields->GetSignedReceiptRequest(aSignedReceiptRequest);

+  if (signedReceiptRequest) {
+    aIdentity->GetEmail(getter_Copies(mSignedReceiptRequest_ReceiptTo));
+  }

don't need extra braces, unless Kaie likes them.

+nsMsgSignedReceiptRequestGenerator::nsMsgSignedReceiptRequestGenerator()
+{
+    m_outputStream = nsnull;
+}

you don't need to init comptrs

+    rv = smtpService->SendMailMessage(m_fileSpec, m_to, m_identity, nsnull, this, nsnull, nsnull, nsnull, getter_AddRefs(aRequest));
+    
+    return rv;

can just return directly, no need for rv here.

+    // Create Signed Message
+    m_secureCompose = do_CreateInstance(NS_MSGCOMPOSESECURE_CONTRACTID, &rv);
+    if (NS_FAILED(rv))
+        return rv;

for things like this, that we don't expect to fail, we use NS_ENSURE_SUCCESS(rv, rv); so that we'll get a warning on the console if it happens, in debug builds.

+      if (params.isSigningCertAvailable)
+        signedReceiptRequest_element.value = yes_string;
+      else
+        signedReceiptRequest_element.value = not_possible_string;

can just use the ? operator here.

On the trunk, as part of our string cleanup, we're using ACString in idl where appropriate:

A larger question - a lot of this code is copied from nsMsgMDNGenerator.cpp (which should at least make it easier for you to see how to get rid of nsIFileSpec, since that work is done on the trunk). But I wonder if there's some way to share the code, perhaps inherit from nsMsgMDNGenerator. We'd need to figure out how to build it - perhaps it could be in the extensions/mdn directory, but only get built when S/MIME is built (I think we always build S/MIME anyway).

+  signedReceiptRequestGenerator.process(msgWindow, msgFolder, msgHdr.messageKey, mimeHdr, aSignedContentIdentifier, aReceiptsFrom, aReceiptsTo);
+
+  // Reset mark msg MDN "Sent" and "Not Needed".
+  msgHdr.OrFlags(MSG_FLAG_SIGNED_RECEIPT_SENT);
+

This worries me a little - if you go directly through the msgHdr, you're not storing the flag on the server, for imap, or in the x-mozilla-status headers, for local mail.

Do you really need a special msg flag for signed_receipt_sent, in addition to the one for return receipt sent?

That's probably enough comments for now...thx for the patch!

Comment 4

[David :Bienvenu]

Comment on attachment 270313 [details] [diff] [review]
Signed Receipt patch v0.1

Asking Kaie for a review of the smime and nss stuff...

Comment 5

[zug_treno]

Thunderbird bug 279352, Thunderbird bug 368775 and bug 293981 also mention s/mime receipts, are those duplicates of this bug?

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Considering the reaction and response that bug 295922 got, I am VERY confident
that, if we add the signed return receipt feature to mozilla mail/news, we 
MUST absolutely make it something that the user can override and disable.

Any other choice would make sending a signed message to someone an invasion
of the recipient's privacy, a way to probe for live mailboxes, whether the 
user of that mail box wanted to reveal his existence or not. 

If the patch attached to this bug does not provide a way for the user to 
disable the automatic sending of signed replies, then it must be denied
until that is fixed.  I have not examined this patch to see what it does
about that.

I suggest the simplest thing to do is to use the very same preference that 
now exists for sending return receipts as the pref for signed return receipts.
Just treat signed and unsigned return receipts exactly the same.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Dan, I don't know if the Thunderbird group is looking at core S/MIME bugs
and RFEs, but there are a number of them that have significant patches
and are awaiting some mail developer to review/approve them.  
This is one of them.

Comment 9

[David :Bienvenu]

I believe Kaie is the module owner for this code, but I'm not sure I'd call him a mail developer. If Kaie can't review this, we might need to figure out a new way of getting it reviewed.

Comment 11

[Raphaël Fairise]

Current attached patch (v0.1) is not up-to-date.

I've worked on this feature last month and fixed the structure of the receipt request.
As for now, only receipt request is correctly implemented. Response consists of a simple signed message (without RFC 2634 "receipt" attribute in the signature).

It now also provides a way for the user to disable the automatic sending of the response or ask with a pop-up dialog before sending, based on a user preference.

It's currently based on Thunderbird 2 (not ported yet to Thunderbird 3).
You can test it with our build of Thunderbird called Trustedbird:
http://www.trustedbird.org/tb/Trustedbird

Trustedbird also includes RFC 2634 Triple Wrapping and Security Labels.

Screenshots:
http://www.trustedbird.org/tb/Signed_Receipt

Sources/SVN:
http://adullact.net/plugins/scmsvn/viewcvs.php/trunk/mozilla/?root=milimail

Trustedbird/Milimail project

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 270313 [details] [diff] [review]
Signed Receipt patch v0.1

This is mailnews code.  It needs review by mailnews folks.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 270314 [details]
Screenshots showing the modified UI screens

This actually needs ui-review first; requesting.

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 270314 [details]
Screenshots showing the modified UI screens

thanks again for the screenshots!

I'll go through each one to cover some text / nits.  I think the larger question in my mind is coming from comment #7 and how to prevent remote content leaking information out.

SignedReceipt_Account_en.gif

The checkbox seems fine.  Can you explain how this interacts with our existing return receipt system?  I'm assuming it doesn't turn on regular return receipts.

Also I saw that Trustedbird has the [ Ask Me | v ] option for what to do with return receipts.  Is that included in this patch even though it's not in the screenshots?

SignedReceipt_Info_en.gif

Looks fine.

SignedReceipt_Options1_en.gif
SignedReceipt_Options2_en.gif

The wording on both of these seems a little off.  I think it should be more action oriented, maybe: "Request Signed Return Receipt" or "Request Signed Receipt" 

I need to know more about the general interactions this creates.  I couldn't find more at the Trustedbird site regarding what happens when you receive a signed receipt request and other such parts.

I'll minus this for now as it doesn't seem ready yet but I'll continue reviewing when there's more information.  Thanks.

Comment 15

[Raphaël Fairise]

(In reply to comment #14)
> I'll go through each one to cover some text / nits.  I think the larger
> question in my mind is coming from comment #7 and how to prevent remote content
> leaking information out.

As I said in comment #11, attached screenshots and patchs are two years old and obsolete.
There is now an option to choose what you want to do when a request is received.
I've updated screenshots here:
http://www.trustedbird.org/tb/Signed_Receipts

> The checkbox seems fine.  Can you explain how this interacts with our existing
> return receipt system?  I'm assuming it doesn't turn on regular return
> receipts.

Yes, it's completely independent from existing return receipt system.

Comment 16

[Ludovic Hirlimann [:Usul]]

Bryan are you happier with the UI in comment 15 ?

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 270313 [details] [diff] [review]
Signed Receipt patch v0.1

As per comment 15, these patches are obsolete.  Removing review requests.

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #16)
> Bryan are you happier with the UI in comment 15 ?

I just looked over the screenshots at trustedbird
http://www.trustedbird.org/tb/Signed_Receipts

Everything looks pretty good, the only one I had issue with was the following dialog
http://www.trustedbird.org/tb/File:SignedReceipt3.png

We're using a notification bar for this type of question in regular return receipts and so I think it's fitting to use it here; not forgetting that they are a lot nicer user experience than the dialogs.

I'd be happy to go into further reviewing when it's available.

Comment 19

[Raphaël Fairise]

Thanks Bryan.
I'm working on a new patch for S/MIME receipts for comm-central (and yes, using the great notification bar :-).

Comment 20

[Raphaël Fairise]

Here is a patch for comm-central which provides RFC 2634 S/MIME Receipts for Thunderbird and Seamonkey.

* User can request for a signed receipt when sending a signed S/MIME message.
* Thunderbird understands S/MIME signatures with a receipt request.
* It can generate and send a S/MIME receipt message in response (the user can choose not to send the receipt).
* Thunderbird can read and display S/MIME receipt messages (new content-type in CMS SignedData) and verify the data with a sent message.

The patch is split in 3 parts:
* patch-smime-receipts-backend.diff (for comm-central)
* patch-smime-receipts-ui.diff (for comm-central)
* patch-smime-receipts-nss.diff (for mozilla-central)

(These patches have been updated with comm-central as of 2010/09/01)

Screenshots are available here (in Trustedbird 3.1 section):
http://www.trustedbird.org/tb/Signed_Receipts

You can test this feature in Thunderbird 3.1 by downloading our test build Trustedbird:
http://www.trustedbird.org/tb/Trustedbird


What needs to be watched carefully:
* memory management in NSS: I'm not sure to have understood everything about memory allocation/freeing with arena pools.
* security/nss/lib/smime/cmsdecode.c: line 545, I removed an "else" in order to decode SignedData with content-type=receipt.
* security/nss/lib/smime/cmsasn1.c: line 102, SEC_ASN1_XTRN replaced with SEC_ASN1_DYNAMIC in order to be able to attach the receipt ContentInfo.

Known limitations:
* in a receipt request, only the first recipient listed in the request object is taken into account to send the receipt.
* when a receipt is read in Thunderbird, information are extracted and saved into message db. The verification with the original (sent) message is only possible if message has also been read once in order to decode its signature. I haven't found a way to decode a S/MIME message without displaying it.

Comment 21

[Raphaël Fairise]

Attachment: patch-smime-receipts-backend.diff

Comment 22

[Raphaël Fairise]

Attachment: patch-smime-receipts-ui.diff

Comment 23

[Raphaël Fairise]

Attachment: patch-smime-receipts-nss.diff

Comment 24

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 471133 [details] [diff] [review]
patch-smime-receipts-ui.diff

Doing my ui-review based off the screenshots currently located at: http://www.trustedbird.org/tb/Signed_Receipts

* Compose Window
** Looks good, simple and straight forward

* Reception of a receipt request
** The text is long, but I see that you've taken our regular return receipt text and added "signed".  I don't really have a suggestion to shorten the text without changing the UI.

* Reception of a receipt 
** This part looks pretty good although I think it would make more sense for the message body to say this instead of using a notification bar but I can understand that it's easier to use the notification bar.

* Settings
** Looks good, simple and similar

Comment 25

[Ludovic Hirlimann [:Usul]]

kaie review ping

Comment 26

[Nikolay Shopik]

kaie review ping
wtc review ping

Comment 27

[Kai Engert [:KaiE:]]

Comment on attachment 471132 [details] [diff] [review]
patch-smime-receipts-backend.diff

sorry, I never found time to review new features, I was fully busy with the problems in our existing code. I don't think this will change any time soon.

Furthermore, nowadays, any addition of new features would have to come with automated tests by policy.

Comment 28

[Ludovic Hirlimann [:Usul]]

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

> Furthermore, nowadays, any addition of new features would have to come with
> automated tests by policy.

Is this documented somewhere ?

Comment 29

[Kai Engert [:KaiE:]]

> Is this documented somewhere ?

That's what johnath and bsmith said.
I don't know if it applies to TB features, too, but it seems to be true for everything Firefox.

Comment 30

[Magnus Melin [:mkmelin]]

The "add tests for new features" policy applies to tb too afaik, though its not enforced that strictly.

Comment 31

[Wayne Mery (:wsmwk)]

Raphael is unable to continue with the patches

Comment 32

[magist3r]

Hi all. I can update patches and write unit tests if someone tells me where to start.

Comment 33

[Wayne Mery (:wsmwk)]

(In reply to magist3r from comment #32)
> Hi all. I can update patches and write unit tests if someone tells me where
> to start.

Comment 34

[Magnus Melin [:mkmelin]]

AFAIK, s/mime don't really have any tests yet unfortunately... 
(In general, see https://developer.mozilla.org/en-US/docs/MailNews_xpcshell-tests)

Comment 35

[Joshua Cranmer [:jcranmer]]

I did write an S/MIME unit test in a patch, but I haven't gotten around to completing it since I wasn't sure how to handle certificate generation, since I'm not enamored with checking in a unit test that will fail by itself in N months.

(<http://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/c3e0beb90186/patches-newcompose/test-smime> is that work in progress; the most useful part perhaps is the code that knows how to import a certificate store, which I used to make <https://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/test/unit/test_smime_mimemsg_representation.js#16> work properly).

Comment 36

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 37

[Wayne Mery (:wsmwk)]

magist3r?  Does comment 35 help you?

(In reply to magist3r from comment #32)
> Hi all. I can update patches and write unit tests if someone tells me where
> to start.

(In reply to Joshua Cranmer [:jcranmer] from comment #35)
>...
> (<http://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/
> c3e0beb90186/patches-newcompose/test-smime> is that work in progress; the
> most useful part perhaps is the code that knows how to import a certificate
> store, which I used to make
> <https://dxr.mozilla.org/comm-central/source/mailnews/db/gloda/test/unit/
> test_smime_mimemsg_representation.js#16> work properly).

Comment 38

[Wayne Mery (:wsmwk)]

Kai, this bug and bug 598282 both have reviews pending for WTC. I see you were a contributor to attachment 471134 [details] [diff] [review]