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

Ignore bad percent encodings in paths during URIUtil.equalsIgnoreEncodings() #4033

Closed wilkinsona closed 5 years ago

wilkinsona commented 5 years ago

The problem is illustrated by the following test:

@Test
public void equalsIgnoreEncodingsHandlesPercentCorrectly() {
    assertThat(URIUtil.equalsIgnoreEncodings("test%25path", "test%path")).isTrue();
}

It fails with a NumberFormatException:

java.lang.NumberFormatException: !hex p
    at org.eclipse.jetty.util.TypeUtil.convertHexDigit(TypeUtil.java:373)
    at org.eclipse.jetty.util.URIUtil.equalsIgnoreEncodings(URIUtil.java:1028)
    …

I discovered this while investigating this Spring Boot issue to see if Jetty was affected in any way as well. A more user-facing symptom of the problem in URIUtil or TypeUtil is, I believe, that a static resource in META-INF/resources that includes certain reserved characters in its name will be inaccessible.

joakime commented 5 years ago

Good catch. I'll submit a PR for this shortly.

joakime commented 5 years ago

Opened PR #4034 to address this with a lenient percent decoding solution. This will leave bad sequences alone when decoding.

joakime commented 5 years ago

@wilkinsona we have a fix in place on PR #4034 that seems to address the issue you are encountering. But without a longer stack, I can't tell where specifically your usage of URIUtil.equalsIgnoreEncodings(String,String) is coming from.

In our codebase, that method only used in test cases and FileResource which deprecated and not used by anything internally in Jetty anymore. You could be using the FileResource improperly in your code, when you should be using it's replacement PathResource (which solves for many of these kinds of encoding issues).

wilkinsona commented 5 years ago

@joakime Thanks for taking a look at this so quickly. Sorry for the context that was lost in the overly minimal example of the problem. When the problem occurs in Jetty proper, equalsIgnoreEncodings(String, String) is being called via equalsIgnoreEncodings(URI, URI). Here's a full stack of the NumberFormatException in that case:

java.lang.NumberFormatException: !hex &
    at org.eclipse.jetty.util.TypeUtil.convertHexDigit(TypeUtil.java:373)
    at org.eclipse.jetty.util.URIUtil.equalsIgnoreEncodings(URIUtil.java:1023)
    at org.eclipse.jetty.util.URIUtil.equalsIgnoreEncodings(URIUtil.java:1068)
    at org.eclipse.jetty.util.resource.PathResource.checkAliasPath(PathResource.java:74)
    at org.eclipse.jetty.util.resource.PathResource.<init>(PathResource.java:217)
    at org.eclipse.jetty.util.resource.PathResource.addPath(PathResource.java:300)
    at org.eclipse.jetty.util.resource.ResourceCollection.addPath(ResourceCollection.java:265)
    at org.eclipse.jetty.server.handler.ContextHandler.getResource(ContextHandler.java:1913)
    at org.eclipse.jetty.webapp.WebAppContext.getResource(WebAppContext.java:407)
    at org.eclipse.jetty.servlet.DefaultServlet.getResource(DefaultServlet.java:430)
    at org.eclipse.jetty.server.ResourceContentFactory.getContent(ResourceContentFactory.java:62)
    at org.eclipse.jetty.server.ResourceService.doGet(ResourceService.java:235)
    at org.eclipse.jetty.servlet.DefaultServlet.doGet(DefaultServlet.java:457)
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:645)
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:750)
    at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:876)
    at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1623)
    at org.eclipse.jetty.websocket.server.WebSocketUpgradeFilter.doFilter(WebSocketUpgradeFilter.java:214)
    at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1610)
    at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:200)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:118)
    at org.eclipse.jetty.servlet.ServletHandler$CachedChain.doFilter(ServletHandler.java:1610)
    at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:540)
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:146)
    at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:548)
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:132)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:257)
    at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1711)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:255)
    at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1347)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:203)
    at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:480)
    at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1678)
    at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:201)
    at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1249)
    at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:144)
    at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:132)
    at org.eclipse.jetty.server.Server.handle(Server.java:505)
    at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:370)
    at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:267)
    at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:305)
    at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:103)
    at org.eclipse.jetty.io.ChannelEndPoint$2.run(ChannelEndPoint.java:117)
    at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:781)
    at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:917)
    at java.lang.Thread.run(Thread.java:748)

