360800 - MDN confirmation dialog does not say which addresses the receipt will be sent to (can be multiple)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jorrit Kronjee]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.8) Gecko/20061025 Thunderbird/1.5.0.8

When an e-mail arrives with a Disposition-Notification-To: header the user (depending in the settings) is asked if it wants to sends a return receipt to the sender. When confirmed, the return receipt is sent to the e-mail address in the header.

However, because the Disposition-Notification-To: header allows any kind of e-mail address (even multiple), the user might unwillingly be sending the return receipt to other parties than the original sender.

Although it's a minor security issue, this attack could be used to make a user flood another mailserver, perhaps triggering spamfilters and blacklists.

Reproducible: Always

Steps to Reproduce:
1. Craft an e-mail with a Disposition-Notification-To: header containing multiple third-party e-mail addresses.
2. Open the e-mail in Thunderbird.
3. Click OK when the Confirm dialog box pops up.

Actual Results:  
Depending on the return receipt settings, a confirm dialog box pops up asking the user if it wants to send a return receipt without specifying the recipients.

Expected Results:  
The dialog box should have stated what e-mail addresses it's going to send the return receipt to. 

If the user has 'always send' set, this dialog box should still appear, because the sender e-mail address differs from the e-mail address in the header field.

Comment 1

[Daniel Veditz [:dveditz]]

Are multiple notification addresses part of the spec? If we can kill that part that removes the potential unwitting DoS attack. The fact that the return address might not quite be the sender doesn't bother me as much (there's also the Reply-To: header for a similar effect) but showing the name on the dialog would possibly be a nice touch.

Comment 2

[Phil Ringnalda (:philor)]

Multiples are in RFC 3798 ("mdn-request-header = "Disposition-Notification-To" ":" mailbox *("," mailbox)"), as is nearly all of comment 0:

[[[
   MDNs SHOULD NOT be sent automatically if the address in the
   Disposition-Notification-To header differs from the address in the
   Return-Path header (see [RFC-MSGFMT]).  In this case, confirmation
   from the user SHOULD be obtained, if possible.  If obtaining consent
   is not possible (e.g., because the user is not online at the time),
   then an MDN SHOULD NOT be sent.

   Confirmation from the user SHOULD be obtained (or no MDN sent) if
   there is no Return-Path header in the message, or if there is more
   than one distinct address in the Disposition-Notification-To header.
]]]

Comment 3

[[:Aureliano Buendía]]

This has been fixed by bug #151244.
If I'm wrong feel free to reopen it.

Comment 4

[Magnus Melin [:mkmelin]]

This is not fixed.

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Screenshot coming up

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: screenshot

Comment 7

[Jim Porter (:squib)]

Suggestion: keep the old way around for when Disposition-Notification-To == From?

Comment 8

[Jorrit Kronjee]

Magnus, screenshot looks good. Keep in mind though that the Disposition-Notification-To allows multiple addresses, so that might make it look weird from an interface perspective.

Comment 9

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

Comment on attachment 440291 [details] [diff] [review]
proposed fix

It seems like it would be nice to have the more common, single MDN, use the original shorthand text and the more complicated version use this style.

ui-r- for now until we get the details sorted.

This also feels like it leads to being able to reply to certain MDN requests and not others; I'm assuming that right now it would just reply to all requests even if multiple were available.

Comment 10

[David :Bienvenu]

Comment on attachment 440291 [details] [diff] [review]
proposed fix

clearing review request, then.

Comment 11

[Magnus Melin [:mkmelin]]

Attachment: screenshot, v2 (proposed)

How about this?
The idea is to
 - shorten the text and use the name instead of "the sender of this message"
 - make it clear it's a return receipt we send (i think it may be non-obvious to a lot of people). we have a "return receipt" option in the compose window but how are someone to know this is it unless we use the same verbiage

Comment 12

[Jim Porter (:squib)]

For what it's worth, I like that a lot. It's probably not a dependency as such, but if the message header display name changes from bug 474721 go in, the display name here should probably use the same formatting function.

Comment 13

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

Comment on attachment 441092 [details]
screenshot, v2 (proposed)

That looks pretty good to me.  I would leave out the "!" in the (on ... !) as it might not be a malicious thing but just a different address.

