Last Comment Bug 411433 - (CVE-2008-2808) file location URL in directory listing should be HTML escaped
(CVE-2008-2808)
: file location URL in directory listing should be HTML escaped
Status: VERIFIED FIXED
[sg:low]
: verified1.8.1.15
Product: Core Graveyard
Classification: Graveyard
Component: File Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9
Assigned To: Masahiro YAMADA
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-08 18:49 PST by Masahiro YAMADA
Modified: 2016-06-22 12:16 PDT (History)
16 users (show)
dveditz: blocking1.8.1.15+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
asac: wanted1.8.0.x+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch rv1.0 (3.04 KB, patch)
2008-01-17 15:53 PST, Masahiro YAMADA
no flags Details | Diff | Splinter Review
Patch rv1.1 for Trunk (3.07 KB, patch)
2008-01-17 16:00 PST, Masahiro YAMADA
no flags Details | Diff | Splinter Review
Patch rv.1.2 for Trunk (3.29 KB, patch)
2008-02-27 20:33 PST, Masahiro YAMADA
bzbarsky: superreview+
Details | Diff | Splinter Review
Patch rv1.3 for Trunk (3.26 KB, patch)
2008-02-28 01:30 PST, Masahiro YAMADA
cbiesinger: review+
bzbarsky: superreview+
dsicore: approval1.9+
Details | Diff | Splinter Review
Patch for 1.8 Branch Rev.1.0 (3.02 KB, patch)
2008-04-24 03:56 PDT, Masahiro YAMADA
no flags Details | Diff | Splinter Review
Patch for 1.8 Branch Rev.1.1 (3.02 KB, patch)
2008-04-24 04:02 PDT, Masahiro YAMADA
cbiesinger: review+
dveditz: superreview-
Details | Diff | Splinter Review
1.8 branch version that applies (3.27 KB, patch)
2008-06-05 18:15 PDT, Daniel Veditz [:dveditz]
dveditz: superreview+
dveditz: approval1.8.1.15+
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Description Masahiro YAMADA 2008-01-08 18:49:24 PST
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"
Comment 1 Masahiro YAMADA 2008-01-08 18:58:01 PST
(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).
Comment 2 Masahiro YAMADA 2008-01-08 19:01:46 PST
Reproduced at other HTML character entity references.
Comment 3 Masahiro YAMADA 2008-01-08 19:29:11 PST
%xx is not needed.
It is also reproduced by "test"-a.html"
Comment 4 Masahiro YAMADA 2008-01-08 22:21:11 PST
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.
Comment 5 Arie Paap [:wildmyron] 2008-01-08 22:30:12 PST
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
Comment 6 Masahiro YAMADA 2008-01-08 23:46:41 PST
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.
Comment 7 Masahiro YAMADA 2008-01-09 00:13:44 PST
Please set this bug as Security Sensitive. 
This bug may be very dangerous.
Comment 8 Arie Paap [:wildmyron] 2008-01-09 04:58:34 PST
I have forwarded your comment to security@mozilla.org
Comment 9 chris hofmann 2008-01-09 07:38:23 PST
added security sensitive flag until this is figured out.
Comment 10 Masahiro YAMADA 2008-01-09 08:02:08 PST
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 "<>.
Comment 11 Masahiro YAMADA 2008-01-09 08:12:29 PST
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
Comment 12 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-01-09 10:47:13 PST
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.
Comment 13 Daniel Veditz [:dveditz] 2008-01-09 15:00:23 PST
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?
Comment 14 Masahiro YAMADA 2008-01-12 21:18:34 PST
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.
Comment 15 Masahiro YAMADA 2008-01-15 04:20:49 PST
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.
Comment 16 Masahiro YAMADA 2008-01-16 01:01:17 PST
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
Comment 17 Masahiro YAMADA 2008-01-17 15:53:31 PST
Created attachment 297644 [details] [diff] [review]
Patch rv1.0

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 ?
Comment 18 Masahiro YAMADA 2008-01-17 16:00:39 PST
Created attachment 297647 [details] [diff] [review]
Patch rv1.1 for Trunk

