Bug 411433 (CVE-2008-2808)

file location URL in directory listing should be HTML escaped

VERIFIED FIXED in mozilla1.9

Status

Core Graveyard
File Handling
VERIFIED FIXED
10 years ago
a year ago

People

(Reporter: Masahiro YAMADA, Assigned: Masahiro YAMADA)

Tracking

({verified1.8.1.15})

Trunk
mozilla1.9
verified1.8.1.15
Bug Flags:
blocking1.8.1.15 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:low])

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

10 years ago
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"
(Assignee)

Comment 1

10 years ago
(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).
(Assignee)

Comment 2

10 years ago
Reproduced at other HTML character entity references.
Summary: Directory view creats invalid link for a file its name contains %xx after &gt → Directory view creats invalid link for a file its name contains %xx after HTML character entity reference
(Assignee)

Updated

10 years ago
Version: unspecified → Trunk
(Assignee)

Updated

10 years ago
Summary: Directory view creats invalid link for a file its name contains %xx after HTML character entity reference → directory listing creats invalid link for a file its name contains %xx after HTML character entity reference
(Assignee)

Comment 3

10 years ago
%xx is not needed.
It is also reproduced by "test"-a.html"
Summary: directory listing creats invalid link for a file its name contains %xx after HTML character entity reference → directory listing creats invalid link for a file its name contains HTML character entity reference
(Assignee)

Comment 4

10 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 6

10 years ago
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.
(Assignee)

Comment 7

10 years ago
Please set this bug as Security Sensitive. 
This bug may be very dangerous.
I have forwarded your comment to security@mozilla.org

Comment 9

10 years ago
added security sensitive flag until this is figured out.
Group: security
(Assignee)

Comment 10

10 years ago
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 "<>.
(Assignee)

Comment 11

10 years ago
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?
Whiteboard: [sg:low]
(Assignee)

Comment 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
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.
(Assignee)

Updated

10 years ago
Summary: directory listing creats invalid link for a file its name contains HTML character entity reference → file location URL in directory listing should be HTML escaped
(Assignee)

Comment 16

10 years ago
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
(Assignee)

Comment 17

10 years ago
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 ?
Attachment #297644 - Flags: superreview?
Attachment #297644 - Flags: review?
(Assignee)

Comment 18

10 years ago
Created attachment 297647 [details] [diff] [review]
Patch rv1.1 for Trunk

Fixed broken indent (code is no chanced).
Attachment #297644 - Attachment is obsolete: true
Attachment #297647 - Flags: superreview?
Attachment #297647 - Flags: review?
Attachment #297644 - Flags: superreview?
Attachment #297644 - Flags: review?
(Assignee)

Comment 19

10 years ago
(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.
(Assignee)

Updated

10 years ago
Attachment #297647 - Attachment description: Patch rv1.1 → Patch rv1.1 for Trunk
(Assignee)

Updated

10 years ago
Attachment #297647 - Flags: review? → review?(dveditz)
(Assignee)

Comment 20

10 years ago
> 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 ?
Attachment #297647 - Flags: superreview?(bzbarsky)
Attachment #297647 - Flags: superreview?
Attachment #297647 - Flags: review?(dveditz)
Attachment #297647 - Flags: review?(cbiesinger)
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)?
(Assignee)

Comment 22

10 years ago
Created attachment 306194 [details] [diff] [review]
Patch rv.1.2 for Trunk
Attachment #297647 - Attachment is obsolete: true
Attachment #306194 - Flags: superreview?(bzbarsky)
Attachment #306194 - Flags: review?(cbiesinger)
Attachment #297647 - Flags: superreview?(bzbarsky)
Attachment #297647 - Flags: review?(cbiesinger)
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.
Attachment #306194 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 24

10 years ago
Created attachment 306224 [details] [diff] [review]
Patch rv1.3 for Trunk
Attachment #306224 - Flags: review+
Attachment #306194 - Flags: review?(cbiesinger)
but this patch seems entirely unrelated to comment 0
(Assignee)

Updated

9 years ago
Attachment #306224 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 26

9 years ago
(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.
Attachment #306224 - Flags: superreview?(bzbarsky) → superreview+
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.
(Assignee)

Comment 28

9 years ago
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.)
Attachment #306224 - Flags: approval1.9?
Comment on attachment 306224 [details] [diff] [review]
Patch rv1.3 for Trunk

a1.9+=damons
Attachment #306224 - Flags: approval1.9? → approval1.9+
Oh, isn't this checked-in???
Assignee: nobody → masa141421356
Keywords: checkin-needed
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.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.15?

Comment 32

9 years ago
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?

Attachment #306194 - Attachment is obsolete: true
mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp 	1.92
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
(Assignee)

Comment 34

9 years ago
Verified at
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041707 Minefield/3.0pre
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.15? → blocking1.8.1.15+
(Assignee)

Comment 35

9 years ago
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.
(Assignee)

Comment 36

9 years ago
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
Attachment #317507 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #317510 - Attachment is patch: true
(Assignee)

Updated

9 years ago
Attachment #317510 - Flags: superreview?(bzbarsky)
Attachment #317510 - Flags: review?(cbiesinger)
Attachment #317510 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 37

9 years ago
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.
(Assignee)

Comment 39

9 years ago
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?
Attachment #317510 - Flags: superreview?(bzbarsky) → superreview?(dveditz)
Whiteboard: [sg:low] → [sg:low][needs sr 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.
Attachment #317510 - Flags: superreview?(dveditz) → superreview-
Created attachment 323969 [details] [diff] [review]
1.8 branch version that applies
Attachment #317510 - Attachment is obsolete: true
(Assignee)

Comment 43

9 years ago
(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.
Whiteboard: [sg:low][needs sr dveditz] → [sg:low]
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
Attachment #323969 - Flags: superreview+
Attachment #323969 - Flags: approval1.8.1.15+
Fix checked in to 1.8 branch.
Flags: wanted1.8.0.x?
Flags: blocking1.8.0.15?
Keywords: fixed1.8.1.15
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.
Keywords: fixed1.8.1.15 → verified1.8.1.15

Updated

9 years ago
Flags: wanted1.8.0.x?
Flags: wanted1.8.0.x+
Flags: blocking1.8.0.15?
Flags: blocking1.8.0.15+
Alias: CVE-2008-2808
Group: security

Comment 47

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

a=asac for 1.8.0
Attachment #323969 - Flags: approval1.8.0.next+
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.