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

RESOLVED FIXED in Thunderbird 5.0b1

Status

RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: bugzilla, Assigned: mkmelin)

Tracking

(Blocks: 1 bug)

Trunk
Thunderbird 5.0b1
Bug Flags:
blocking-thunderbird3 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want])

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
Assignee: mscott → bienvenu
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [sg:investigate]
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.
Group: security
Whiteboard: [sg:investigate] → [sg:want]
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.
]]]
Severity: minor → normal
Flags: blocking-thunderbird3?
(Assignee)

Updated

11 years ago
Summary: Confirm dialog for notification is not descriptive enough → MDN confirmation dialog does not say which addresses the receipt will be sent to (can be multiple)
Flags: wanted-thunderbird3?
Flags: blocking-thunderbird3?
Flags: blocking-thunderbird3-
This has been fixed by bug #151244.
If I'm wrong feel free to reopen it.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:want] → [sg:want][fixed by bug #151244]
(Assignee)

Comment 4

9 years ago
This is not fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

9 years ago
Whiteboard: [sg:want][fixed by bug #151244] → [sg:want]
(Assignee)

Comment 5

9 years ago
Created attachment 440291 [details] [diff] [review]
proposed fix

Screenshot coming up
Assignee: bienvenu → mkmelin+mozilla
Status: REOPENED → ASSIGNED
Attachment #440291 - Flags: ui-review?(clarkbw)
Attachment #440291 - Flags: review?(bienvenu)
(Assignee)

Comment 6

9 years ago
Created attachment 440292 [details]
screenshot
Suggestion: keep the old way around for when Disposition-Notification-To == From?
(Reporter)

Comment 8

9 years ago
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 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.
Attachment #440291 - Flags: ui-review?(clarkbw) → ui-review-

Comment 10

9 years ago
Comment on attachment 440291 [details] [diff] [review]
proposed fix

clearing review request, then.
Attachment #440291 - Flags: review?(bienvenu)
(Assignee)

Comment 11

9 years ago
Created attachment 441092 [details]
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
Attachment #440291 - Attachment is obsolete: true
Attachment #440292 - Attachment is obsolete: true
Attachment #441092 - Flags: ui-review?(clarkbw)
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 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.
Attachment #441092 - Flags: ui-review?(clarkbw) → ui-review+
(Assignee)

Comment 14

9 years ago
Created attachment 444267 [details] [diff] [review]
proposed fix, v2

Carrying fwd ui-r=clarkbw
Attachment #444267 - Flags: ui-review+
Attachment #444267 - Flags: review?(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("ake@example.com") == -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.
Attachment #444267 - Flags: review?(bwinton) → review-
(Assignee)

Comment 16

8 years ago
Created attachment 521271 [details] [diff] [review]
proposed fix, v3

Addressing review comments.
Attachment #444267 - Attachment is obsolete: true
Attachment #521271 - Flags: ui-review+
Attachment #521271 - Flags: review?
Comment on attachment 521271 [details] [diff] [review]
proposed fix, v3

(I'm guessing you wanted to request this from me…  ;)
Attachment #521271 - Flags: review? → review?(bwinton)
(Assignee)

Comment 18

8 years ago
Ah yes, your email changed. Guess that's what i get for leaving it to rot for a year :)
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 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==?=", "ake@example.org"],
>+                            clobberHeaders: { "Disposition-Notification-To": "ake@example.com" }
>+                           });
>+  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.
Attachment #521271 - Flags: review?(bwinton) → review+
(Assignee)

Comment 21

8 years ago
Created attachment 529271 [details] [diff] [review]
proposed fix, v4 (for checkin)

For checkin. Carrying fwd ui-r=clarkbw, r=bwinton
Attachment #521271 - Attachment is obsolete: true
Attachment #529271 - Flags: ui-review+
Attachment #529271 - Flags: review+
(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 ?
(Assignee)

Comment 23

8 years ago
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.
(Assignee)

Comment 24

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

->FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago8 years ago
Flags: wanted-thunderbird3?
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.3a4
Blocks: 360488
Flags: in-testsuite+
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.