Fixed broken indent (code is no chanced).
Comment 19 Masahiro YAMADA 2008-01-17 16:11:08 PST
(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.
Comment 20 Masahiro YAMADA 2008-01-17 17:06:22 PST
> 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 ?
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2008-02-27 12:24:51 PST
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 22 Masahiro YAMADA 2008-02-27 20:33:36 PST
Created attachment 306194 [details] [diff] [review]
Patch rv.1.2 for Trunk
Comment 23 Boris Zbarsky [:bz] (still a bit busy) 2008-02-27 20:44:23 PST
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.
Comment 24 Masahiro YAMADA 2008-02-28 01:30:23 PST
Created attachment 306224 [details] [diff] [review]
Patch rv1.3 for Trunk
Comment 25 Christian :Biesinger (don't email me, ping me on IRC) 2008-03-08 02:58:37 PST
but this patch seems entirely unrelated to comment 0
Comment 26 Masahiro YAMADA 2008-03-11 17:46:15 PDT
(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.
Comment 27 Christian :Biesinger (don't email me, ping me on IRC) 2008-03-12 17:04:01 PDT
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 28 Masahiro YAMADA 2008-03-14 04:17:01 PDT
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 29 Damon Sicore (:damons) 2008-03-14 16:11:03 PDT
Comment on attachment 306224 [details] [diff] [review]
Patch rv1.3 for Trunk

a1.9+=damons
Comment 30 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-04-12 17:10:46 PDT
Oh, isn't this checked-in???
Comment 31 Daniel Veditz [:dveditz] 2008-04-15 16:24:26 PDT
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.
Comment 32 cmtalbert 2008-04-16 08:54:05 PDT
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?

Comment 33 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-04-16 13:23:47 PDT
mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp 	1.92
Comment 34 Masahiro YAMADA 2008-04-17 17:27:01 PDT
Verified at
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre
Comment 35 Masahiro YAMADA 2008-04-24 03:56:23 PDT
Created attachment 317507 [details] [diff] [review]
Patch for 1.8 Branch Rev.1.0

I did't tested this patch (Only worked on text editor).
Sorry, My PC has CPU-cooler trouble. I cannot build Firefox now.
Comment 36 Masahiro YAMADA 2008-04-24 04:02:20 PDT
Created attachment 317510 [details] [diff] [review]
Patch for 1.8 Branch Rev.1.1

Sorry, path is not valid.
C++ source diff is same as Rev.1.0
Comment 37 Masahiro YAMADA 2008-04-24 06:30:45 PDT
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)
Comment 38 Masayuki Nakano [:masayuki] (Mozilla Japan) (Offline: 9/19, 9/22-9/25, 9/28)) 2008-04-24 06:56:31 PDT
(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.
Comment 39 Masahiro YAMADA 2008-04-28 02:56:13 PDT
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 (")?
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2008-04-28 13:03:56 PDT
Works correctly in what sense?  What needs to be tested?
Comment 41 Daniel Veditz [:dveditz] 2008-06-05 18:11:03 PDT
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.
Comment 42 Daniel Veditz [:dveditz] 2008-06-05 18:15:36 PDT
Created attachment 323969 [details] [diff] [review]
1.8 branch version that applies
Comment 43 Masahiro YAMADA 2008-06-05 20:59:17 PDT
(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 44 Daniel Veditz [:dveditz] 2008-06-06 14:42:28 PDT
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
Comment 45 Daniel Veditz [:dveditz] 2008-06-06 16:25:22 PDT
Fix checked in to 1.8 branch.
Comment 46 juan becerra [:juanb] 2008-06-10 18:23:47 PDT
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 47 Alexander Sack 2009-01-05 11:59:09 PST
Comment on attachment 323969 [details] [diff] [review]
1.8 branch version that applies

a=asac for 1.8.0

Note You need to log in before you can comment on or make changes to this bug.