677021 - [regression] Attachment extension can again be deceptive

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. Open a virus/worm email that you received. Look at the attachment,
   particularly its type.

Content-Disposition: attachment;
  filename="attach_mozilla.txt                                  .scr".  

Actual result:
Thunderbird mail viewer UI shows:
  1 attachment: attach_mozilla.txt              ...             .scr

Expected result:
Thunderbird mail viewer UI shows:
  1 attachment: attach_mozilla.txt….scr
or better yet:
  1 attachment: attach_mozilla.txt .scr
or better yet:
  1 attachment: attach_mozilla.txt.scr

Importance:
The email of course looked suspicious, but the "attach_mozilla.txt" look innocent to me. I thought several times "but it's a .txt file! It's harmless. I can open it. But wait, there must be something wrong..." until I finally discovered the ".scr".

The .scr, as shown currently in the UI, has no relation to the filename and must be missed by users. If they miss it, they run a hostile executable and are infected. We must prevent this, but the current UI leads people into the pit. Thus Severity Major.

Comment 1

[Ben Bucksch (:BenB)]

Attachment: Testcase, contains a worm/virus in attachment

Comment 2

[Ben Bucksch (:BenB)]

Attachment: Screenshot of current suboptimal UI

Comment 3

[Ben Bucksch (:BenB)]

Fix could be simple: just strip all whitespace from the filename during display.
And during saving, collapse all consecutive whitespace to one space.

Comment 4

[Ben Bucksch (:BenB)]

To be even safer, strip all characters apart from certain allowed ones. The allowed ones would be just latin letters, and optionally letters from other languages like Japanese. If we're lucky, there already is a function (e.g. in the Unicode module or in netwerk) that we can use.

We should just make sure to collapse all punctation and control characters like ._-?! to one, including those in other Unicode tables.

Comment 5

[Jim Porter (:squib)]

I really think we only need to collapse whitespace, since a long run of anything *but* whitespace is going to look very strange. That should be enough to set off most people's "something is wrong" sensors.

I'd also expect to be able to see short runs of non-whitespace characters (e.g. three periods for ellipses), which would make stripping those more complicated.

Comment 6

[Jim Porter (:squib)]

Oh, and that's not the current UI anymore, since the list in the bottom looks different now, and should show the real extension more clearly.

Comment 7

[Ben Bucksch (:BenB)]

> since a long run of anything *but* whitespace is going to look very strange.

Not if it forms a line, for example. Or stars. Because it would nicely occupy the whole space up to the right.

  1 attachment: attach_mozilla.txt ________________…________________.scr
  1 attachment: attach_mozilla.txt ----------------…----------------.scr
  1 attachment: attach_mozilla.txt ****************…****************.scr

In all cases, I stop looking after ".txt ".

---

The screenshot shows TB 5.0 (the released version).

Comment 8

[Ben Bucksch (:BenB)]

I thought this had worked better in previous versions, that this bug didn't exist in TB 3.0, and indeed: In TB 3.0, it is much better, because there is less space for the filename, which nicely solves this bug.

TB 3.1 and earlier had the attachment filename display specifically like that to avoid the problem described here. TB 3.3 and later has the problem again. This makes it formally a regression.

Comment 9

[Ben Bucksch (:BenB)]

Attachment: Screenshot with TB 3.0/3.1 - problem wasn't as serious there

Comment 10

[Ben Bucksch (:BenB)]

Comment on attachment 551284 [details]
Screenshot with TB 3.0/3.1 - problem wasn't as serious there

So, one solution is just to show less of the filename, therefore ensuring that the extension is close to the filename.

Comment 11

[Jim Porter (:squib)]

(In reply to Ben Bucksch (:BenB) from comment #9)
> Created attachment 551284 [details]
> Screenshot with TB 3.0/3.1 - problem wasn't as serious there

This really looks just about as bad to me. Imagine if the filename were a bit shorter: you'd have "attachment.txt       .scr". Until you select the attachment, it looks like you have two attachments: "attachment.txt" and ".scr". This is made worse by the fact that the background color for selected attachments is pretty similar to unselected attachments on most platforms (this is fixed in 8.0).

I've been trying to think of a decent algorithm for collapsing long runs of the same character without munging legitimate filenames. The closest I can come up with is to collapse runs of 5* or more of the same character to "x...x". I don't think we should do this only for whitespace/punctuation, since there are lots of glyphs that would cause the same issue, despite being alphanumeric. For instance, the Japanese numeral 1 (一), which looks pretty much like a hyphen (-).

* Or 10. Or 8. Or...

Comment 12

[Jim Porter (:squib)]

Sorry, you'd have "attachment.txt     ...   .scr", which is still pretty confusing.

Anyway, I marked this as blocking bug 654222, since we want to be able to show the full filename for (legitimate) attachments in the attachment list too (i.e. not just in the collapsed attachment bar).

Comment 13

[Jim Porter (:squib)]

Attachment: Fix this

Here's a patch that does the following:

1) Trim left and right whitespace
2) Collapse runs of 5 or more whitespace characters
3) Collapse runs of 10 or more characters to "X…X"

