411433 - (CVE-2008-2808) file location URL in directory listing should be HTML escaped

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Masahiro YAMADA]

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]

(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]

Reproduced at other HTML character entity references.

Comment 3

[Masahiro YAMADA]

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

Comment 4

[Masahiro YAMADA]

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]]

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]

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]

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

Comment 8

[Arie Paap [:wildmyron]]

I have forwarded your comment to [e-mail]

Comment 9

[chris hofmann]

added security sensitive flag until this is figured out.

Comment 10

[Masahiro YAMADA]

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]

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: [e-mail]]]

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]]

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]

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]

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]

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]

Attachment: 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]

Attachment: Patch rv1.1 for Trunk

Fixed broken indent (code is no chanced).

Comment 19

[Masahiro YAMADA]

(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]

> 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 [:bzbarsky]]

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]

Attachment: Patch rv.1.2 for Trunk

Comment 23

[Boris Zbarsky [:bzbarsky]]

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]

Attachment: Patch rv1.3 for Trunk

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

but this patch seems entirely unrelated to comment 0

Comment 26

[Masahiro YAMADA]

(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)]

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]

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)]

Comment on attachment 306224 [details] [diff] [review]
Patch rv1.3 for Trunk

a1.9+=damons

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 31

[Daniel Veditz [:dveditz]]

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]

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: [e-mail]]]

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

Comment 34

[Masahiro YAMADA]

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]

Attachment: 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]

Attachment: 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]

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] (he/him)(JST, +0900)]

(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]

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 [:bzbarsky]]

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

Comment 41

[Daniel Veditz [:dveditz]]

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]]

Attachment: 1.8 branch version that applies

Comment 43

[Masahiro YAMADA]

(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]]

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]]

Fix checked in to 1.8 branch.

Comment 46

[juan becerra [:juanb]]

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]

Comment on attachment 323969 [details] [diff] [review]
1.8 branch version that applies

a=asac for 1.8.0