Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UriCompliance.Violation ignored despite being set #11448

Closed
puskarpeter opened this issue Feb 26, 2024 · 20 comments · Fixed by #11457
Closed

UriCompliance.Violation ignored despite being set #11448

puskarpeter opened this issue Feb 26, 2024 · 20 comments · Fixed by #11457
Labels
Bug For general bugs on Jetty side

Comments

@puskarpeter
Copy link

Jetty version(s)

12.0.6

Jetty Environment

ee10

Java version/vendor (use: java -version)
openjdk 17.0.10 2024-01-16
OpenJDK Runtime Environment Temurin-17.0.10+7 (build 17.0.10+7)
OpenJDK 64-Bit Server VM Temurin-17.0.10+7 (build 17.0.10+7, mixed mode, sharing)

OS type/version
Ubuntu 22.04 LTS

Description

Jetty throws Error 400 Ambiguous URI encoding despite the violation being configured to be ignored.

How to reproduce?

Spring Boot 3.2.3 app using spring-boot-starter-jersey with embedded jetty.
Configure Jetty like so:

@Component
public class JettyCustomizer implements WebServerFactoryCustomizer<JettyServletWebServerFactory> {

    @Override
    public void customize(JettyServletWebServerFactory factory) {
        factory.addServerCustomizers(this::customizeUriCompliance);
    }

    private void customizeUriCompliance(Server server) {
        for (Connector connector : server.getConnectors()) {
            connector.getConnectionFactories().stream()
                    .filter(factory -> factory instanceof HttpConnectionFactory)
                    .forEach(factory -> {
                        HttpConfiguration httpConfig = ((HttpConnectionFactory) factory).getHttpConfiguration();
                        httpConfig.setUriCompliance(UriCompliance.from(Set.of(
                                UriCompliance.Violation.AMBIGUOUS_PATH_SEPARATOR,
                                UriCompliance.Violation.AMBIGUOUS_PATH_ENCODING)));
                    });
        }
    }
}

Sample endpoint:

@Path("/")
public class DemoController {

    @GET
    @Path("/{sequence: .+}")
    @Produces(MediaType.APPLICATION_JSON)
    public Map<String, String> parseKeyValueSequence(@PathParam("sequence") String sequence) {
        Map<String, String> keyValueMap = new HashMap<>();
        String[] pairs = sequence.split("/");
        for (String pair : pairs) {
            String[] keyValue = pair.split("=");
            if (keyValue.length == 2) {
                keyValueMap.put(keyValue[0], keyValue[1]);
            }
        }
        return keyValueMap;
    }
}

Then call curl --location 'http://localhost:8080/foo=bar%2F1'

Even though the configuration is in place, I still get 400: Ambiguous URI encoding due to the %2F used in the URI. I expected this to pass as it previously did on Jetty 11 with default behavior which had the same set of violations.

Now I was able to see in the debugger that this method returns null on the request as I believe it should:

    public static String checkUriCompliance(UriCompliance compliance, HttpURI uri)
    {
        for (UriCompliance.Violation violation : UriCompliance.Violation.values())
        {
            if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation)))
                return violation.getDescription();
        }
        return null;
    }

But the Servlet still throws away the request and returns the following:

<html>
<head>
<meta http-equiv="Content-Type" content="text/html;charset=ISO-8859-1"/>
<title>Error 400 Ambiguous URI encoding</title>
</head>
<body><h2>HTTP ERROR 400 Ambiguous URI encoding</h2>
<table>
<tr><th>URI:</th><td>/foo=bar%2F1</td></tr>
<tr><th>STATUS:</th><td>400</td></tr>
<tr><th>MESSAGE:</th><td>Ambiguous URI encoding</td></tr>
<tr><th>SERVLET:</th><td>jettyreproducer.demo.MyJerseyConfig</td></tr>
<tr><th>CAUSED BY:</th><td>org.eclipse.jetty.http.HttpException$IllegalArgumentException: 400: Ambiguous URI encoding</td></tr>
</table>
<h3>Caused by:</h3><pre>org.eclipse.jetty.http.HttpException$IllegalArgumentException: 400: Ambiguous URI encoding
	at org.eclipse.jetty.ee10.servlet.ServletApiRequest$AmbiguousURI.getServletPath(ServletApiRequest.java:1325)
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:236)
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHolder.handle(ServletHolder.java:736)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1614)
	at org.eclipse.jetty.ee10.websocket.servlet.WebSocketUpgradeFilter.doFilter(WebSocketUpgradeFilter.java:195)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1586)
	at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1586)
	at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1586)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$MappedServlet.handle(ServletHandler.java:1547)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.dispatch(ServletChannel.java:805)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.handle(ServletChannel.java:431)
	at org.eclipse.jetty.ee10.servlet.ServletHandler.handle(ServletHandler.java:464)
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:571)
	at org.eclipse.jetty.ee10.servlet.SessionHandler.handle(SessionHandler.java:703)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:763)
	at org.eclipse.jetty.server.Server.handle(Server.java:179)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:619)
	at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:410)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:322)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:971)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1201)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1156)
	at java.base/java.lang.Thread.run(Thread.java:1583)