I don't want to try to get any more clever than this for fear of munging legitimate attachment names.

Comment 14

[Ben Bucksch (:BenB)]

> 2) Collapse runs of 5 or more whitespace characters

s/5/1/ (comment 3, there's no reason at all to have more than 1 consequtive whitespace in a filename)

> 3) Collapse runs of 10 or more characters to "X…X"

s/10/3/ (comment 4)

Thanks for the patch!

I don't care too much about munging legitimate filenames, they are just a proposal by the sender.

Can we change the filename used for saving, too?

Also, I personally don't think that "?" or "!" or "*", quotes or similar stuff should ever be used in a filename, because they can be special characters on the commandline, for example.

Comment 15

[Ben Bucksch (:BenB)]

> (especially Windows will drop trailing dots
> and whitespace from filename extensions)

Please keep this part of the comment (in the function description), because it contains real factual information. Note the dot thing, for example.

Comment 16

[Jim Porter (:squib)]

(In reply to Ben Bucksch (:BenB) from comment #14)
> > 2) Collapse runs of 5 or more whitespace characters
> 
> s/5/1/ (comment 3, there's no reason at all to have more than 1 consequtive
> whitespace in a filename)

I'm erring on the side of caution, but I don't have a real problem with collapsing to a single space.

> 
> > 3) Collapse runs of 10 or more characters to "X…X"
> 
> s/10/3/ (comment 4)

This would be a really bad idea, since the year "1999" would become "19…9". I figure 10 repeated characters is somewhere around the threshold between normal and evil filenames. I think the bare minimum should be 7, since it's reasonable for ISO dates to have 6 repeated digits (20111111, or November 11th, 2011).

> I don't care too much about munging legitimate filenames, they are just a
> proposal by the sender.
> 
> Can we change the filename used for saving, too?

The filename contains important information, especially if you're trying to keep directories in sync on multiple computers (basically a poor-man's FTP, which I've done before out of sheer laziness). Given that the goal of this is to make it obvious to the user what the true extension is *before* opening the file, I don't think it's necessary to change what the filename looks like *after* that.

> Also, I personally don't think that "?" or "!" or "*", quotes or similar
> stuff should ever be used in a filename, because they can be special
> characters on the commandline, for example.

Well, it's not really our business to say what should and shouldn't be a character in a filename. Since we're not (as far as I know) saving files by creating strings to pass to the command-line, I doubt this presents a security threat, so we may as well leave it.

I also don't think it makes much sense to try to get too creative with sanitizing filenames, since this is really a stopgap measure, and the ideal way to protect people would be to fix bug 255150.

(In reply to Ben Bucksch (:BenB) from comment #15)
> > (especially Windows will drop trailing dots
> > and whitespace from filename extensions)
> 
> Please keep this part of the comment (in the function description), because
> it contains real factual information. Note the dot thing, for example.

In that case, we should make the comment relevant and drop trailing dots on Windows.

Comment 17

[Ben Bucksch (:BenB)]

> I'm erring on the side of caution

Me too, but for me that means assuming the sender is hostile ;-).

Your examples with dates are good, though.

> we should ... drop trailing dots on Windows.

definitely :)

Comment 18

[Joe Sabash [:JoeS1]]

I'm pretty sure that .scr .exe and others are not executable in an attachment anymore.
That's not to say we shouldn't know they are there. Saving to disc can be an attack vector.

Comment 19

[Jim Porter (:squib)]

Attachment: Add tests

Here are some tests to ensure that this works as expected.

Comment 20

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

Comment on attachment 559729 [details] [diff] [review]
Add tests

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

All in all, I'm pretty happy with it, and it looks kinda nice.

So, ui-r=me (cause it changes the look of attachments), and r=me with the questions below answered.

Thanks,
Blake.