Comment 14

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2

Carrying fwd ui-r=clarkbw

Comment 15

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 444267 [details] [diff] [review]
proposed fix, v2

>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -2575,26 +2575,54 @@ var gMessageNotificationBar =
>+  setMDNMsg: function(aMdnGenerator, aMsgHeader, aMimeHdr)
>   {
>     this.mdnGenerator = aMdnGenerator;
>     this.msgHeader = aMsgHeader;
>+
>+    let mdnHdr = aMimeHdr.extractHeader("Disposition-Notification-To", false);
>+    let fromHdr = aMimeHdr.extractHeader("From", false);
>+
>+    let headerParser = Components.classes["@mozilla.org/messenger/headerparser;1"]
>+                                 .getService(Components.interfaces.nsIMsgHeaderParser);

If you make that line start two spaces after the Components, it'll fit in 80 characters.  (I don't mind if you want to leave it, though.)

>+    let mdnAddr = headerParser.extractHeaderAddressMailboxes(mdnHdr);
>+    let fromAddr = headerParser.extractHeaderAddressMailboxes(fromHdr);
>+
>+    let authorName = headerParser.extractHeaderAddressName(
>+                       aMsgHeader.mime2DecodedAuthor) || aMsgHeader.author;
>+
>+    let mdnBarMsg = document.getElementById("mdnBarMessage");
>+    if (mdnBarMsg.firstChild) // might have to remove old text first
>+      mdnBarMsg.removeChild(mdnBarMsg.firstChild);

Should that be "if (mdnBarMsg.firstChild)" or "while (mdnBarMsg.firstChild)"?

>+    // If the return recepit doesn't go to the sender address, note that in there
>+    // notification.

I would break this comment after the comma, to get it all in 80 characters.  Actually, if you just made it "note that in *the* notification.", which I think is what you meant to say, it would fit, so feel free to do that instead.

>+    if (mdnAddr != fromAddr)
>+    {
>+      mdnBarMsg.appendChild(document.createTextNode(gMessengerBundle.
>+        getFormattedString("mdnBarMessageAddressDiffers", [authorName, mdnAddr])));
>+    }
>+    else
>+    {
>+      mdnBarMsg.appendChild(document.createTextNode(gMessengerBundle.
>+        getFormattedString("mdnBarMessageNormal", [authorName])));
>+    }

I don't think you need the {}s here, if you wanted to get rid of them.

And you could re-break those lines as:
      mdnBarMsg.appendChild(document.createTextNode(
        gMessengerBundle.getFormattedString("mdnBarMessageAddressDiffers",
                                            [authorName, mdnAddr])));
and:
      mdnBarMsg.appendChild(document.createTextNode(
        gMessengerBundle.getFormattedString("mdnBarMessageNormal",
                                            [authorName])));
But again, I'm not that unhappy with the current code.

