960749 - [Download Manager API] - Insufficient error message when < and > used in filenames to Download

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[Rob Fletcher [:omerta]]

While doing some security testing on Download Manager, the following name was tried:
<h1>bad<h1>.exe

The download is initiated and the top notification shows the animation that typically happens at beginning of download. After that, the file does not download and there is no error message.

If we change the file name to the following, the file downloads normally:
bad.exe

Flagging security, because currently this behavior "mitigates" HTML injections in filenames, which is something we will need to test for once this is fixed.

Comment 1

[(no longer active)]

It would also be interesting to see if we're vulnerable to something similar to bug 511521 here.

Comment 2

[Boris Zbarsky [:bzbarsky]]

We sanitize filenames, in general.  But it's somewhat OS-specific how that works and what happens afterward.  What OS did you to your testing on?  Do you have a link to the testcase?

Comment 3

[Rob Fletcher [:omerta]]

This is on Firefox OS, so it was incorrectly categorized when created. My fault! :/

The testcase was the following:
<a href='http://pwnetrationguru.com/<h1>bad<h1>.exe'>Download Me</a>

This was testing on a ZTE 1.4 build. Specific details are below:
OS Version - 1.4.0.0-prerelease
Hardware revision - roamer2
Platform version - 29.0a1
Build Identifier - 20140114115604
Git commit info - 2014-01-14 21:30:30 9d5a87d6

Comment 4

[Cristian Rodriguez (:crdlc)]

I added this code [1] in gaia in order to escape HTML characters but I don't receive any notification here [2] when I try to download this file [3].

Moving to general because there are something wrong in the API

Gecko c5866e0

[1] https://github.com/mozilla-b2g/gaia/pull/16481
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/download/download_manager.js#L27
[3] http://owd.tid.es/dm/%3Ch1%3Ebad%3Ch1%3E.exe

Comment 5

[Aus Lacroix [:aus]]

I think the obvious thing to do here is to filter out those characters on top of the others that we do already for this particular target platform. There's OS specific code for all targeted OS's (including FxOS).

Comment 6

[Aus Lacroix [:aus]]

Here's an example of the new regular expression we could use to strip out all potential HTML tags from the file name. Seems like a good thing to do to me.

http://jsfiddle.net/auswerk/H6stL/

Comment 7

[Aus Lacroix [:aus]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #6)
> Here's an example of the new regular expression we could use to strip out
> all potential HTML tags from the file name. Seems like a good thing to do to
> me.
> 
> http://jsfiddle.net/auswerk/H6stL/

Correct URL: http://jsfiddle.net/auswerk/H6stL/1/

Comment 8

[Aus Lacroix [:aus]]

Attachment: Patch - v1 (lacks test) - Strip out all potential HTML tags from filenames.

Fabrice, the only other thing I would add is a mochitest to ensure that this functionality doesn't break.

Comment 9

[Aus Lacroix [:aus]]

This is the error that is currently occuring when we do not strip out potential HTML tags.

I/Gecko   (  676): *** exception in makeFileUnique: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIFile.create]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: jar:file:///system/b2g/omni.ja!/components/HelperAppDialog.js :: HelperAppLauncherDialog.prototype.makeFileUnique :: line 100"  data: no]

Comment 10

[Rob Fletcher [:omerta]]

Based on this regex provided at jsfiddle:
filename = filename.replace(/^\.|(<([^>]+)>)/ig, '');

The following input still allows the > and < characters to sneak through:
var filename = '>.ev>il<h<H1>"1>>>EVIL</h1>file.bin<';
Output: >.ev>il"1>>>EVILfile.bin<

The following input will allow filenames that start with '.':
var filename = '<h1>.ev>il<h<H1>"1>>>EVIL</h1>file.bin<';
Output: .ev>il"1>>>EVILfile.bin<

Comment 11

[Aus Lacroix [:aus]]

Ah, yep. That regex is trying to strip out html tags, not just < or >. I'll add more magic to the regex to fix the cases listed by Rob.

Comment 12

[Rob Fletcher [:omerta]]

