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 -

        According to RFC2396:

        path_segments = segment *( "/" segment )

        so // is significant in a URI
        So your URI has three segments:
        "test"
        ""
        "favicon.ico"

        Now it turns out that the implementation of FileURL Handler interprests
        "" segments as ".". But I can't see anywhere that is mandated.

        Jetty has a very strict policy on file aliases, because they can be used to pass security constraints.
        Eg. on VMS you can try hit /snoop.jsp;4 and the ;4 passes anything mapped to *.jsp. But for a VMS file
        system ;4 is a version number, so you will get the source code of the JSP!

        Thus by default, Jetty does not accept any file requests for aliases files - because who knows what strange
        file system it will be run on.

        Is it possible to get this test changed.... it looks to be a bug in the test to me.

        Show
        Greg Wilkins added a comment - According to RFC2396: path_segments = segment *( "/" segment ) so // is significant in a URI So your URI has three segments: "test" "" "favicon.ico" Now it turns out that the implementation of FileURL Handler interprests "" segments as ".". But I can't see anywhere that is mandated. Jetty has a very strict policy on file aliases, because they can be used to pass security constraints. Eg. on VMS you can try hit /snoop.jsp;4 and the ;4 passes anything mapped to *.jsp. But for a VMS file system ;4 is a version number, so you will get the source code of the JSP! Thus by default, Jetty does not accept any file requests for aliases files - because who knows what strange file system it will be run on. Is it possible to get this test changed.... it looks to be a bug in the test to me.
        Hide
        Greg Wilkins added a comment -

        Also in rfc1738 we have:

        3.10 FILES
        The file URL scheme is used to designate files accessible on a
        particular host computer. This scheme, unlike most other URL schemes,
        does not designate a resource that is universally accessible over the
        Internet.

        A file URL takes the form:

        file://<host>/<path>

        where <host> is the fully qualified domain name of the system on
        which the <path> is accessible, and <path> is a hierarchical
        directory path of the form <directory>/<directory>/.../<name>.

        So // definitely implies a directory, and that it completely file system depending if "" directory
        is equivalent to . or not.

        Show
        Greg Wilkins added a comment - Also in rfc1738 we have: 3.10 FILES The file URL scheme is used to designate files accessible on a particular host computer. This scheme, unlike most other URL schemes, does not designate a resource that is universally accessible over the Internet. A file URL takes the form: file:// <host>/<path> where <host> is the fully qualified domain name of the system on which the <path> is accessible, and <path> is a hierarchical directory path of the form <directory>/<directory>/.../<name>. So // definitely implies a directory, and that it completely file system depending if "" directory is equivalent to . or not.
        Hide
        Greg Wilkins added a comment -

        Let me say that again... in complete english sentances:

        So // implies a directory, and it is completely file system dependant if a "" directory
        is equivalent to "." or not.

        I'm loath to change anything in Jetty regarding this, as it is security related.
        So if it is at all possible to change the test, that would be best.

        Show
        Greg Wilkins added a comment - Let me say that again... in complete english sentances: So // implies a directory, and it is completely file system dependant if a "" directory is equivalent to "." or not. I'm loath to change anything in Jetty regarding this, as it is security related. So if it is at all possible to change the test, that would be best.
        Hide
        jgenender jgenender added a comment -

        Hi Greg. Unfortunately Sun will not fix the test unless the RFC specifically excludes the double slash. Their response is: "If the specs do not specifically indicate it is invalid, then I cannot really request a patch. I do not think I have ever seen another web server barf on that either."

        If you could fix this, it would be appreciated.

        Show
        jgenender jgenender added a comment - Hi Greg. Unfortunately Sun will not fix the test unless the RFC specifically excludes the double slash. Their response is: "If the specs do not specifically indicate it is invalid, then I cannot really request a patch. I do not think I have ever seen another web server barf on that either." If you could fix this, it would be appreciated.
        Hide
        Greg Wilkins added a comment -

        Jeff,

        I'm not saying that /foo//bar is invalid.

        I'm just saying that nowhere I can see implies that /foo//bar is equivalent to /foo/./bar .
        It is totally file system dependent what results you will get.

        In fact, who is to say that java cannot be run on a platform that allows the empty
        string as a directory name and that /foo/bar and /foo//bar could be files from completely
        different directories.

        Could you ask them one more time?

        I will however look to see if I can provide an optional hack to allow this.

        Show
        Greg Wilkins added a comment - Jeff, I'm not saying that /foo//bar is invalid. I'm just saying that nowhere I can see implies that /foo//bar is equivalent to /foo/./bar . It is totally file system dependent what results you will get. In fact, who is to say that java cannot be run on a platform that allows the empty string as a directory name and that /foo/bar and /foo//bar could be files from completely different directories. Could you ask them one more time? I will however look to see if I can provide an optional hack to allow this.
        Hide
        Greg Wilkins added a comment -

        So anybody know what the canonical form of /foo//../bar is meant to be?
        /foo/bar or /bar?

        Show
        Greg Wilkins added a comment - So anybody know what the canonical form of /foo//../bar is meant to be? /foo/bar or /bar?
        Hide
        David Jencks added a comment -

        I just cooked up this little unit test which passes for me on sun jdk 1.5:

        public void testNormalize() throws Exception

        { assertEquals(URI.create("/foo/bar/baz"), URI.create("/foo//bar//baz").normalize()); assertEquals(URI.create("/baz"), URI.create("/foo//../bar//../baz").normalize()); }

        I think this indicates that the canonical form of /foo//../bar is /bar and that // is equivalent to /.

        I don't understand why you think file systems have anything to do with this question. I don't see a file URL coming into the server, and surely the details of the file system that some static files happen to be located in should not be propagated into the server behavior? If you mapped that URL to a jsp page then would you expect the behavior to change?

        Show
        David Jencks added a comment - I just cooked up this little unit test which passes for me on sun jdk 1.5: public void testNormalize() throws Exception { assertEquals(URI.create("/foo/bar/baz"), URI.create("/foo//bar//baz").normalize()); assertEquals(URI.create("/baz"), URI.create("/foo//../bar//../baz").normalize()); } I think this indicates that the canonical form of /foo//../bar is /bar and that // is equivalent to /. I don't understand why you think file systems have anything to do with this question. I don't see a file URL coming into the server, and surely the details of the file system that some static files happen to be located in should not be propagated into the server behavior? If you mapped that URL to a jsp page then would you expect the behavior to change?
        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!
        Greg Wilkins made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 6.1.5rc0 [ 13601 ]
        Resolution Fixed [ 1 ]
        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
        Greg Wilkins made changes -
        Attachment CompactHandler.java [ 31424 ]
        Greg Wilkins made changes -
        Attachment JETTY-386-6.1.5.jar [ 31425 ]
        Greg Wilkins made changes -
        Fix Version/s 6.1.7 [ 13950 ]
        Fix Version/s 6.1.5rc0 [ 13601 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved: