Closed Bug 1617370 (CVE-2020-12397) Opened 1 year ago Closed 10 months ago

Sender Email Address Spoofing by encoded extraneous white space in display name

Categories

(Thunderbird :: Message Reader UI, defect)

defect
Not set
normal

Tracking

(thunderbird_esr68+ fixed, thunderbird76+ affected, thunderbird77 fixed)

RESOLVED FIXED
Thunderbird 77.0
Tracking Status
thunderbird_esr68 + fixed
thunderbird76 + affected
thunderbird77 --- fixed

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

(Keywords: csectype-spoof, sec-low)

Attachments

(1 file, 1 obsolete file)

+++ 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].

Group: mail-core-security

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

Status: NEW → ASSIGNED
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.

Actually,
YmzDtiA8aW52aXNpYmxlc3BhY2VAZnJpZW5kLmV4YW1wbGUuY29tPiAu decodes to blö <invisiblespace@friend.example.com> . 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 :-(

Attachment #9128371 - Flags: review-

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?

Flags: needinfo?(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".

Flags: needinfo?(jfkthame)

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.

Attachment #9128371 - Flags: review?(kaie)

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

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.

Attachment #9128371 - Attachment is obsolete: true
Attachment #9138509 - Flags: review?(paul)
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.
Attachment #9138509 - Flags: review?(paul) → review+
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=?= <foe@example.com>"

This needs a comment. I'm not a great fan of this hacky UTF16 to UTF8 conversion, see bug 1551746.
Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

Sec issue.
Attachment #9138509 - Flags: approval-comm-esr68?
Attachment #9138509 - Flags: approval-comm-beta?

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 ;-)

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.

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

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

Thanks for the tests
Attachment #9138509 - Flags: approval-comm-beta? → approval-comm-beta+
Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

Good in beta, so approved for ESR
Attachment #9138509 - Flags: approval-comm-esr68? → approval-comm-esr68+

Should this bug get marked as resolved?

Yes.

Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED

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

Flags: needinfo?(mkmelin+mozilla)

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

Flags: needinfo?(mkmelin+mozilla)
Alias: CVE-2020-12397

(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!

Group: mail-core-security → core-security-release
Comment on attachment 9138509 [details] [diff] [review]
bug1617370_spoof_white_encoded.patch

Beta uplift wasn't processed.
Attachment #9138509 - Flags: approval-comm-beta+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.