1526456 - junk after address can be parsed as the address

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Jianjun Chen]

Attachment: thunderbird-chase-phshing.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_2) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36

Steps to reproduce:

When an attacker sends the following email with a malformed email address to a victim,

From: <[e-mail]>[e-mail]
To: <[e-mail]>
...

Thunderbird takes [e-mail] as the email sender, but many email providers like hotmail, iCloud, protonmail use [e-mail] for DMARC authentication.

This bug allows an attacker to spoof any websites, even if those websites deploy strict DMARC policy. Attachment is an example that exploits this bug to spoof Chase bank email.

Actual results:

Thunderbird mistakenly parses <[e-mail]>[e-mail] as [e-mail].

Expected results:

Thunderbird should display <[e-mail]>[e-mail] as [e-mail] or <[e-mail]>[e-mail], so that Thunderbird users could know that this email could be a spoofing or phishing email.

Comment 1

[Daniel Veditz [:dveditz]]

Thunderbird does not enforce DMARC, that's typically enforced by the receiving mail server. The only checking Thunderbird does for sender legitimacy is with S/MIME signed messages (and PGP signed with the Enigmail addon). Otherwise mail is completely forgable.

The standard for mail display is the bit outside the <> is the "pretty name" and should be the primary display piece. That's what thunderbird is doing. I believe your request is something like

If the display name "looks like" an email address, show the "real" email address instead

That might be a reasonable change.

Alternately your request could be read as "Thunderbird should enforce DMARC". Without that the "real" email could be just as fraudulent as the display name.

Comment 2

[Jianjun Chen]

Hi Daniel,

Thanks for your reply.

The standard for mail display is the bit outside the <> is the "pretty name" and should be the primary display piece. That's what thunderbird is doing. I believe your request is something like
If the display name "looks like" an email address, show the "real" email address instead

In the above example, Thunderbird actually takes the bit outside the <> as real email address, not display name.

I'm not suggesting Thunderbird should enforce DMARC. The problem here is that Thunderbird is inconsistent with receiving mail server on understanding the malformed email address, which could lead to security issues. The receiving mail server authenticates one domain, but Thunderbird shows another one.

Thanks,
Jianjun

Comment 3

[Jorg K (CEST = GMT+2)]

Magnus, any action on this one. (Still looking at bug I haven't seen from the last 365 days.)

Comment 4

[Magnus Melin [:mkmelin]]

Attachment: bug1526456_after_address.patch

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0eb56d2944d6e15f9282f3c0af8647980a4c4ddd

Comment 5

[Magnus Melin [:mkmelin]]

Attachment: bug1526456_after_address.patch

Fixed lint issue.

Comment 6

[Jorg K (CEST = GMT+2)]

Sorry about the delay here, other fires burning hotter. Here's try, which I did for other purposes:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c26ac5daf508d0c216f86600830904cfde906794

Comment 7

[Jorg K (CEST = GMT+2)]

Comment on attachment 9104738 [details] [diff] [review]
bug1526456_after_address.patch

Please look at this test case:
From: <[e-mail]> <[e-mail]>
TB 68 shows "[e-mail]"@example.com which is aligned with its usual behaviour. With your patch I get: [e-mail] example.com.

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 9104738 [details] [diff] [review]
bug1526456_after_address.patch

yes, this looks like it doesn't cover Jörg's example from comment 7.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: 1526456-v2.patch

Magnus, this tries to improve your patch. Was your intention that everything after "name" <address> should get ignored?

With this patch, we ignore the angle brackets after we're done parsing an address.

Also, this code seems to handle multiple addresses separated by comma, so I think we need to reset the new flag after a comma.

Comment 10

[Magnus Melin [:mkmelin]]

Attachment: 1526456-v2.patch

Thanks for improving the patch.
I added a little more tests around this. The change to throw away the name when we're after the address was not entirely correct - the new tests cover this.

Comment 11

[Kai Engert (:KaiE:)]

Comment on attachment 9126365 [details] [diff] [review]
1526456-v2.patch

>         } else if (token === "@") {
>+          if (afterAddress) {
>+            //name = "";
>+            continue;
>+          }

Was it intentional that you left in the code commented out?

Comment 12

[Magnus Melin [:mkmelin]]

No, I'll remove that.

Comment 13

[Kai Engert (:KaiE:)]

Comment on attachment 9126365 [details] [diff] [review]
1526456-v2.patch

r+ with removed //name= "";

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/cd29db7dc105
junk after address should not be parsed as the address. r=kaie

Comment 15

[Jorg K (CEST = GMT+2)]

What about comment #7:
From: <[e-mail]> <[e-mail]>

Comment 16

[Kai Engert (:KaiE:)]

Jörg, what about comment 8 and 9 ? ;)
I hope we have fixed your scenario.

