ESS: Signed Receipts (RFC2634 - 2)

NEW
Unassigned

Status

--
enhancement
11 years ago
3 years ago

People

(Reporter: eballetbaz, Unassigned, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patchlove][needs new assignee][psm-smime])

Attachments

(5 attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 1

11 years ago
Created attachment 270313 [details] [diff] [review]
Signed Receipt patch v0.1
(Reporter)

Comment 2

11 years ago
Created attachment 270314 [details]
Screenshots showing the modified UI screens
(Reporter)

Updated

11 years ago
Version: unspecified → 1.8 Branch

Comment 3

11 years ago
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!
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 4

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

Asking Kaie for a review of the smime and nss stuff...
Attachment #270313 - Flags: review?(kengert)

Comment 5

11 years ago
Thunderbird bug 279352, Thunderbird bug 368775 and bug 293981 also mention s/mime receipts, are those duplicates of this bug?
Duplicate of this bug: 293981
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.
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

10 years ago
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.

Updated

10 years ago
Component: Security: S/MIME → Security: S/MIME
Product: Core → MailNews Core
Duplicate of this bug: 368775
Whiteboard: [needs review Kaie]

Comment 11

9 years ago
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 on attachment 270313 [details] [diff] [review]
Signed Receipt patch v0.1

This is mailnews code.  It needs review by mailnews folks.
Attachment #270313 - Flags: review?(dmose)
Comment on attachment 270314 [details]
Screenshots showing the modified UI screens

This actually needs ui-review first; requesting.
Attachment #270314 - Flags: ui-review?(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.
Attachment #270314 - Flags: ui-review?(clarkbw) → ui-review-

Comment 15

9 years ago
(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.
Bryan are you happier with the UI in comment 15 ?
Attachment #270313 - Flags: review?(kaie)
Attachment #270313 - Flags: review?(dmose)
Comment on attachment 270313 [details] [diff] [review]
Signed Receipt patch v0.1

As per comment 15, these patches are obsolete.  Removing review requests.
Whiteboard: [needs review Kaie]

Updated

8 years ago
Assignee: kaie → nobody
Whiteboard: [psm-smime]
(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

8 years ago
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

8 years ago
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

8 years ago
Created attachment 471132 [details] [diff] [review]
patch-smime-receipts-backend.diff

Comment 22

8 years ago
Created attachment 471133 [details] [diff] [review]
patch-smime-receipts-ui.diff

Comment 23

8 years ago
Created attachment 471134 [details] [diff] [review]
patch-smime-receipts-nss.diff
Assignee: nobody → raphael.fairise.bugs
Status: NEW → ASSIGNED
Version: 1.8 Branch → Trunk
Attachment #471133 - Flags: ui-review?(clarkbw)
Attachment #471132 - Flags: review?(kaie)
Attachment #471134 - Flags: review?(wtc)
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
Attachment #471133 - Flags: ui-review?(clarkbw) → ui-review+
kaie review ping
Blocks: 279352

Comment 26

8 years ago
kaie review ping
wtc review ping

Comment 27

6 years ago
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.
Attachment #471132 - Flags: review?(kaie)

Updated

6 years ago
Attachment #471132 - Flags: review?
(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

6 years ago
> 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.
The "add tests for new features" policy applies to tb too afaik, though its not enforced that strictly.

Comment 31

6 years ago
Raphael is unable to continue with the patches
Assignee: raphael.fairise.bugs → nobody
See Also: → bug 598282
Whiteboard: [psm-smime] → [patchlove][needs new assignee][psm-smime]

Updated

6 years ago
Status: ASSIGNED → NEW
Attachment #471132 - Flags: review?

Comment 32

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

Comment 33

3 years ago
(In reply to magist3r from comment #32)
> Hi all. I can update patches and write unit tests if someone tells me where
> to start.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(Pidgeot18)
AFAIK, s/mime don't really have any tests yet unfortunately... 
(In general, see https://developer.mozilla.org/en-US/docs/MailNews_xpcshell-tests)
Flags: needinfo?(mkmelin+mozilla)
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).
Flags: needinfo?(Pidgeot18)
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

3 years ago
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).
Flags: needinfo?(magist3r)
You need to log in before you can comment on or make changes to this bug.