(I'm also getting two errors running the tests, but I believe they're unrelated:
SUMMARY-UNEXPECTED-FAIL | d:\Programming\comm-central\mail\test\mozmill\attachme
nt\test-attachment.js | test_attachment_list_expansion
  EXCEPTION: Attachment list should be expanded after clicking bar!
    at: test-folder-display-helpers.js line 2809
       assert_true(false,"Attachment list should be expanded after clicking bar!
") test-folder-display-helpers.js 2809
       test_attachment_list_expansion() test-attachment.js 222
            frame.js 554
            frame.js 623
            frame.js 666
            frame.js 503
            frame.js 678
            server.js 182
            server.js 186
SUMMARY-UNEXPECTED-FAIL | d:\Programming\comm-central\mail\test\mozmill\attachme
nt\test-attachment.js | test_attachments_compose_menu
  EXCEPTION: Multiple attachments are selected!: 'Remove Attachment' != 'Remove
Attachments'.
    at: test-folder-display-helpers.js line 2809
       assert_true(false,"Multiple attachments are selected!: 'Remove Attachment
' != 'Remove Attachments'.") test-folder-display-helpers.js 2809
       assert_equals("Remove Attachment","Remove Attachments","Multiple attachme
nts are selected!") test-folder-display-helpers.js 2796
       test_attachments_compose_menu() test-attachment.js 380
            frame.js 554
            frame.js 623
            frame.js 666
            frame.js 503
            frame.js 678
            server.js 182
            server.js 186
make: *** [mozmill-one] Error 1.)

::: mail/base/content/mailCore.js
@@ -587,0 +587,13 @@
> > + * Create a sanitized display name for an attachment in order to help prevent
> > + * people from hiding malicious extensions behind a run of spaces, etc. To do
> > + * this, we strip leading/trailing whitespace and collapse long runs of either
> > + * whitespace or identical characters. Windows especially will drop trailing
NaN more ...

Will that regex handle crazy non-printing unicode characters?

@@ -587,0 +587,15 @@
> > + * Create a sanitized display name for an attachment in order to help prevent
> > + * people from hiding malicious extensions behind a run of spaces, etc. To do
> > + * this, we strip leading/trailing whitespace and collapse long runs of either
> > + * whitespace or identical characters. Windows especially will drop trailing
NaN more ...

What if the spaces and dots are interleaved?  ("foo.scr. . . . . . . ...  ."?)

::: mail/test/mozmill/attachment/test-attachment.js
@@ -105,2 +122,5 @@
> >    ];
> >  
> > +  // Add another evilly-named attachment for Windows tests, to ensure that
> > +  // trailing periods are stripped.
> > +  if ('@mozilla.org/windows-registry-key;1' in Components.classes) {

Do these also test the attachment box in the compose window?  And the names in the expanded attachment panel?

Comment 21

[Jim Porter (:squib)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #20)
> (I'm also getting two errors running the tests, but I believe they're
> unrelated:

I'm running a try build to see what happens with these...

> Will that regex handle crazy non-printing unicode characters?

Since the goal is to hide printing characters that might obscure the last few characters of a filename, I don't think non-printing characters will hurt us.

> @@ -587,0 +587,15 @@
> > > + * Create a sanitized display name for an attachment in order to help prevent
> > > + * people from hiding malicious extensions behind a run of spaces, etc. To do
> > > + * this, we strip leading/trailing whitespace and collapse long runs of either
> > > + * whitespace or identical characters. Windows especially will drop trailing
> NaN more ...
> 
> What if the spaces and dots are interleaved?  ("foo.scr. . . . . . . ... 
> ."?)

This seems to confuse Windows a fair bit. It's probably worth changing the regex to /[\s\.]+$/ to accommodate this.

> Do these also test the attachment box in the compose window?  And the names
> in the expanded attachment panel?

I test the expanded panel, but the compose window doesn't do any sanitization since the compose window contains attachments controlled by the user.

Comment 22

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

(In reply to Jim Porter (:squib) from comment #21)
> > Will that regex handle crazy non-printing unicode characters?
> Since the goal is to hide printing characters that might obscure the last
> few characters of a filename, I don't think non-printing characters will
> hurt us.

Dang it, I meant to say "crazy unicode space characters", like <http://www.fileformat.info/info/unicode/char/3000/index.htm> or the like.

Thanks,
Blake.

Comment 23

[Jim Porter (:squib)]

(In reply to Blake Winton (:bwinton - Thunderbird UX) from comment #22)
> Dang it, I meant to say "crazy unicode space characters", like
> <http://www.fileformat.info/info/unicode/char/3000/index.htm> or the like.

It works on that space character too (I just tried it). Here's the whole list of what \s matches[1]:

[ \f\n\r\t\v​\u00A0\u1680​\u180e\u2000​\u2001\u2002​\u2003\u2004​\u2005\u2006​\u2007\u2008​\u2009\u200a​\u2028\u2029​\u2028\u2029​\u202f\u205f​\u3000]

[1] https://developer.mozilla.org/en/JavaScript/Guide/Regular_Expressions

Comment 24

[Jim Porter (:squib)]

Checked in: http://hg.mozilla.org/comm-central/rev/4bc84315d7d5

Comment 25

[Jim Porter (:squib)]

Attachment: Patch for checkin

Here's the patch that was checked in; the only change from the previous version is that I changed a regex from /(\.)+$/ to /[ \.]+$/. (Note that I'm using a space here instead of \s because all whitespace was already converted to regular spaces).

Comment 26

[Mark Banner (:standard8)]

Checked into aurora:

http://hg.mozilla.org/releases/comm-aurora/rev/567184a444a2

Comment 27

[Ben Bucksch (:BenB)]

Does this cover -.-.-.-.-.-.-.- and -=-=-=-=-=-=-=-=-=-=- ?