1617370 - (CVE-2020-12397) Sender Email Address Spoofing by encoded extraneous white space in display name

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Magnus Melin [:mkmelin]]

+++ This bug was initially created as a clone of Bug #1506587 +++

By abusing Unicode whitespaces (e.g., U+3000) within the 'From:' email header, an attacker can spoof the sender email address when displayed by the Thunderbird mail client.

Turns out bug 1506587 didn't fix the case where the special characters are encoded.

Testcase in attachment 9128245 [details].

Comment 1

[Jorg K (CEST = GMT+2)]

Yes, I was wondering which code path RFC 2047 encoded tokens would take.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1617370_spoof_white_encoded.patch

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&selectedJob=290046100

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9128371 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

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

Have you checked linting? I see a eslint-enable complexity in the vicinity of the changes, perhaps that needs to move now.

::: mailnews/mime/jsmime/test/unit/test_header.js
@@ +815,5 @@
>            ],
>          ],
> +        // Collapse multiple "special" spaces like zero width spaces (\u200B)
> +        // etc. also when encoded.
> +        // This test case has invisible spaces both encoded and not.

Please expand on this comment as follows:
1) State the characters encoded in the base64 string. I tried decoding it at https://www.base64decode.org/, but from the result I can't see which invisible characters you used.
2) Please state that \u00A0 is a NBSP and \u2003 is an EM space
3) \u2003 is used in the code, but \u200B is used in the comment. Most likely you have the latter in the base64 string, but that's hard to tell.

Comment 4

[Jorg K (CEST = GMT+2)]

Actually,
YmzDtiA8aW52aXNpYmxlc3BhY2VAZnJpZW5kLmV4YW1wbGUuY29tPiAu decodes to blö <[e-mail]> . and encoding that again I get
YmzDtiA8aW52aXNpYmxlc3BhY2VAZnJpZW5kLmV4YW1wbGUuY29tPiAu
So it seems to me that the comment is in fact incorrect and the RFC 2047 token doesn't contain invisible (zero width) spaces like \u200B. It's also not used outside the token so it appears that the test doesn't actually prove anything :-(

Comment 5

[Henri Sivonen (:hsivonen)]

jfkthame, is there a CSS property that can collapse sequences of space-like and default-ignorable characters to a single rendered space without modifying the DOM text node value?

Comment 6

[Jonathan Kew [:jfkthame]]

I can't think of something in CSS that would do this... we have the CSS white space processing rules, but collapsing only applies to normal <space> (and <tab>) characters, not to what the spec calls "other space separators".

Comment 7

[0xsobky]

As a minor optimization to the patch, I think that cleanToken() shouldn't be invoked at all if the token only contains characters in the ASCII range \x20-\x7E. So only call cleanToken(t.token) if the following regex test /[^\x20-\x7E]/.test(t.token) returns true. There is no point cleaning the token if it doesn't contain any zero-width characters, especially that consecutive spaces are collapsed outside of the cleanToken function.

Comment 8

[Magnus Melin [:mkmelin]]

I think for consistency it's just easier to to find-replace at once.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: bug1617370_spoof_white_encoded.patch

Adds some tests but other than that it's mostly moving the call to a better place.

There is one slight imperfection in the patch. For case D I replace with space, not remove. For some reason if i replace with "" throughout the process, somewhere, e.g. for zero width space the spaces surrounding it will also get completely removed. I haven't figured out exactly why, but it shouldn't matter in practice.

Comment 10

[Paul Morris [:pmorris]]

Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

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

Changes look good to me, with a few comments, nits addressed.  It looks like Jorgk's concerns have been addressed.  (Right?)  The test_header.js test passes locally with the patch applied.

