Closed Bug 358128 Opened 13 years ago Closed 13 years ago

HTML injection in gopher dir listing ("xxs in Firefox 2.0?")

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dveditz, Assigned: dougt)

References

()

Details

(Keywords: fixed1.8.0.9, fixed1.8.1.1, Whiteboard: needs 1.8.0.x landing, [need testcase])

Attachments

(2 files, 4 obsolete files)

There was a post on Full-disclosure describing an oddity with gopher directory listings: http://lists.grok.org.uk/pipermail/full-disclosure/2006-October/050287.html

   <iframe src='gopher://"><center><input><button><H1><b>heeelo_word<'
   width=100% height=100%>

There are a couple problems here.
 - unrelated, the gopher protocol handler should probably be
   nsIStandardURI::URITYPE_AUTHORITY rather than URITYPE_STANDARD

 - the gopher handler should be returning an error for the bogus
   host, like http and ftp do.

 - the directory listing code should be escaping the ". To make this work
   the " has to be in the host part of the gopher URI, which makes it
   an invalid host. Anywhere else and nsStandardURI will have escaped it.
   But, should a buggy protocol handler use the directory lister (gopher
   being one) it will run afoul at

http://lxr.mozilla.org/mozilla/source/netwerk/streamconv/converters/nsIndexedToHTML.cpp#337

   where the raw baseURI is appended to "<base href=\"". In the other two
   places the spec is use it's run through nsEscapeHTML2. Each time.
1) we do not need to use the auth url parser.  gopher doesn't do logins (if it supports them, we do not handle them now.  i brought this up last night, but it is clear not that it orthogonal to the problem we are seeing.

2) I fixed the gopher stream converter to properly propogate the underlying transport's status/error code.  This fixes the problem we are seeing as the quote char as a server name does not resolve.  With this, we not correctly get a server not found page.

3) I fixed the index html stream converter to url encode the base uri before injecting content.  The result of this (without fix 2) results in content that properly escapes the quote char.

patching coming up.
Attached patch patch v.1 (obsolete) — Splinter Review
Attachment #243660 - Flags: review?(dveditz)
is this branch only? (trunk doesn't have this mPartChannel oddness anymore)
yup, this patch was for the branch.  I think bug 312760 fixed the gopher stream converter on the trunk.  

However, we probably can apply the patch to nsIndexedToHTML.cpp on the trunk.
seams like the encoding in nsIndexedToHTML might be too aggressive, but it doesn't show any problem.  However we might be able to just simply drop the base uri in the html.  I have tested this way without any problem.

new patch coming up.
Attached patch patch v.2 (obsolete) — Splinter Review
drops base url from the generated content as it is not needed as much as we can tell.
Attachment #243660 - Attachment is obsolete: true
Attachment #243660 - Flags: review?(dveditz)
Attachment #243699 - Attachment is obsolete: true
Comment on attachment 243700 [details] [diff] [review]
patch v.2 (for real)

r=dveditz

Thanks for removing the base href. Slightly worried it was needed for something, but it appears to have been added as part of bug 78148 without any comment explaining why.

If we break something we can go back to the v1 patch and add esc_Minimal to the NS_EscapeURL flags, but if we don't need the base then I'd rather not have it at all.
Attachment #243700 - Flags: review+
landed the nsIndexedToHTML.cpp patch on the trunk.  What do we need to do to get the patch on the branch?

Checking in nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v  <--  nsIndexedToHTML.cpp
new revision: 1.77; previous revision: 1.76
done
So... isn't the base URI needed for the case of FTP URIs that refer to directories but don't end in a slash? E.g. ftp://ftp.mozilla.org/pub
tested that and it works.  also tested file: and gopher:
so clicking a link in the listing for that FTP URL goes to the right place?
yes. :-)

The fix is now on the trunk.  dan and I were looking for edge cases but we really couldn't find any.
this should land on the branch.
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
(In reply to comment #13)
> yes. :-)
> 
> The fix is now on the trunk.  dan and I were looking for edge cases but we
> really couldn't find any.

Hm... It doesn't work for me on latest trunk. I load ftp://ftp.mozilla.org/pub and click the "mozilla.org" link there, and get a 550 failed to change directory error.
i see this regression also. :-/

I am backing out the change to nsIndexedToHTML.cpp on the trunk:

Checking in nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v  <--  nsIndexedToHTML.cpp
new revision: 1.78; previous revision: 1.77
done


We can probably do something like patch 243660.... although it isn't strictly needed.
Attached patch nsIndexedToHTML.cpp patch. (obsolete) — Splinter Review
this patch check to ensure that the baseUri can not prematurely terminate the base href string.  This patch should be applied to the trunk as-is.  For the branch, we probably want patch 243700 (minus the stuff touching nsIndexedToHTML.cpp) + this patch.
Attachment #244930 - Flags: review?(dveditz)
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
Comment on attachment 244930 [details] [diff] [review]
nsIndexedToHTML.cpp patch.

>+    // If there is a quote character in the baseUri, then
>+    // set baseUri to an empty string.  The reason for this
>+    // is that we stick baseUri into a quoted string and a
>+    // quote character will prematurely close the base href
>+    // string.  See bug 358128.

May want to note that this is a fall-back check and that's why
punitive blanking is OK rather than trying to play nice and escaping
the quotes.

      // Set baseUri to an empty string if we find a quote character
      // (see bug 358128). Legit quotes in the path should already be
      // escaped (%22) and quotes in a hostname are invalid; if a
      // quote gets this far something is very, very wrong.

>+    NS_UnescapeURL(baseUri);

This isn't good, the baseUri needs to remain escaped.

>+    if (baseUri.Find("\"") > -1)
>+    {
>+        baseUri = "";

Stick an NS_ERROR("broken protocol handler didn't escape double-quote");
in here.

>+    }
>+

r- because of the unescaping, seems to break things. Comments are just nits.
Attachment #244930 - Flags: review?(dveditz) → review-
Right, i was worried about something like:

gopher://%22<button>

But that doesn't seam to work. In anycase, we should have used the unmodified string.
Attachment #244930 - Attachment is obsolete: true
Attachment #245460 - Flags: review?(dveditz)
Comment on attachment 245460 [details] [diff] [review]
nsIndexedToHTML.cpp patch w/ dveditz's comments

r+ dveditz

"baseUri.FindChar() != kNotFound" might be better, Find(str) is heavier-weight.
Attachment #245460 - Flags: review?(dveditz) → review+
Attached patch patch v.3Splinter Review
Slightly different approach.  We only add a base href when we have to.
Attachment #245460 - Attachment is obsolete: true
Attachment #245669 - Flags: review?(dveditz)
Comment on attachment 245669 [details] [diff] [review]
patch v.3

r=dveditz
Attachment #245669 - Flags: review?(dveditz)
Attachment #245669 - Flags: review+
Attachment #245669 - Flags: approval1.8.1.1?
Whiteboard: needs trunk landing
Comment on attachment 245669 [details] [diff] [review]
patch v.3

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #245669 - Flags: approval1.8.1.1?
Attachment #245669 - Flags: approval1.8.1.1+
Attachment #245669 - Flags: approval1.8.0.9+
Comment on attachment 243700 [details] [diff] [review]
patch v.2 (for real)

gopher part approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #243700 - Flags: approval1.8.1.1+
Attachment #243700 - Flags: approval1.8.0.9+
Trunk:  (remember that the trunk doesn't need the gopher patch as it doesn't have a part channel)

Checking in nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v  <--  nsIndexedToHTML.cpp
new revision: 1.79; previous revision: 1.78
done

Branch:

Checking in nsGopherDirListingConv.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsGopherDirListingConv.cpp,v  <--  nsGopherDirListingConv.cpp
new revision: 1.19.8.2; previous revision: 1.19.8.1
done
Checking in nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v  <--  nsIndexedToHTML.cpp
new revision: 1.68.16.3; previous revision: 1.68.16.2
done


Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Looks like it still needs a 1.8.0 branch landing?
Whiteboard: needs trunk landing → needs 1.8.0.x landing
Whiteboard: needs 1.8.0.x landing → needs 1.8.0.x landing, [need testcase]
on the 1.8.0 branch per jay/dveditz:
Checking in nsGopherDirListingConv.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsGopherDirListingConv.cpp,v  <--
  nsGopherDirListingConv.cpp
new revision: 1.19.16.1; previous revision: 1.19
done
Checking in nsIndexedToHTML.cpp;
/cvsroot/mozilla/netwerk/streamconv/converters/nsIndexedToHTML.cpp,v  <--  nsInd
exedToHTML.cpp
new revision: 1.68.20.1; previous revision: 1.68
Keywords: fixed1.8.0.9
You need to log in before you can comment on or make changes to this bug.