So I am not very good at assembling regexs, but this works for me:
//remove characters the filesystem doesn't like
//remove leading '.'s to prevent hidden files; it is important this is last
filename = filename.replace(/</g, '');
filename = filename.replace(/>/g, '');
filename = filename.replace(/\//g, '');
filename = filename.replace(/\\/g, '');
filename = filename.replace(/^\.{0,}/g, ''); //it is important this is the final replace()

I think global replaces and then checking to make sure it doesn't start with '.' is the most straightforward approach, but I'm unsure the performance implications of multiple replaces; it definitely seems inefficient to parse the line 5 separate times.

Comment 13

[Aus Lacroix [:aus]]

Yeah, it may be easier to split the string into an array and check each character one at a time and simply toss the ones that are not allowed out. It would mean that we wouldn't really strip out the html tags but we would make them do nothing.

Should we go ahead and replace the bad characters with '_'?

Comment 14

[Aus Lacroix [:aus]]

:djabber, 

When stripping out invalid characters in filenames, should we replace it with a different character such as '_'?

Comment 15

[Francis Djabri [:djabber]]

This feels fine to me from a UX point of view as it gives some indication to the user of what has been done.

Comment 16

[[:fabrice] Fabrice Desré]

Comment on attachment 8387951 [details] [diff] [review]
Patch - v1 (lacks test) - Strip out all potential HTML tags from filenames.

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

According to comment 13 you don't plan to use a regexp anymore?

Also, other platforms have the exact same code so should be fixed too: https://mxr.mozilla.org/mozilla-central/search?string=Remove%20any%20leading%20periods%2C%20since

Comment 17

[[:fabrice] Fabrice Desré]

Comment on attachment 8387951 [details] [diff] [review]
Patch - v1 (lacks test) - Strip out all potential HTML tags from filenames.

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

According to comment 13 you don't plan to use a regexp anymore?

Also, other platforms have the exact same code so should be fixed too: https://mxr.mozilla.org/mozilla-central/search?string=Remove%20any%20leading%20periods%2C%20since

Comment 18

[Aus Lacroix [:aus]]

OK, after some fiddling around (no pun intended) this is the best solution I could come up with.

var SUB_CHAR = '_';
var BAD_CHARS = ['<', '>', '/'];
var output = document.getElementById('output');
var filename = '.<h1>.ev>il<h<H1>"1>>>EVIL</h1>file.bin<';

filename = filename.split('');
for(var i = 0; i < filename.length; i++) {
    var c = filename[i];
    if (BAD_CHARS.indexOf(c) != -1) {
        filename.splice(i, 1, SUB_CHAR);
    }
    else if (i === 0 && c == '.') {
        filename.splice(i, 1);
        i = -1;
    }    
}
filename = filename.join('');

This preserves current functionality (stripping out the leading '.') and adds replacement of '<', '>' and '/' with '_'. This avoids makeFileUnique failures while trying to retain as accurate of an original filename as possible.

Fabrice, we won't need to update the other platforms because they show the save as dialog which will ensure that a good file name is used. We could, however, improve the UX by filtering out some known bad chars for all platforms (and even platform specific ones). I'm checking with UX / Product on FF Desktop to see if that's something they would want. 

If yes, then I will also update http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/HelperAppDialog.js and http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js.

Comment 19

[Aus Lacroix [:aus]]

+metro as well.

Comment 20

[(no longer active)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #13)
> Yeah, it may be easier to split the string into an array and check each
> character one at a time and simply toss the ones that are not allowed out.
> It would mean that we wouldn't really strip out the html tags but we would
> make them do nothing.
> 
> Should we go ahead and replace the bad characters with '_'?

This is in fact what we do in Gecko, see <http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp?force=1#1428> for example.

FILE_PATH_SEPARATOR is the platform specific path separator ("\\" on Windows, "/" everywhere else).  FILE_ILLEGAL_CHARACTERS is defined here <http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCRTGlue.h#133> and it includes control characters, and also an OS-specific set of illegal chars (which I guess should be filesystem specific, not OS specific per se.)  For example, the "<" character is legal for most Linux filesystems.

Comment 21

[Dave Hylands [:dhylands]]

However, < isn't legal on most FAT file systems, which is where most downloaded files are stored on our current shipping phones.

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCRTGlue.h#133 defines the set of legal characters based on the OS, which is technically incorrect.

It should be based on the filesystem that the files are being stored to, not the OS storing the file.

Comment 22

[Aus Lacroix [:aus]]

(In reply to Dave Hylands [:dhylands] from comment #21)
> However, < isn't legal on most FAT file systems, which is where most
> downloaded files are stored on our current shipping phones.
> 
> http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCRTGlue.h#133
> defines the set of legal characters based on the OS, which is technically
> incorrect.
> 
> It should be based on the filesystem that the files are being stored to, not
> the OS storing the file.

Yep! :) I think it probably makes sense in this case to not only ensure typical OS illegal characters are replaced but also for FAT/VFAT/exFAT.

Comment 23

[Aus Lacroix [:aus]]

This would be this set of characters: \ / : * ? " < > |

Comment 24

[Dave Hylands [:dhylands]]

(In reply to Ghislain Aus Lacroix [:aus] from comment #23)
> This would be this set of characters: \ / : * ? " < > |

And all characters less than space (0x00 thru 0x31)

Historical Aside: I remember on the Commodore PET that space and shifted-space were considered separate characters, so you could make some interesting filenames :)

Comment 25

[Aus Lacroix [:aus]]

:dhylands, thanks for the tip! It will simplify the code even more.

Comment 26

[Aus Lacroix [:aus]]

Attachment: Patch - v2 - Replace known bad characters for VFAT filesystems (and all non-printable characters) since SD cards typically use VFAT not the FS type of the OS

This additionally fixes a bad 'debug' statement in DownloadsIPC.jsm.

Comment 27

[Aus Lacroix [:aus]]

Attachment: Patch - v3 - Replace known bad characters for VFAT filesystems (and all non-printable characters) since SD cards typically use VFAT not the FS type of the OS.

* additionally fixes a bad 'debug' statement in DownloadsIPC.jsm.
* also removes change from patch v2 where we re-enabled mochitests for b2g emulator and desktop. tests are back to the state they are in on central.

Comment 28

[(no longer active)]

Comment on attachment 8390099 [details] [diff] [review]
Patch - v3 - Replace known bad characters for VFAT filesystems (and all non-printable characters) since SD cards typically use VFAT not the FS type of the OS.

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

::: b2g/components/HelperAppDialog.js
@@ +74,5 @@
> +    aLeafName = aLeafName.split("");
> +    for(var i = 0; i < aLeafName.length; i++) {
> +      // Any character code smaller than the value for space is invalid as 
> +      // well as any character in our array of bad characters.
> +      if (aLeafName[i].charCodeAt(0) < 32 ||

Don't you also need to handle other control chars here?

Comment 29

[Aus Lacroix [:aus]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #28)
> Comment on attachment 8390099 [details] [diff] [review]
> Patch - v3 - Replace known bad characters for VFAT filesystems (and all
> non-printable characters) since SD cards typically use VFAT not the FS type
> of the OS.
> 
> Review of attachment 8390099 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: b2g/components/HelperAppDialog.js
> @@ +74,5 @@
> > +    aLeafName = aLeafName.split("");
> > +    for(var i = 0; i < aLeafName.length; i++) {
> > +      // Any character code smaller than the value for space is invalid as 
> > +      // well as any character in our array of bad characters.
> > +      if (aLeafName[i].charCodeAt(0) < 32 ||
> 
> Don't you also need to handle other control chars here?

I believe the rest of the potential bad characters are already handled in the external app helper service as they are bad for all filesystems. If this is incorrect, I'm happy to add additional checks here.

Comment 30

[(no longer active)]

(In reply to Ghislain Aus Lacroix [:aus] from comment #29)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #28)
> > Comment on attachment 8390099 [details] [diff] [review]
> > Patch - v3 - Replace known bad characters for VFAT filesystems (and all
> > non-printable characters) since SD cards typically use VFAT not the FS type
> > of the OS.
> > 
> > Review of attachment 8390099 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: b2g/components/HelperAppDialog.js
> > @@ +74,5 @@
> > > +    aLeafName = aLeafName.split("");
> > > +    for(var i = 0; i < aLeafName.length; i++) {
> > > +      // Any character code smaller than the value for space is invalid as 
> > > +      // well as any character in our array of bad characters.
> > > +      if (aLeafName[i].charCodeAt(0) < 32 ||
> > 
> > Don't you also need to handle other control chars here?
> 
> I believe the rest of the potential bad characters are already handled in
> the external app helper service as they are bad for all filesystems. If this
> is incorrect, I'm happy to add additional checks here.

If they were, then things like < and > would also be handled, right?

Comment 31

[Aus Lacroix [:aus]]

Not in this case, because we're running under Linux/Gonk we assume ext2/3/4 filesystems or compatible with those. Those filesystems support those characters just fine. It's the existence of both ext4 and vfat that is causing this issue. 

To my knowledge, we do not have any way to query the filesystem for what it supports so this is kind of a compromise. We expect standard filtering to be done like it would for linux, then we apply our own filtering which guarantees that the filename will also be compliant for vfat.

Comment 32

[[:fabrice] Fabrice Desré]

Comment on attachment 8390099 [details] [diff] [review]
Patch - v3 - Replace known bad characters for VFAT filesystems (and all non-printable characters) since SD cards typically use VFAT not the FS type of the OS.

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

Redirecting the review to Ehsan since he seems interested!

Comment 33

[(no longer active)]

Comment on attachment 8390099 [details] [diff] [review]
Patch - v3 - Replace known bad characters for VFAT filesystems (and all non-printable characters) since SD cards typically use VFAT not the FS type of the OS.

Let's fix this once and for all by making FILE_ILLEGAL_CHARACTERS the most restrictive set of chars on all platforms, that is, what it is currently on Windows.

Please ask :bsmedberg for a review on that patch since that would be XPCOM land, and push to try before the review, running all tests everywhere.  Thanks!

Comment 34

[Aus Lacroix [:aus]]

Attachment: Patch - v4 - Unify FILE_ILLEGAL_CHARACTERS for all OS's.

Comment 35

[Aus Lacroix [:aus]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=6265a44688a8

Comment 36

[Aus Lacroix [:aus]]

Attachment: Patch - v5 - Unify FILE_ILLEGAL_CHARACTERS for all OS's

* Updated commit message (used to be try server request message)

Comment 37

[Aus Lacroix [:aus]]

Comment on attachment 8392482 [details] [diff] [review]
Patch - v5 - Unify FILE_ILLEGAL_CHARACTERS for all OS's

Try run is looking great. Let's see how the rest of the patch looks. :)

Comment 38

[Benjamin Smedberg]

Comment on attachment 8392482 [details] [diff] [review]
Patch - v5 - Unify FILE_ILLEGAL_CHARACTERS for all OS's

You can just skip DEFAULT_OS_FILE_ILLEGAL_CHARACTERS and #define OS_FILE_ILLEGAL_CHARACTERS directly; that way you don't have to repeat it in the platform ifdefs.

r=me with that change

Comment 39

[Aus Lacroix [:aus]]

Attachment: Patch - Final - Unify FILE_ILLEGAL_CHARACTERS for all OS's

address review request, simply define OS_FILE_ILLEGAL_CHARACTERS. r+ carries from bsmedberg.

Comment 40

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c91b18cafc64

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/2709c36738d1 for failing just like it did on try.

Comment 42

[Aus Lacroix [:aus]]

(In reply to Wes Kocher (:KWierso) from comment #41)
> Backed out in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/2709c36738d1 for
> failing just like it did on try.

Sorry about that guys, I got a little too excited. :( I'll work through the test failures and try and get it all green before another push attempt to MozInbound.

Comment 43

[Aus Lacroix [:aus]]

OK, I went through the test failures and definitely, all of the windows test failures are caused by this patch. The rest are known intermittent failures and I've marked them as such. I have an idea of how to avoid these failures while still reaching the goal of unifying file_illegal_characters.

Comment 44

[Aus Lacroix [:aus]]

Attachment: Patch - v7 - Unify OS_FILE_ILLEGAL_CHARACTERS, provide KNOWN_PATH_SEPARATORS in addition to FILE_PATH_SEPARATOR.

So, I think part of the problem with the previous patch is that it *added* the windows file path separator as an illegal file character on all platforms. I would imagine it is safe to assume that the system itself will always be running on a filesystem that is compliant with overarching platform.

Because of this, I've added KNOWN_PATH_SEPARATORS as a supplemental define that contains *all* known path separators. We already use FILE_PATH_SEPARATOR to remove them from filenames. 

I've updated the most relevant usages of FILE_PATH_SEPARATOR to KNOWN_PATH_SEPARATORS. There is one potential extra spot: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentAreaDragDrop.cpp#605

Comment 45

[Aus Lacroix [:aus]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=b5d000dd723c

Comment 46

[Aus Lacroix [:aus]]

Comment on attachment 8393736 [details] [diff] [review]
Patch - v7 - Unify OS_FILE_ILLEGAL_CHARACTERS, provide KNOWN_PATH_SEPARATORS in addition to FILE_PATH_SEPARATOR.

The try run is looking much better. I'd like to consider this patch for landing.

Comment 47

[Aus Lacroix [:aus]]

Attachment: Patch - v8 - Unify OS_FILE_ILLEGAL_CHARACTERS, provide KNOWN_PATH_SEPARATORS in addition to FILE_PATH_SEPARATOR. [check-in]

* Updated commit message.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/905eb355cf90

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/905eb355cf90

Comment 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/203ea09d5bdb