The call to equalsIgnoreEncodings(String,String) is reached because the input URIs are not equal:

file:///private/var/folders/jj/5zwjflp915gg3gyqdkgqwnf40000gn/T/jetty-docbase.2190897992002719274.0/nested-reserved-!%23$%25&%27()*+,/:%3B=%3F@%5B%5D-meta-inf-resource.txt
file:///private/var/folders/jj/5zwjflp915gg3gyqdkgqwnf40000gn/T/jetty-docbase.2190897992002719274.0/nested-reserved-!%23$%25&'()*+,/:;=%3F@%5B%5D-meta-inf-resource.txt

The ' and ; characters are encoded in the first URI but not in the second.

joakime commented 5 years ago

Oh, wow, cool test.

What does that file name (and parent path with nested-reserved-....) look like on disk, from a ls -la perspective? I need to know which characters are stored as-is, and which are escaped by the java.nio.files.FileSystem. Meanwhile, I'll merge the current lenient decode PR, and work on a new PR that tests the PathResource better (using similar techniques like i'm seeing in your updated comment)

wilkinsona commented 5 years ago

What does that file name (and parent path with nested-reserved-....) look like on disk, from a ls -la perspective?

In this particular case, the file isn't on disk. It's in META-INF/resources of a jar that's on the classpath. The code in Jetty that looks for that isn't being reached due to the NumberFormatException that's thrown when looking for the file on disk beneath the doc base.

joakime commented 5 years ago

@wilkinsona there's a 9.4.21-SNAPSHOT build available with these Lenient decode changes at the standard Jetty snapshot repository.

https://oss.sonatype.org/content/repositories/jetty-snapshots/

Can you give it whirl on your tests?

wilkinsona commented 5 years ago

I've tried the 9.4.21 snapshot and the changes look good to me, @joakime. Thanks!

joakime commented 5 years ago

Thanks for the feedback! I'll consider this complete then.

joakime commented 5 years ago

@wilkinsona this fix is on the schedule for the Jetty 9.4.21 release.

gregw commented 5 years ago

I am worried that this fix has gone to far.

Previously a request for /%foo/bar% would result in a 400 bad request. However we are now being lenient on the original decode so that such requests are allowed into the container! The typically receive 404 responses... but can get 200s depending on the handler. I think this is a very unintended consequence of fixing a NPE in equalsIgnoreEncodings

joakime commented 5 years ago

URIUtil.equalsIgnoreEncodings() only applies to alias checking on PathResource and FileResource.

joakime commented 5 years ago

Opened PR #4272

wilkinsona commented 3 years ago

There seems to have been a change in this area in Jetty 10 or 11. Before opening a new issue, I thought it best to mention it here to hopefully learn if it's intentional. A test that tries to access a static resource named nested-reserved-!%23$%25&%27()*+,/:%3B=%3F@%5B%5D-meta-inf-resource.txt that passed with Jetty 9 (200 response) is failing with Jetty 11 (400 response).

joakime commented 3 years ago

What is the static filename (in the raw, without URI encoding/decoding) on your filesystem (be it a real filesystem, network filesystem, or jar/zip filesystem)

Is it nested-reserved-!#$%&'()*+,/:;=?@[]-meta-inf-resource.txt ?

I use this tiny project to test encoding/decoding and static resources ... https://github.com/joakime/maniacal-servlet-encoding

So far, I can access all of the content in the jar file.

wilkinsona commented 3 years ago

Thanks, @joakime. The file's in a jar file and it's named META-INF/resources/nested-reserved-!#\$%&()*+,:=?@[]-meta-inf-resource.txt. I'm working on a branch of Spring Boot that upgrades to Jakarta EE 9 and, among many other things, Jetty 11 is part of that. Now that I know that this should still work, let me dig a bit and see if I can pinpoint what's changed. I'll open a new issue with a reproducer once I've confirmed that something in Jetty is the cause.