Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 6.1.4
    • Fix Version/s: 6.1.7
    • Component/s: None
    • Labels:
      None
    • Environment:
      Linux
    • Number of attachments :
      2

      Description

      The following request works fine:

      wget http://localhost:8080/test/favicon.ico

      While the following request fails (note the double slashes in the url):

      wget http://localhost:8080/test//favicon.ico

      On Tomcat both requests are handled ok.

        Activity

        Hide
        Greg Wilkins added a comment -

        OK - I checked in a filthy hack... that if the file is an alias and then I try again with all // stripped to /.

        I actually think this is wrong as I think that /foo//../bar should be /foo/bar not
        /bar .... but I think others simply do this hack (eg firefox address line).

        I feel dirty!

        Show
        Greg Wilkins added a comment - OK - I checked in a filthy hack... that if the file is an alias and then I try again with all // stripped to /. I actually think this is wrong as I think that /foo//../bar should be /foo/bar not /bar .... but I think others simply do this hack (eg firefox address line). I feel dirty!
        Hide
        Greg Wilkins added a comment -

        David,

        I think that file systems come into this because rfc 2396 clearly states that every / is significant
        and that /foo//bar has three segments, the middle one being the empty string.

        rfc1738 says that path segments are directory names when interpreting file URLs.
        There is nothing to stop java being run on a file system where the
        empty string is allowed as a valid directory name.

        As for the URL.normalize() method - it does say that it's idea of normal form is to
        remove empty segments // - but I can't find any reference to why that is considered
        a normal form? RFC2396 says that normal form is shema dependant and 1738 is
        silent about it.

        So I think sun have just made an arbitrary decision on what they think normal means.
        But I don't think that it is in any way standard or even valid.

        But enough of my complaining I've implemented it for files.

        Show
        Greg Wilkins added a comment - David, I think that file systems come into this because rfc 2396 clearly states that every / is significant and that /foo//bar has three segments, the middle one being the empty string. rfc1738 says that path segments are directory names when interpreting file URLs. There is nothing to stop java being run on a file system where the empty string is allowed as a valid directory name. As for the URL.normalize() method - it does say that it's idea of normal form is to remove empty segments // - but I can't find any reference to why that is considered a normal form? RFC2396 says that normal form is shema dependant and 1738 is silent about it. So I think sun have just made an arbitrary decision on what they think normal means. But I don't think that it is in any way standard or even valid. But enough of my complaining I've implemented it for files.
        Hide
        jgenender jgenender added a comment -

        Thanks for fixing this Greg. We'll give it a shot.

        Show
        jgenender jgenender added a comment - Thanks for fixing this Greg. We'll give it a shot.
        Hide
        Endre Stølsvik added a comment -

        Greg,

        Just a quick comment here.

        I don't know how you've implemented this, but here's my opinion:

        If you're accepting "//" as "/", then you should do this always. An approach of first trying file system access directly, then trying with the stuff hacked is a recipe for (a possible security-) disaster, IMO - that is when you get problems such as you outline: Something working one way on one OS, but another way (with a possible huge security hole) on another OS.

        Also, what about "///"? Or "////" for that matter?

        I think you should do this quick and brutal BEFORE accessing any files and whatnots, possibly even before giving any Servlet access to the URL line (req.getPathInfo()), so that some user-implemented default servlet doesn't suddenly just shove the pathInfo to the filesystem. Do a quick scan over the url string, very early: More than one consecutive slash is collapsed to one, regardless of how many. Then the processing proceeds.

        PS: Come to think about it - wasn't such a double-slash handling once a big issue with some MS bug thingy? I don't remember it clearly, but I think it had something to do with normalization and encoding combined with some double slashes stuff..! And then I think we had a couple of versions of some MS OS where "..." (three dots) was two directories up, a "convenient shorthand" for "../..", which of course also opens up a whole raft of interesting security issues. Possibly check against that too, or?

        Show
        Endre Stølsvik added a comment - Greg, Just a quick comment here. I don't know how you've implemented this, but here's my opinion: If you're accepting "//" as "/", then you should do this always . An approach of first trying file system access directly, then trying with the stuff hacked is a recipe for (a possible security-) disaster, IMO - that is when you get problems such as you outline: Something working one way on one OS, but another way (with a possible huge security hole) on another OS. Also, what about "///"? Or "////" for that matter? I think you should do this quick and brutal BEFORE accessing any files and whatnots, possibly even before giving any Servlet access to the URL line (req.getPathInfo()), so that some user-implemented default servlet doesn't suddenly just shove the pathInfo to the filesystem. Do a quick scan over the url string, very early: More than one consecutive slash is collapsed to one, regardless of how many. Then the processing proceeds. PS: Come to think about it - wasn't such a double-slash handling once a big issue with some MS bug thingy? I don't remember it clearly, but I think it had something to do with normalization and encoding combined with some double slashes stuff..! And then I think we had a couple of versions of some MS OS where "..." (three dots) was two directories up , a "convenient shorthand" for "../..", which of course also opens up a whole raft of interesting security issues. Possibly check against that too, or?
        Hide
        Greg Wilkins added a comment -

        this "fix" did indeed open up a vulnerability and thus I'm backing out this change and instead have added an option to ContextHandler to compact all URLs like /foo///bar to /foo/bar. This was the handling is the same for files and for security constraints.

        Meanwhile, this makes 6.1.5 and 6.1.6 open to view static content in WEB-INF and behind security constraints.
        Release 6.1.7 is now being created with this fix. In the meanwhile that attached jar can be used to secure an existing installation as follows:

        1) copy the attached JETTY-386-6.1.5.jar in $JETTY_HOME/lib/ext The 6.1.5 version will also work for 6.1.6 and for hightide.

        2) Modify the jetty.xml so that the section that currently is

        <Set name="handler">
        <New id="Handlers" class="org.mortbay.jetty.handler.HandlerCollection">
        <Set name="handlers">
        <Array type="org.mortbay.jetty.Handler">
        <Item>
        <New id="Contexts" class="org.mortbay.jetty.handler.ContextHandlerCollection"/>
        </Item>
        <Item>
        <New id="DefaultHandler" class="org.mortbay.jetty.handler.DefaultHandler"/>
        </Item>
        <Item>
        <New id="RequestLog" class="org.mortbay.jetty.handler.RequestLogHandler"/>
        </Item>
        </Array>
        </Set>
        </New>
        </Set>

        is replaced with:

        <Set name="handler">
        <New class="org.mortbay.jetty.CompactHandler">
        <Set name="handler">
        <New id="Handlers" class="org.mortbay.jetty.handler.HandlerCollection">
        <Set name="handlers">
        <Array type="org.mortbay.jetty.Handler">
        <Item>
        <New id="Contexts" class="org.mortbay.jetty.handler.ContextHandlerCollection"/>
        </Item>
        <Item>
        <New id="DefaultHandler" class="org.mortbay.jetty.handler.DefaultHandler"/>
        </Item>
        <Item>
        <New id="RequestLog" class="org.mortbay.jetty.handler.RequestLogHandler"/>
        </Item>
        </Array>
        </Set>
        </New>
        </Set>
        </New>
        </Set>

        3) Restart your server

        Show
        Greg Wilkins added a comment - this "fix" did indeed open up a vulnerability and thus I'm backing out this change and instead have added an option to ContextHandler to compact all URLs like /foo///bar to /foo/bar. This was the handling is the same for files and for security constraints. Meanwhile, this makes 6.1.5 and 6.1.6 open to view static content in WEB-INF and behind security constraints. Release 6.1.7 is now being created with this fix. In the meanwhile that attached jar can be used to secure an existing installation as follows: 1) copy the attached JETTY-386 -6.1.5.jar in $JETTY_HOME/lib/ext The 6.1.5 version will also work for 6.1.6 and for hightide. 2) Modify the jetty.xml so that the section that currently is <Set name="handler"> <New id="Handlers" class="org.mortbay.jetty.handler.HandlerCollection"> <Set name="handlers"> <Array type="org.mortbay.jetty.Handler"> <Item> <New id="Contexts" class="org.mortbay.jetty.handler.ContextHandlerCollection"/> </Item> <Item> <New id="DefaultHandler" class="org.mortbay.jetty.handler.DefaultHandler"/> </Item> <Item> <New id="RequestLog" class="org.mortbay.jetty.handler.RequestLogHandler"/> </Item> </Array> </Set> </New> </Set> is replaced with: <Set name="handler"> <New class="org.mortbay.jetty.CompactHandler"> <Set name="handler"> <New id="Handlers" class="org.mortbay.jetty.handler.HandlerCollection"> <Set name="handlers"> <Array type="org.mortbay.jetty.Handler"> <Item> <New id="Contexts" class="org.mortbay.jetty.handler.ContextHandlerCollection"/> </Item> <Item> <New id="DefaultHandler" class="org.mortbay.jetty.handler.DefaultHandler"/> </Item> <Item> <New id="RequestLog" class="org.mortbay.jetty.handler.RequestLogHandler"/> </Item> </Array> </Set> </New> </Set> </New> </Set> 3) Restart your server

          People

          • Assignee:
            Unassigned
            Reporter:
            Jarek Gawor
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: