User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9b3pre) Gecko/2008010813 Minefield/3.0b3pre Build Identifier: When dropping file (its name contains %) from Explorer (or desktop) to browser, browser opens incorrect file. Reproducible: Always Steps to Reproduce: 1.Create file named test&gt%21.html in a foloder 2.Open the folder with browser. 3.Click link to test&gt%21.html Actual Results: Error (File not found) for Firefox can't find the file at test>%21.html. Expected Results: File should be opened. According to DOM inspector, Value of href of "A" element in directory view is "test>%2521.html", So, it should be "test>%2521.html"

(In reply to comment #0) > So, it should be "test>%2521.html" oops, Its value should be "test&gt%2521.html" (html source should be "test&gt%2521.html).

Reproduced at other HTML character entity references.

%xx is not needed. It is also reproduced by "test"-a.html"

Replacing "&" to "&" may not work well ("&" is also spacial character for URI). It may be better to replace "&" to "%26" is needed. # Opera replaces & to %26 at link to file in directory list.

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008010813 Minefield/3.0b3pre ID:2008010813 using filename from comment 3: test"-a.html

Accoring to nsIndexedToHTML.cpp, this bug contains two issue. 1.HTML Escape for link is needed but not executed. 2. "&" in filename is not QUERY DELIMITER of URL. So, It should be replaced to %26 when creating URL String ( and it should be done before HTML escape). # May this bug causes XSS? on ftp:// or jar ?. # (Windows does not allow " or > in filename, But Unix/linux allows. ) # If yes, this bug may be needed to change Security Sensitive.

Please set this bug as Security Sensitive. This bug may be very dangerous.

I have forwarded your comment to security@mozilla.org

added security sensitive flag until this is figured out.

I don't have OS that accepts '"','<','>' for filename(Windows rejects them) But I think '"><&;+' is needed to be encoded like "%22%3E%3C%26%3B%2B" by security and stability reason. 1. ">< may causes to enable to inject any tags into html. 2. "&" causes invalid link (Comment #3) 3-a. According to RFC, ";" is special character for URL at ftp scheme. 3-b. At "file" scheme, firefox requires ";" in filename to be escaped as "&3B". (testcase: a;b.html) 4. Filename aa+ACI-+AD4-+ADw-script+AD4-alert(location)+ADw-+AC8-script+AD4-.html causes to run script when user selects UTF-7 as character encoding. And ,I think character encoding should no be changeable at directory list. Current HTML Escape in directory list can not escape UTF-7 encoded "<>.

folloing filenames may be danger. 'a"<script>alert(location)</script>.html' a"onmouseover="alert(location)" aa+ACI-+AD4-+ADw-script+AD4-alert(location)+ADw-+AC8-script+AD4-.html These filenames are testcase to check URL encode is valid. test&quot-a.html a;b.html

I'm not sure this really needs to be security sensitive. Any potential exploit needs to get a file on the user's hard disk, and then get the user to click on it in a file:// directory listing, right? And even if they manage that, all they get to do is run script in the context of the file:// page, which isn't privileged afaik.

A more interesting scenario would be an ftp server that allows uploads and doesn't vet filenames. You'd be laying potential traps for other users of that server. This bug is starting to cover several different issues which should be filed as sub-bugs with testcases otherwise we're likely to miss parts of the fix. 1. non-working directory links 2. tag injection 3. character-set interpretation (e.g. UTF-7), though I'm not too worried ... what else?

Reason to mark this bug as security sensitive is related as Bug 411480. At trunk, UI of directory list is implemented by JavaScript. If auther of ftp server (or CD-ROM,etc.) can inject to script to directory list, both of injected scripts and UI of directory list is executed.

I filed bug 412428 for problem around ";" in file/directory/extention name (It seems to be non-security bug), and bug 412431 for UTF-7 encoded tag injection.

I started to trying to write patch. But, There are three questions. 1. Is it needed to check result of nsEscapeHTML2(),nsEscapeHTML() and nsEscape() ? In current source, only Line 967 checks result. 2. If it is needed to check, How should I check it and exit function ? 3. Is it needed to deallocate it using using nsMemory::Free() ? In current source, it is used in only Line 971. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp&rev=1.89

This patch contains adds HTML escape for all "href"s 1. <base> 2. <a> for parent directory 3. <a> for members of directory. Is it needed ui-review ?

Fixed broken indent (code is no chanced).

(In reply to comment #16) > 1. Is it needed to check result of nsEscapeHTML2(),nsEscapeHTML() and > nsEscape() ? > In current source, only Line 967 checks result. > 2. If it is needed to check, How should I check it and exit function ? > 3. Is it needed to deallocate it using using nsMemory::Free() ? > In current source, it is used in only Line 971. > Please ignore them. Adopt contains null-check and Adopt does give the ns(C)String ownership. So, there is no memory-leaks and null-pointer exceptions.

> Created an attachment (id=297647) [details] > Patch rv1.1 > I think it will conflict to patch for Bug 411854. Is "bug 411854" more important than this bug ?

Please fix the "htmlEscaedURL" typo, and pass the string lengths to nsEscapeHTML2 (so put the converter string on the stack, and then pass its .get() and .Length() in)?

Comment on attachment 306194 [details] [diff] [review] Patch rv.1.2 for Trunk >Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp >+ nsString utf16BaseUri = NS_ConvertUTF8toUTF16(baseUri); NS_ConvertUTF8toUTF16 utf16BaseURI(baseURI); Avoids a string copy... Same thing in the other two places, and sr=me.

but this patch seems entirely unrelated to comment 0

(In reply to comment #25) > but this patch seems entirely unrelated to comment 0 > Comment 0 contains important mistake. please see also comment 1. Testcase of Comment 0 contains 2 bugs. (";" and "&") (1)problem about ";" (bug 412428) I think it is caused by mismatch of handling ";" between URL enbcoder and URL encoder. (2)problem about "&" (URL is not HTML escaped) It is fixed by this patch.

Oh I see. I was confused by this part of comment 0: >When dropping file (its name contains %) from Explorer (or desktop) to browser, >browser opens incorrect file.

Comment on attachment 306224 [details] [diff] [review] Patch rv1.3 for Trunk At Trunk(1.9), Directory listing uses JavaScript for User Interface. It is needed to disable to inject script using malformed file name. (User believes directory listing is safe.)

Comment on attachment 306224 [details] [diff] [review] Patch rv1.3 for Trunk a1.9+=damons

Oh, isn't this checked-in???

Masayuki: thanks for noticing the lack of progress on this bug. The "checkin-needed" volunteers generally can't see security bugs unless explicitly CC'd.

The patch still applies cleanly. Who is going to do the checkin? I can do it (since I just applied the patch), if you want. Does it only need to be checked in on trunk?

mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp 1.92

Verified at Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre

I did't tested this patch (Only worked on text editor). Sorry, My PC has CPU-cooler trouble. I cannot build Firefox now.

Sorry, path is not valid. C++ source diff is same as Rev.1.0

Comment on attachment 317510 [details] [diff] [review] Patch for 1.8 Branch Rev.1.1 If 1.8Branch does not use UTF-8 for local file path, this patch should be obsolete. Because this patch requires local file path to be UTF-8. (related to Bug 278161)

(In reply to comment #37) > (From update of attachment 317510 [details] [diff] [review]) > If 1.8Branch does not use UTF-8 for local file path, this patch should be > obsolete. > Because this patch requires local file path to be UTF-8. > (related to Bug 278161) Fx2 uses UTF-8.

I have one more question. At Linux.Unix based OS, directory name can contain double quotation '"'. But, <base href="......"> is deleted if its base uri contains '"' (bug 358128). Is disrectory list on file scheme works correctly on Unix/Linux based system if the directory name contains \x22 (")?

Works correctly in what sense? What needs to be tested?

Comment on attachment 317510 [details] [diff] [review] Patch for 1.8 Branch Rev.1.1 This supposed 1.8-branch patch didn't apply to my 1.8 branch -- I am confused... >diff -u -8 -p -r1.68.16.3 nsIndexedToHTML.cpp Yup, that's the version I get, so how can each of the three chunks not apply? > buffer.AppendLiteral("</head>\n<body>\n<h1>"); End of the first chunk, I several lines of a big '<style>' section before the <head>. Yet your diff seems to show those lines untouched in your tree. > buffer.AppendLiteral("<p id=\"UI_goUp\"><a class=\"up\" href=\""); >- AppendASCIItoUTF16(parentStr, buffer); Second chunk, my line here is not "<p ..." but buffer.AppendLiteral("<tr><td colspan=\"3\"><a href=\""); and has been that way since r1.60 in 2004 Third chunk was just some whitespace differences. What tree did you diff this against? Kinda scary, really.

(In reply to comment #41) > > What tree did you diff this against? Kinda scary, really. > Oops, I downlaoded source from LXR manually, and , I think, I selected bad tree. Sorry, and, thank you to fix patch.

Comment on attachment 323969 [details] [diff] [review] 1.8 branch version that applies sr=dveditz Approved for 1.8.1.15, a=dveditz for release-drivers

Fix checked in to 1.8 branch.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.15pre) Gecko/2008061004 BonEcho/2.0.0.15pre (also tested on Ubuntu 8.04 with Fx20014 and Fx20015pre Using steps from comment #0 and filename from comment #3. In Fx20014 I get an error saying it cannot find the file, while Fx20015pre can open it.

Comment on attachment 323969 [details] [diff] [review] 1.8 branch version that applies a=asac for 1.8.0