</pre>

</body>
</html>

I can provided more information if needed. Thanks for the help in advance.

@puskarpeter puskarpeter added the Bug For general bugs on Jetty side label Feb 26, 2024
@joakime
Copy link
Contributor

joakime commented Feb 26, 2024

Might be a duplicate of Issue #11361 being handled in PR #11444

@puskarpeter
Copy link
Author

Okay thanks, I will retest as soon as the PR is included in the next version and will close this after verifying it works as expected.

@joakime
Copy link
Contributor

joakime commented Feb 27, 2024

@puskarpeter do you have an example spring-boot project that can demonstrate this reported issue?

@puskarpeter
Copy link
Author

@puskarpeter do you have an example spring-boot project that can demonstrate this reported issue?

Here is minimal reproducer for both Jetty 11 and 12 to compare their behavior:
https://github.com/puskarpeter/jetty-ambiguous-uri-reproducer

@joakime
Copy link
Contributor

joakime commented Feb 27, 2024

Thank you for your demo project.

So here's what's happening.

The Spring HttpClient is encoding the %2F before sending it.

On the network, the path is received as /foo=bar%252Fbaz=baz.
That specific scenario is being seen as an AMBIGUOUS_PATH_ENCODING violation, because the resulting decode of % when applying URI rules.

Eventually, it reaches Jersey's ServletContainer and the following stacktrace occurs.

org.eclipse.jetty.http.HttpException$IllegalArgumentException: 400: Ambiguous URI encoding
	at org.eclipse.jetty.ee10.servlet.ServletApiRequest$AmbiguousURI.getServletPath(ServletApiRequest.java:1325)
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:236)
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHolder.handle(ServletHolder.java:736)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1614)
	at org.eclipse.jetty.ee10.websocket.servlet.WebSocketUpgradeFilter.doFilter(WebSocketUpgradeFilter.java:195)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1586)
	at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:100)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1586)
	at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1586)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$MappedServlet.handle(ServletHandler.java:1547)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.dispatch(ServletChannel.java:805)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.handle(ServletChannel.java:431)
	at org.eclipse.jetty.ee10.servlet.ServletHandler.handle(ServletHandler.java:464)
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:571)
	at org.eclipse.jetty.ee10.servlet.SessionHandler.handle(SessionHandler.java:703)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:763)
	at org.eclipse.jetty.server.Server.handle(Server.java:179)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:619)
	at org.eclipse.jetty.server.internal.HttpConnection.onFillable(HttpConnection.java:410)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:322)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:971)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1201)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1156)
	at java.base/java.lang.Thread.run(Thread.java:1583)

This is coming from the following piece of code...

protected ServletApiRequest newServletApiRequest()
{
if (getHttpURI().hasViolations() && !getServletChannel().getServletContextHandler().getServletHandler().isDecodeAmbiguousURIs())
{
// TODO we should check if current compliance mode allows all the violations?
for (UriCompliance.Violation violation : getHttpURI().getViolations())
{
if (UriCompliance.AMBIGUOUS_VIOLATIONS.contains(violation))
return new ServletApiRequest.AmbiguousURI(this);
}
}
return new ServletApiRequest(this);
}

This kind of URI path is actually tripping over many things around path encoding and decoding in the Servlet spec.
These were largely resolved in time for Servlet 6.0 as part of the discussion in jakartaee/servlet#18

Not sure if we can support / allow these kinds of ambiguous URLs through safely into the Servlet spec APIs.
@gregw what's your thoughts on this?

@puskarpeter
Copy link
Author

So are you saying this is expected to fail even though I've explicitly configured those violations to be allowed as per the behavior that is configured in Jetty 11 and can be seen in the picture below?
compliance
The thing is that our application logic handles '%2f' differently from '/' as it is used as a part of an identifier and we need this distinction to be able to tell whether '/' is part of an identifier or it separates 2 URI segments.

@joakime
Copy link
Contributor

joakime commented Feb 27, 2024

How you construct your java.net.URI object has a lot to do with it.
Also, the HTTP client you are using has yet another impact on how that input string is handled.

Take this updated testcase for Jetty12

That shows the same HTTP endpoint being accessed via 5 different URI construction techniques, using 2 different HTTP clients.

First thing you'll notice is that the java.net.URI construction technique you use will impact if the path portion is encoded BEFORE it is even sent to the HttpClient.
The assertion assertThat("Oops, your URI technique is encoding the percent sign.", uri.toASCIIString(), not(containsString("%25"))); tests for this, feel free to comment it out.

The next thing you'll see is that the Jersey TestRestTemplate will also encode the URI again before sending it over the wire.

If you construct your URI properly, and send it via an HttpClient that doesn't encode before sending the Path then you'll get the results you want. A passing test!

Otherwise you'll wind up with /foo=bar%252Fbaz=baz on the wire, which is squarely falling into the ambiguous URL logic from the Servlet spec simply because it's not possible to represent %252F in encoded vs decoded form in the various parts of the Servlet spec, and this causes problems with url-patterns, constraints, security, etc... (see the discussion at jakartaee/servlet#18)

@joakime
Copy link
Contributor

joakime commented Feb 27, 2024

The thing is that our application logic handles '%2f' differently from '/' as it is used as a part of an identifier and we need this distinction to be able to tell whether '/' is part of an identifier or it separates 2 URI segments.

This is still possible, and the updated testcases show this is possible.
https://github.com/joakime/jetty-ambiguous-uri-reproducer/blob/uri-construction/jetty12/src/test/java/jetty12/Jetty12Test.java

You just have to be extra careful how you test it.
Using the Jersey TestRestTemplate or the Spring URI construction techniques (org.springframework.web.util.UriComponentsBuilder) will both mangle the URI that is sent over the wire to the point where it is ambiguous to the Servlet spec.
If you send the request using something that doesn't mangle the URI path and leaves it at /foo=bar%2Fbaz=baz (note this is %2F, not %252F) on the network it will work as you want.

@puskarpeter
Copy link
Author

Thanks for pointing out this double encoding problem, I guess that also might be the case in our integration test suite then, as we are invoking the requests via requests library from python. I will double-check that specified behavior and if it works then only it might be the case of fixing that and clarifying the docs. 😌

@joakime
Copy link
Contributor

joakime commented Feb 27, 2024

@gregw at a minimum I think we should double check the error handling presentation of URIs.

The error text this triggers reads as following (for text/plain)

URI: /foo=bar%2Fbaz=baz
STATUS: 400
MESSAGE: Ambiguous URI encoding
SERVLET: jetty12.DemoJerseyConfig
CAUSED BY org.eclipse.jetty.http.HttpException$IllegalArgumentException: 400: Ambiguous URI encoding
org.eclipse.jetty.http.HttpException$IllegalArgumentException: 400: Ambiguous URI encoding
\tat org.eclipse.jetty.ee10.servlet.ServletApiRequest$AmbiguousURI.getServletPath(ServletApiRequest.java:1325)
\tat org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:236)
\tat org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:205)
\tat org.eclipse.jetty.ee10.servlet.ServletHolder.handle(ServletHolder.java:736)
\tat org.eclipse.jetty.ee10 ...

I notice that the URI presented on the Error output isn't the raw URI path, but the decoded URI path.

This is actually the correct on-network raw URI path.
But the Exception message is confusing, making it seems like it's a AMBIGUOUS_PATH_ENCODING fault, when in actuality it is a AMBIGUOUS_PATH_SEPARATOR fault.

@joakime
Copy link
Contributor

joakime commented Feb 27, 2024

@gregw a quick PR to make the Errors more useful can be found at PR #11457

@puskarpeter
Copy link
Author

I just checked the python requests behavior and it seems like it is not performing any additional encoding so it should be sent over the network as following:
image
Yet 400 is still returned 🤔

@puskarpeter
Copy link
Author

image

Just to make sure that Burp does not perform any encoding/decoding too.

joakime added a commit that referenced this issue Feb 28, 2024
* Some testing of HttpURI for Issue #11448
* Issue #11448 - improved stacktrace message for ambiguous URI
@puskarpeter
Copy link
Author

Why was this closed? I get that the message is now improved but to me, it seems that it is still ignoring the configured violation modes, I even tried the UNSAFE and LEGACY and it still fails with 400 from various HTTP clients (Java, Python, Postman, curl). Also in that test case, you are doing POST request for GET endpoint, not sure if that is also in spec and does not cause any problems. Thanks

@joakime
Copy link
Contributor

joakime commented Feb 28, 2024

Sorry, it was automatic from merging PR #11457

@joakime joakime reopened this Feb 28, 2024
@joakime
Copy link
Contributor

joakime commented Feb 28, 2024

Warning! The following is a Terrible Idea!

@puskarpeter see the latest changes to your /jetty12/ tree at https://github.com/joakime/jetty-ambiguous-uri-reproducer/tree/uri-construction
This will show how to bypass the Servlet 6 rules on Ambiguous URIs. Pay attention to the test case changes, and the JettyCustomizer.

Servlet 6 (Jetty 12 / ee10) clarified a ton of vague language and behaviors around a bunch of core concepts.
Back in Servlet 5 (Jetty 11 or Jetty 12 / ee9) these were fundamentally broken and lead to a long array of security issues and broken libraries that worked with Servlet 5.

If you want to work in Servlet 6, and want to send something like "%2F" then don't use that in the path section of the URI, use it in the query section.
This is how the Servlet 6 spec works.

To summarize, Servlet 6 has changed how the URI path section works (for encoding / decoding / path parameters / normalization / equivalence / etc)

The following APIs are impacted.

  • HttpServletRequest.getRequestURI()
  • HttpServletRequest.getContextPath()
  • HttpServletRequest.getServletPath()
  • HttpServletRequest.getPathInfo()
  • HttpServletRequest.getRequestDispatcher(String)

The first 4 APIs in that list are critical for proper operation of Jersey, you really do not want to mess with them in the technique demonstated in the above git repository.

For more information, see Servlet 6 changes around URI path (encoding / decoding / normalization / equivalence / etc):

@puskarpeter
Copy link
Author

You are correct, I just read it here:

Rejecting Suspicious Sequences.
If suspicious sequences are discovered during the prior processing steps, the request must be
rejected with a 400 bad request rather than dispatched to the target servlet. If a context is matched
then the error handling of the context may be used to generate the 400 response. By default, the set
of suspicious sequences is defined below, but may be configured differently by a container:
◦ The encoded "/" character (e.g. /path%2Finfo)

I guess Jersey is first to follow the suit with strict parsing since in MVC it still works (I pushed an example into my repo)

@joakime
Copy link
Contributor

joakime commented Feb 28, 2024

For others that find this issue in the future, here's the Servlet 6 Spec documentation.

https://github.com/jakartaee/servlet/blob/6.0.0-RELEASE/spec/src/main/asciidoc/servlet-spec-body.adoc#352-uri-path-canonicalization

See 3.5.2. URI Path Canonicalization and point 10 in that section in particular.

@janbartel
Copy link
Contributor

I think this issue can be closed now?

@joakime
Copy link
Contributor

joakime commented Mar 6, 2024

Yes, it can be closed.

@joakime joakime closed this as completed Mar 6, 2024
paulrutter added a commit to blueconic/felix-dev that referenced this issue Apr 19, 2024
- Add `org.eclipse.jetty.UriComplianceMode` option
- Possible values are: DEFAULT, LEGACY, RFC3986, UNAMBIGUOUS, UNSAFE
- When UNSAFE or LEGACY is used, also apply workaround as suggested in jetty/jetty.project#11448 (comment). This is not Servlet API 6 compliant though, so use on your own risk.
cziegeler pushed a commit to apache/felix-dev that referenced this issue Apr 29, 2024
* FELIX-6698 Ability to configure URI Compliance mode
- Add `org.eclipse.jetty.UriComplianceMode` option
- Possible values are: DEFAULT, LEGACY, RFC3986, UNAMBIGUOUS, UNSAFE
- When UNSAFE or LEGACY is used, also apply workaround as suggested in jetty/jetty.project#11448 (comment). This is not Servlet API 6 compliant though, so use on your own risk.

* FELIX-6698 Ability to configure URI Compliance mode
- Add UNAMBIGUOUS

* Code review comments

* Code review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants