Closed Bug 960749 Opened 6 years ago Closed 6 years ago

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

Categories

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

defect
Not set

Tracking

(blocking-b2g:1.4+, firefox29 wontfix, firefox30 fixed, firefox31 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 1.4+
Tracking Status
firefox29 --- wontfix
firefox30 --- fixed
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: rfletcher, Assigned: aus)

References

Details

(Whiteboard: [systemsfe][p=8])

Attachments

(1 file, 7 obsolete files)

8.35 KB, patch
aus
: review+
Details | Diff | Splinter Review
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.
It would also be interesting to see if we're vulnerable to something similar to bug 511521 here.
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?
Component: DOM → File Handling
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
Group: core-security
Component: File Handling → General
Product: Core → Firefox OS
Whiteboard: [systemsfe]
Blocks: 946543
Component: General → Gaia::System
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
Component: Gaia::System → General
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).
Assignee: nobody → aus
Status: NEW → ASSIGNED
Whiteboard: [systemsfe] → [systemsfe][p=3]
Target Milestone: --- → 1.4 S4 (28mar)
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/
(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/
Fabrice, the only other thing I would add is a mochitest to ensure that this functionality doesn't break.
Attachment #8387951 - Flags: feedback?(fabrice)
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]
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<
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.
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.
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 '_'?
:djabber, 

When stripping out invalid characters in filenames, should we replace it with a different character such as '_'?
Flags: needinfo?(fdjabri)
This feels fine to me from a UX point of view as it gives some indication to the user of what has been done.
Flags: needinfo?(fdjabri)
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
Attachment #8387951 - Flags: feedback?(fabrice)
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
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.
Flags: needinfo?(fabrice)
+metro as well.
(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.
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.
(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.
This would be this set of characters: \ / : * ? " < > |
(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 :)
:dhylands, thanks for the tip! It will simplify the code even more.
Flags: needinfo?(fabrice)
blocking-b2g: --- → 1.4+
Component: General → Runtime
This additionally fixes a bad 'debug' statement in DownloadsIPC.jsm.
Attachment #8390071 - Flags: review?(fabrice)
Attachment #8387951 - Attachment is obsolete: true
* 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.
Attachment #8390099 - Flags: review?(fabrice)
Attachment #8390071 - Attachment is obsolete: true
Attachment #8390071 - Flags: review?(fabrice)
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?
(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.
(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?
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.
Flags: needinfo?(ehsan)
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!
Attachment #8390099 - Flags: review?(fabrice) → review?(ehsan)
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!
Attachment #8390099 - Flags: review?(ehsan) → review-
Flags: needinfo?(ehsan)
Attachment #8390099 - Attachment is obsolete: true
* Updated commit message (used to be try server request message)
Attachment #8392415 - Attachment is obsolete: true
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. :)
Attachment #8392482 - Flags: review?(benjamin)
Component: Runtime → Gaia::System
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
Attachment #8392482 - Flags: review?(benjamin) → review+
address review request, simply define OS_FILE_ILLEGAL_CHARACTERS. r+ carries from bsmedberg.
Attachment #8392482 - Attachment is obsolete: true
Attachment #8393065 - Flags: review+
Keywords: checkin-needed
(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.
Whiteboard: [systemsfe][p=3] → [systemsfe][p=8]
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.
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
Attachment #8393065 - Attachment is obsolete: true
Attachment #8393736 - Flags: feedback?(benjamin)
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.
Attachment #8393736 - Flags: feedback?(benjamin) → review?(benjamin)
Attachment #8393736 - Flags: review?(benjamin) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/905eb355cf90
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 992879
You need to log in before you can comment on or make changes to this bug.