search

jetty / jetty.project

Eclipse Jetty® - Web Container & Clients - supports HTTP/2, HTTP/1.1, HTTP/1.0, websocket, servlets, and more
https://eclipse.dev/jetty
Other
3.88k stars 1.91k forks source link

URIUtil.canonicalPath incorrectly handles paths with query parameters containing '../' #1079

Closed ontology-rory closed 7 years ago

ontology-rory commented 8 years ago

When preforming some penetration testing of our web app, we discovered several NullPointerExceptions in the log with pages getting 500 errors. We are using Jetty 9.3.8

java.lang.NullPointerException at org.eclipse.jetty.server.Response.sendRedirect(Response.java:671) at org.eclipse.jetty.server.Response.sendRedirect(Response.java:691) at org.apache.wicket.protocol.http.servlet.ServletWebResponse.sendRedirect(ServletWebResponse.java:297)

This was due to URIUtil.canonicalPath returning NULL for the path "/foo/bar?10&5=../../../../../../../../../../../../../../../../etc/passwd". I would have expected the path to be returned AS IS, as the ../ sequence are in the query parameters rather than the query info. For example

joakime commented 7 years ago

@gregw should this be fixed by making URIUtil.canonicalPath() query string aware? Or by making change locally in Response.sendRedirect() only, applying the URIUtil.canonicalPath() to the path portion only (ignoring the query portion)

joakime commented 7 years ago

This is trivially easy to replicate.

Just add the following test condition to the URIUtilTest#testCanonicalPath() use cases

{"/foo/bar?10&5=../../../../etc/passwd", "/foo/bar?10&5=../../../../etc/passwd"},
gregw commented 7 years ago

I'll look at the rfc early next week. I'm not sure if / is legal in a query string and it may need to be encoded.

But returning null is wrong. It should be left as is or a malformed uri exception from canonical path.

Send redirect should also better handle a null return.

On 15 Dec 2016 07:27, "Joakim Erdfelt" notifications@github.com wrote:

This is trivially easy to replicate.

Just add the following test condition to the URIUtilTest#testCanonicalPath() use cases

{"/foo/bar?10&5=../../../../etc/passwd", "/foo/bar?10&5=../../../../etc/passwd"},

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/eclipse/jetty.project/issues/1079#issuecomment-267146989, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEUrQZysQGbAS0Y7-pHeGwOr4_Eibcaks5rIFFIgaJpZM4KrY3P .

joakime commented 7 years ago

The query portion can contain "/" unencoded.

See https://tools.ietf.org/html/rfc3986#section-3.4

gregw commented 7 years ago

Fixed by #1487 and #1480