[regression] Attachment extension can again be deceptive

RESOLVED FIXED in Thunderbird 9.0

Status

defect
--
major
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: BenB, Assigned: squib)

Tracking

(Blocks 1 bug, {regression})

Trunk
Thunderbird 9.0
Dependency tree / graph
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird7-, thunderbird8+ fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

8 years ago
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.
Reporter

Updated

8 years ago
Reporter

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

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

Updated

8 years ago
Reporter

Comment 7

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

Comment 8

8 years ago
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.
Keywords: regression
Summary: Attachment extension can still be deceptive → [regression] Attachment extension can again be deceptive
Reporter

Comment 10

8 years ago
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.
Attachment #551284 - Attachment description: Screenshot, with TB 3.0/3.1 - problem didn't exist there → Screenshot with TB 3.0/3.1 - problem wasn't as serious there
Assignee

Comment 11

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

Comment 12

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

Comment 13

8 years ago
Posted patch Fix this (obsolete) — Splinter Review
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.
Assignee: nobody → squibblyflabbetydoo
Status: NEW → ASSIGNED
Assignee

Updated

8 years ago
Attachment #551298 - Flags: feedback?(ben.bucksch)
Reporter

Comment 14

8 years ago
> 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.
Reporter

Comment 15

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

Comment 16

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

Comment 17

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

Comment 19

8 years ago
Posted patch Add tests (obsolete) — Splinter Review
Here are some tests to ensure that this works as expected.
Attachment #551298 - Attachment is obsolete: true
Attachment #551298 - Flags: feedback?(ben.bucksch)
Attachment #559729 - Flags: review?(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?
Attachment #559729 - Flags: ui-review+
Attachment #559729 - Flags: review?(bwinton)
Attachment #559729 - Flags: review+
Assignee

Comment 21

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

Comment 23

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

Comment 24

8 years ago
Checked in: http://hg.mozilla.org/comm-central/rev/4bc84315d7d5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
OS: Windows XP → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 9.0
Assignee

Comment 25

8 years ago
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).
Attachment #559729 - Attachment is obsolete: true
Attachment #561939 - Flags: ui-review+
Attachment #561939 - Flags: review+
Attachment #561939 - Flags: approval-comm-aurora?
Attachment #561939 - Flags: approval-comm-aurora? → approval-comm-aurora+
Reporter

Comment 27

8 years ago
Does this cover -.-.-.-.-.-.-.- and -=-=-=-=-=-=-=-=-=-=- ?
You need to log in before you can comment on or make changes to this bug.