Bug 411433 (CVE-2008-2808)

file location URL in directory listing should be HTML escaped

VERIFIED FIXED in mozilla1.9

Status

defect
VERIFIED FIXED
12 years ago
3 years ago

People

(Reporter: masa141421356, Assigned: masa141421356)

Tracking

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

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.
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
Version: unspecified → Trunk
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
%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
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
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.
Group: security
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?
Whiteboard: [sg:low]
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.
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
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
Posted patch Patch rv1.0 (obsolete) — Splinter Review
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?
Posted patch Patch rv1.1 for Trunk (obsolete) — Splinter Review
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?
(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.
Attachment #297647 - Attachment description: Patch rv1.1 → Patch rv1.1 for Trunk
Attachment #297647 - Flags: review? → review?(dveditz)
> 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)?
Posted patch Patch rv.1.2 for Trunk (obsolete) — Splinter Review
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+
but this patch seems entirely unrelated to comment 0
Attachment #306224 - Flags: superreview?(bzbarsky)
(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.
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+
Assignee: nobody → masa141421356
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?
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
Closed: 12 years ago
Keywords: checkin-needed
OS: Windows 2000 → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
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+
Posted patch Patch for 1.8 Branch Rev.1.0 (obsolete) — Splinter Review
I did't tested this patch (Only worked on text editor).
Sorry, My PC has CPU-cooler trouble. I cannot build Firefox now.
Posted patch Patch for 1.8 Branch Rev.1.1 (obsolete) — Splinter Review
Sorry, path is not valid.
C++ source diff is same as Rev.1.0
Attachment #317507 - Attachment is obsolete: true
Attachment #317510 - Attachment is patch: true
Attachment #317510 - Flags: superreview?(bzbarsky)
Attachment #317510 - Flags: review?(cbiesinger)
Attachment #317510 - Flags: review?(cbiesinger) → review+
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?
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-
Attachment #317510 - Attachment is obsolete: true
(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.
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 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.