>@@ -2603,19 +2631,19 @@ var gMessageNotificationBar =
>   /**
>-   * @param aFlag (kMsgNotificationPhishingBar, kMsgNotificationJunkBar, kMsgNotificationRemoteImages
>+   * @param aFlag one of the mBarFlagValues values

Would it make sense to put mBarFlagValues in {}s or ||s?

>+++ b/mail/locales/en-US/chrome/messenger/messenger.dtd
>@@ -733,21 +733,20 @@ you can use these alternative items. Oth
>+<!ENTITY mdnBarIgnoreButton2.label "Ignore">
>+<!ENTITY mdnBarSendButton2.label "Send Return Receipt">

No accesskeys for these?

>+++ b/mail/test/mozmill/message-header/test-return-receipt.js
>@@ -0,0 +1,99 @@
>+function setupModule(module) {
>+  let fdh = collector.getModule("folder-display-helpers");
>+  fdh.installInto(module);
>+  let wh = collector.getModule("window-helpers");
>+  wh.installInto(module);
>+
>+  folder = create_folder("ReturnRecepitTest");

I think that this                 ^^^^^^^^ should be "Receipt".

[snip…]
>+ * Test that return recepits are shown when Disposition-Notification-To is set

I think that this     ^^^^^^^^ should be "receipt".

>+function test_basic_mdn_shown_() {
>+  be_in_folder(folder);
>+
>+  // Select the first message, which will display it.
>+  // This message requests a return receipt.
>+  let curMessage = select_click_row(0);
>+  assert_selected_and_displayed(mc, curMessage);
>+
>+  let msgNotBar = mc.e("msgNotificationBar");
>+  if (msgNotBar.collapsed)
>+    throw new Error("msgNotificationBar not shown although it should");
>+  if (msgNotBar.selectedIndex != 4) // it's the mdnBar showing
>+    throw new Error("msgNotificationBar didn't show the mdnBar; " +
>+                    "msgNotBar.selectedIndex=" + msgNotBar.selectedIndex);
>+
>+  let mdnBar = mc.e("mdnBar");
>+
>+  let notificationText = mdnBar.textContent;
>+  if (notificationText.indexOf("[e-mail]") == -1)
>+    throw new Error("mdnBar didn't state where to send; notificationText=" +
>+                    notificationText);
>+
>+  // Select the first message, which will display it.

Shouldn't this be "Select the second message, which won't display it"?

And should we move this into a separate test?

And should we have another test that tests showing the mdnBar without showing the email address?

>+  // This message doesn't request a return receipt.
>+  curMessage = select_click_row(1);
>+  assert_selected_and_displayed(mc, curMessage);
>+
>+  if (!msgNotBar.collapsed)
>+    throw new Error("mdnBar shown for message where return receipt isn't requested");
>+}

I'm mostly happy with the patch, but since I'ld like to see the updates you do to the test(s), I'm going to give it an r-.

Thanks,
Blake.

Comment 16

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v3

Addressing review comments.

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 521271 [details] [diff] [review]
proposed fix, v3

(I'm guessing you wanted to request this from me…  ;)

Comment 18

[Magnus Melin [:mkmelin]]

Ah yes, your email changed. Guess that's what i get for leaving it to rot for a year :)

Comment 19

[Jim Porter (:squib)]

Since bug 474721 is fixed, you could probably use FormatDisplayName to get the author's name, though I'd probably need to add an argument to that function to disable the "You" shorthand, since that would probably cause grammatical weirdness in other locales (though you could also provide a new context for it to give it a new localization).

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 521271 [details] [diff] [review]
proposed fix, v3

>+++ b/mail/base/content/mailWindowOverlay.js
>@@ -2633,20 +2633,47 @@ var gMessageNotificationBar =
>+    // If the return recepit doesn't go to the sender address, note that in the

This should be "receipt" (and you make the same typo in a couple more places).

>+++ b/mail/test/mozmill/message-header/test-return-receipt.js
>@@ -0,0 +1,106 @@
>+/* ***** BEGIN LICENSE BLOCK *****
>+ *   Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+*

This * got out of line…

>+  // Create a message that requests a return receipt.
>+  let msg = create_message({from: ["=?UTF-8?B?w4VrZQ==?=", "[e-mail]"],
>+                            clobberHeaders: { "Disposition-Notification-To": "[e-mail]" }
>+                           });
>+  add_message_to_folder(folder, msg);
>+
>+  // ... and one that doesn't request a return receipt.
>+  let msg2 = create_message();
>+  add_message_to_folder(folder, msg2);

So, the other two tests I think I'ld like to see are
1) where the notification is requested for a different address than the from, and
2) where there is more than one address in the notification.

But I believe you can write those tests without my interference, so r=me.  ;)

Thanks,
Blake.

Comment 21

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v4 (for checkin)

For checkin. Carrying fwd ui-r=clarkbw, r=bwinton

Comment 22

[Ludovic Hirlimann [:Usul]]

(In reply to comment #21)
> Created attachment 529271 [details] [diff] [review] [review]
> proposed fix, v4 (for checkin)
> 
> For checkin. Carrying fwd ui-r=clarkbw, r=bwinton

Magnus any reasons you didn't check this in ?

Comment 23

[Magnus Melin [:mkmelin]]

I plan to check it soonish. Had my hg account disabled since i didn't check anything in in such a long time, but it should be working again now.

Comment 24

[Magnus Melin [:mkmelin]]

changeset:   7701:641d41984938
http://hg.mozilla.org/comm-central/rev/641d41984938

->FIXED