::: mailnews/mime/jsmime/jsmime.js
@@ +330,5 @@
>      /**
> +     * Clean up characters that could cause display problems since they
> +     * are not displayed.
> +     * @param {string} token - The string to be cleaned.
> +     * @returns {string} the cleaned string.

nit: capitalize:  the -> The ?

@@ +379,5 @@
> +      // -- case D: /\p{Cf}/u
> +      // https://www.fileformat.info/info/unicode/category/Cf/list.htm
> +      // https://mothereff.in/regexpu#input=/\p{Cf}/u&unicodePropertyEscape=1
> +      // Remove all of these except for \u0600-\u0605.
> +      // XXX: We replace these with space, not empty space. Notably, for

Just to make this really clear, I'd say something like the following ("empty string" seems like a more common wording than "empty space"):

  We replace these with spaces (" "), not empty strings ("").

@@ +1070,5 @@
>            groupName = "";
>          } else {
>            // This is either comment content, a quoted-string, or some span of
>            // dots and atoms.
> +          token = cleanToken(token.toString());

Up to you, but I'd assign to a new variable like "tokenCleaned" rather than re-assign here.  Especially in a long function like this it's harder to keep track of what's what, like whether "token" is cleaned yet or not.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

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

Looks better, but still some mistakes.

::: mailnews/mime/jsmime/test/unit/test_header.js
@@ +843,5 @@
>            ],
>          ],
> +        // Collapse multiple consecutive "special" spaces, like zero width space
> +        // etc.
> +        // \u00A0 is soft hypen. \u200B is zero width space

That's a soft-hyphen? Isn't this a NBSP? I looked it up, soft-hyphen is U+00AD.

@@ +856,5 @@
> +        ],
> +
> +        // Collapse multiple consecutive "special" spaces, like zero width space
> +        // etc. also when encoded.
> +        // \u00A0 is soft hypen. \u200B is zero width space

And here.

@@ +858,5 @@
> +        // Collapse multiple consecutive "special" spaces, like zero width space
> +        // etc. also when encoded.
> +        // \u00A0 is soft hypen. \u200B is zero width space
> +        [
> +          //"=?UTF-8?B?IMKgIGJsw7YgPGludmlzaWJsZXNwYWNlQGZyaWVuZC5leGFtcGxlLmNvbT4g4oCLIOKAiyDigIsu=?= <[e-mail]>"

This needs a comment. I'm not a great fan of this hacky UTF16 to UTF8 conversion, see bug 1551746.

Comment 12

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/84249ce5084a8b7a0925b5d35c00331870a56a0c

Comment 13

[Magnus Melin [:mkmelin]]

Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

Sec issue.

Comment 14

[Jorg K (CEST = GMT+2)]

The comment is incorrect :-( - Just proving the confusion.

+        // btoa takes a binary string to encode. unescape(encodeURIComponent(source))
+        // does UTF-8 to binary conversion. See bug 1551746 for other ways.

Correct is this:
unescape(encodeURIComponent(source)) encodes the JavaScript UTF-16 representation of the string into UTF-8.

And please lose the comment about btoa. It just takes some bytes, and bytes are always binary, in fact, everything is binary ;-)

Comment 15

[Magnus Melin [:mkmelin]]

Well it's from https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/btoa

Comment 16

[Jorg K (CEST = GMT+2)]

Well, yes, btoa() takes some arbitrary binary data. I don't think the hint is useful since we all know that. But more importantly, the comment is totally wrong, since the aim is not UTF-8 to binary, whatever that might mean, but UTF-16 to UTF-8.

Comment 17

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/5a60280601f4f761896dc12d87c7e35c921c8fd2
Follow-up: Correct comment. rs=comment-only DONTBUILD

Comment 18

[Wayne Mery (:wsmwk)]

Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

Thanks for the tests

Comment 19

[Wayne Mery (:wsmwk)]

Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

Good in beta, so approved for ESR

Comment 20

[Kai Engert (:KaiE:)]

Should this bug get marked as resolved?

Comment 21

[Magnus Melin [:mkmelin]]

Yes.

Comment 22

[Rob Lemley [:rjl]]

Thunderbird 68.8.0:
https://hg.mozilla.org/releases/comm-esr68/rev/4ca5075cd516bdff172466a7e625888882a41821
https://hg.mozilla.org/releases/comm-esr68/rev/0abd6e4e477e3ad0eaedd97d8a6e469d786b8ded

Comment 23

[Kai Engert (:KaiE:)]

Magnus, who should get attributed as the reporter of this issue?

Comment 24

[Magnus Melin [:mkmelin]]

I think the reporter of bug 1506587. See bug 1506587 comment 67.

Comment 25

[0xsobky]

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

Magnus, who should get attributed as the reporter of this issue?

If you need a name for the security advisory, please use "Ahmed Elsobky (@0xSobky)". Thanks!

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

Beta uplift wasn't processed.