Comment 17

[Jorg K (CEST = GMT+2)]

Hmm, those comments don't state whether
From: <[e-mail]> <[e-mail]>
is fixed or not. Maybe "With this patch, we ignore the angle brackets after we're done parsing an address" includes that case. But you didn't add it as a test case as far as I can see.

Comment 18

[Kai Engert (:KaiE:)]

The only reason I created the patch in comment 9 was to fix the scenario you reported in comment 7.
I had tested my patch locally with your example, and it worked.

You're right, we didn't add a test for "trailing junk that also uses <> brackets".

Comment 19

[Magnus Melin [:mkmelin]]

Attachment: bug1526456_junk_after-followup.patch

Add more tests. The above case was handled for header parsing already, but not for makeFromDisplayAddress.

Comment 20

[Kai Engert (:KaiE:)]

If there's no ">", then indexOf(">") will return -1. If the second (EndIndex) parameter is given to .slice it will use that as (length-1). In other words, if there's no ">", your result will miss the last character.
Instead of cleanName.indexOf(">") should you use the following? (cleanName.indexOf(">") == -1) ? cleanName.length : (cleanName.indexOf(">")

I need to write down your logic, to ensure I understand it.

• the intention is to search for the first <> block after a potential "" prefix.
• code removes a part "anything" (but keeps empty double "" or single ").
• Scenario (A). If, after removal, if there's still a "<", then we use that smaller string (outside quotes) to search for the email address.
• Scenario (B). Else, after removal, if there's no longer a "<", then an email address is inside the quotes, only. So let's go back, use the full string, and take the address from inside the quotes.
• You use the first index of "<" to search for the address in the (potentially) reduced string, but you use the last index of "<" of the full string to identify the cut off position for the display name.

What about input: "name" <[e-mail]> <[e-mail]> <[e-mail]> ?
It's scenario A. The reduced string is <[e-mail]> <[e-mail]> <[e-mail]>.
addr is [e-mail].
displayname is "name" <[e-mail]> <[e-mail]>

Comment 21

[Kai Engert (:KaiE:)]

In scenario B, if the < is inside quotes, then your display name will have a starting quote, but no trailing quote.

input: "full-name <[e-mail]>" something-else
addr = [e-mail]
displayname = "full-name

Comment 22

[Kai Engert (:KaiE:)]

Also, what happens if there's a > character occurring before the first < ?
It might be good to rewrite this function with more error checking.

Comment 23

[Kai Engert (:KaiE:)]

I haven't tested this code, but maybe something like this could be used:

  // Construct a single email address from a name <local@domain> token.
  _makeSingleAddress(aToken) {
    let addr = "";
    // Remove optional leading whitespace and an optional section in double quotes.
    let withoutIntroName = aToken.replace(/^\s*"[^"\s]*"/, "");
    let displayName = aToken.replace(/^\s*("[^"\s]*").*$/, "$1");
    let openPos = withoutIntroName.indexOf("<");
    if (openPos != -1) {
      let closePos = withoutIntroName.indexOf(">", openPos+1);
      if (closePos != -1) {
        addr = withoutIntroName.slice(openPos+1, closePos);
      } else {
        // no trailing ">" is illegal, so we keep the emptry string for addr
        return this.makeMailboxObject(aToken, "");
      }
    } else {
      // There was an optional start section with quotes, but there's no more "<" in the remaining text.
      // Go back and allow searching inside the quotes, but remove the surrounding quotes.
      let noQuotes = displayName.slice(1, -1);

      openPos = noQuotes.indexOf("<");
      if (openPos != -1) {
        let closePos = noQuotes.indexOf(">", openPos+1);
        if (closePos != -1) {
          addr = noQuotes.slice(openPos+1, closePos);
          displayName = noQuotes.slice(0, openPos);
        } else {
          // no trailing ">" is illegal, so we keep the emptry string for addr
          return this.makeMailboxObject(aToken, "");
        }
      } else {
        return this.makeMailboxObject(aToken, "");
      }
    }
    return this.makeMailboxObject(displayName, addr);
  },

Comment 24

[Kai Engert (:KaiE:)]

Note I have edited the above code multiple times. Will stop editing it now.

Comment 25

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:KaiE:) from comment #21)

In scenario B, if the < is inside quotes, then your display name will have a starting quote, but no trailing quote.

input: "full-name <[e-mail]>" something-else
addr = [e-mail]
displayname = "full-name

I believe that's how it would be, and correctly so.
makeFromDisplayAddress is used for parsing user input inside the application, not parsing headers from incoming mails. If the input is bad enough, what's right gets hard to say.

Comment 26

[Magnus Melin [:mkmelin]]

Or no, that's incorrect.

Comment 27

[Magnus Melin [:mkmelin]]

Attachment: bug1526456_junk_after-followup.patch

Thanks for testing. I adjusted the patch and added some more test cases.

Comment 28

[Magnus Melin [:mkmelin]]

(In reply to Kai Engert (:KaiE:) from comment #20)

• Scenario (B). Else, after removal, if there's no longer a "<", then an email address is inside the quotes, only. So let's go back, use the full string, and take the address from inside the quotes.

I think we probably don't want to, or at least the input is really invalid then.
I suppose there is a case to be made for that if the whole string is quoted, we could first just strip these outer quotes.

Comment 29

[Kai Engert (:KaiE:)]

could someone else help to review the follow-up patch?

Comment 30

[Magnus Melin [:mkmelin]]

Attachment: bug1526456_junk_after-followup.patch

Paul, please take a look.

Comment 31

[Magnus Melin [:mkmelin]]

I added the unquoting for the pasting case (e.g. you're pasting an address that happened to be quoted).

Comment 32

[Paul Morris [:pmorris]]

Comment on attachment 9138029 [details] [diff] [review]
bug1526456_junk_after-followup.patch

Review of attachment 9138029 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, with a few nits.

::: mailnews/mime/src/MimeJSComponents.jsm
@@ +427,5 @@
>      }
>      return output;
>    },
>  
>    // Construct a single email address from a name <local@domain> token.

While here, we should more clearly indicate what the aInput token looks like, for example:  
  "...from a |name <local@domain>| token."

Even better would be a doc comment that documents the param and return values.  A "single email address" sounds like a string but a MailboxObject is actually what is returned.

@@ +429,5 @@
>    },
>  
>    // Construct a single email address from a name <local@domain> token.
> +  _makeSingleAddress(aInput) {
> +    // If the whole string within quotes, unquote it first.

nit: "string within" -> "string is within"

@@ +433,5 @@
> +    // If the whole string within quotes, unquote it first.
> +    aInput = aInput.trim().replace(/^"(.*)"$/, "$1");
> +
> +    if (aInput.includes("<")) {
> +      // We don't want to look for address within quotes.

nit: "for address" -> "for the address"

@@ +447,5 @@
> +      let addr = cleanedInput.slice(
> +        cleanedInput.indexOf("<") + 1,
> +        cleanedInput.indexOf(">")
> +      );
> +      let lbracket = aInput.indexOf("<" + addr + ">");

I'd suggest for readability: lbracket -> leftBracketIndex

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/426213bc6cb9
handle additional cases of junk after email addresses. r=pmorris DONTBUILD

Comment 34

[Wayne Mery (:wsmwk)]

Comment on attachment 9138029 [details] [diff] [review]
bug1526456_junk_after-followup.patch

[Triage Comment]

Comment 35

[Magnus Melin [:mkmelin]]

Thunderbird 68.7: https://hg.mozilla.org/releases/comm-esr68/rev/5f34872523ce8c5b25887546f89995a8d651a066

Comment 36

[Magnus Melin [:mkmelin]]

... and https://hg.mozilla.org/releases/comm-esr68/rev/ffc701b53a1463a1bfeb89e9e2828eee695805ae