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

Review UriCompliance.LEGACY and its omission of SUSPICIOUS_PATH_CHARACTERS #12809

Closed
joakime opened this issue Feb 20, 2025 · 4 comments · Fixed by #12827
Closed

Review UriCompliance.LEGACY and its omission of SUSPICIOUS_PATH_CHARACTERS #12809

joakime opened this issue Feb 20, 2025 · 4 comments · Fixed by #12827
Assignees
Labels
Bug For general bugs on Jetty side Sponsored This issue affects a user with a commercial support agreement

Comments

@joakime
Copy link
Contributor

joakime commented Feb 20, 2025

Jetty version(s)
12.0.16

Jetty Environment
Any

Java version/vendor
Any

OS type/version
Any

Description
The current UriCompliance.LEGACY is missing behavior that is similar to the SUSPICIOUS_PATH_CHARACTERS (but not quite exactly that violation).

Take for example, the following behavior on Jetty 11.0.24 ...

Configure a new jetty.base ...

[bases]$ mkdir jetty-11-demos-legacy
[bases]$ cd jetty-11-demos-legacy/

[jetty-11-demos-legacy]$ java -jar ../../jetty-home-11.0.24/start.jar --add-modules=server,http,deploy,demo
INFO  : mkdir ${jetty.base}/start.d
INFO  : webapp          transitively enabled, ini template available with --add-module=webapp
...(snip)...
INFO  : copy ${jetty.home}/modules/demo.d/root/images/webtide_logo.jpg to ${jetty.base}/webapps/root/images/webtide_logo.jpg
INFO  : Base directory was modified

[jetty-11-demos-legacy]$ gvim start.d/server.ini 
[jetty-11-demos-legacy]$ grep uriCompliance start.d/server.ini 
jetty.httpConfig.uriCompliance=LEGACY

Run that jetty.base ...

[jetty-11-demos-legacy]$ java -jar ../../jetty-home-11.0.24/start.jar 
2025-02-20 11:03:22.427:WARN :oe.jetty:main: demo-realm is deployed. DO NOT USE IN PRODUCTION!
2025-02-20 11:03:22.647:WARN :oejk.KeystoreGenerator:main: Generating Test Keystore: DO NOT USE IN PRODUCTION!
2025-02-20 11:03:23.162:INFO :oejs.Server:main: jetty-11.0.24; built: 2024-08-26T18:11:22.448Z; git: 5dfc59a691b748796f922208956bd1f2794bcd16; jvm 17.0.11+9
...(snip)...
2025-02-20 11:03:25.193:INFO :oejs.AbstractConnector:main: Started ServerConnector@4defd42{HTTP/1.1, (http/1.1)}{0.0.0.0:8080}
2025-02-20 11:03:25.201:INFO :oejs.Server:main: Started Server@79351f41{STARTING}[11.0.24,sto=5000] @3524ms

In a different console test a few suspicious character scenarios.

$ curl --no-progress-meter http://localhost:8080/test/dump/info | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/test/dump/info</td></tr><tr>

$ curl --no-progress-meter http://localhost:8080//test/dump/info | grep -E "(Bad Message|ERROR|getRequestURI)"
<body><h2>HTTP ERROR 404 Not Found</h2>

$ curl --no-progress-meter http://localhost:8080///test/dump/info | grep -E "(Bad Message|ERROR|getRequestURI)"
<body><h2>HTTP ERROR 404 Not Found</h2>

$ curl --no-progress-meter http://localhost:8080/test/dump/in%5Cfo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/test/dump/in%5Cfo</td></tr><tr>

$ curl --no-progress-meter http://localhost:8080/test/dump/in%0Afo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/test/dump/in%0Afo</td></tr><tr>

$ curl --no-progress-meter http://localhost:8080/test/dump/in%00fo | grep -E "(Bad Message|ERROR|getRequestURI)"
<h1>Bad Message 400</h1><pre>reason: Bad Request</pre>

$ curl --no-progress-meter http://localhost:8080/test/dump/in%01fo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/test/dump/in%01fo</td></tr><tr>

$ curl --no-progress-meter http://localhost:8080/test/dump/in%5Ffo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/test/dump/in_fo</td></tr><tr>

$ curl --no-progress-meter http://localhost:8080/test/dump/in%2Ffo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/test/dump/in/fo</td></tr><tr>

$ curl --no-progress-meter http://localhost:8080/test/dump/in%5C2Ffo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/test/dump/in%5C2Ffo</td></tr><tr>

Now lets do the same thing on Jetty 12.0.16

Setup and Run ...

[bases]$ mkdir jetty-12-demos-legacy
[bases]$ cd jetty-12-demos-legacy/
[jetty-12-demos-legacy]$ java -jar ../../jetty-home-12.0.16/start.jar --add-modules=server,http,ee10-deploy,ee10-demos
INFO  : mkdir ${jetty.base}/start.d
INFO  : alpn-java       transitively enabled
...(snip)...
INFO  : copy ${jetty.home}/modules/demo.d/demo-login.properties to ${jetty.base}/etc/demo-login.properties
INFO  : Base directory was modified

[jetty-12-demos-legacy]$ gvim start.d/server.ini 
[jetty-12-demos-legacy]$ grep uriCompliance start.d/server.ini 
jetty.httpConfig.uriCompliance=LEGACY

[jetty-12-demos-legacy]$ java -jar ../../jetty-home-12.0.16/start.jar
2025-02-20 11:58:42.995:WARN :oejk.KeystoreGenerator:main: Generating Test Keystore: DO NOT USE IN PRODUCTION!
2025-02-20 11:58:43.470:WARN :oejx.XmlConfiguration:main: Deprecated method public void org.eclipse.jetty.security.HashLoginService.setHotReload(boolean) in file:///home/joakim/code/jetty/distros/bases/jetty-12-demos-legacy/etc/jetty-demo-realm.xml
2025-02-20 11:58:43.473:WARN :oe.jetty:main: demo-realm is deployed. DO NOT USE IN PRODUCTION!
2025-02-20 11:58:43.551:INFO :oejs.Server:main: jetty-12.0.16; built: 2024-12-09T21:02:54.535Z; git: c3f88bafb4e393f23204dc14dc57b042e84debc7; jvm 17.0.11+9
...(snip)...
2025-02-20 11:58:46.286:INFO :oejs.AbstractConnector:main: Started ServerConnector@4d21c56e{HTTP/1.1, (http/1.1)}{0.0.0.0:8080}
2025-02-20 11:58:46.292:INFO :oejs.Server:main: Started oejs.Server@3e821657{STARTING}[12.0.16,sto=0] @4401ms

Do some initial testing ...

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/info | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/ee10-test/dump/info</td></tr><tr>

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/in%5Cfo | grep -E "(Bad Message|ERROR|getRequestURI)"
<h2>HTTP ERROR 400 Suspicious Path Character</h2>

The suspicious path characters are being rejected due to existing LEGACY behavior.
So lets add it via configuration.

[jetty-12-demos-legacy]$ gvim start.d/server.ini 
[jetty-12-demos-legacy]$ grep uriCompliance start.d/server.ini 
jetty.httpConfig.uriCompliance=LEGACY,SUSPICIOUS_PATH_CHARACTERS

[jetty-12-demos-legacy]$ java -jar ../../jetty-home-12.0.16/start.jar

It is now allowing suspicious path characters, but rejecting the other violations???

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/info | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/ee10-test/dump/info</td></tr><tr>

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/in%5Cfo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/ee10-test/dump/in%5Cfo</td></tr><tr>

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/in%5Ffo | grep -E "(Bad Message|ERROR|getRequestURI)"
<th align="right">getRequestURI:&nbsp;</th><td>/ee10-test/dump/in%5Ffo</td></tr><tr>

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/in%2Ffo | grep -E "(Bad Message|ERROR|getRequestURI)"
<body><h2>HTTP ERROR 400 Ambiguous URI encoding: AMBIGUOUS_PATH_SEPARATOR</h2>

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/in%252Ffo | grep -E "(Bad Message|ERROR|getRequestURI)"
<body><h2>HTTP ERROR 400 Ambiguous URI encoding: AMBIGUOUS_PATH_ENCODING</h2>

[~]$ curl --no-progress-meter http://localhost:8080/ee10-test/dump/in%25fo | grep -E "(Bad Message|ERROR|getRequestURI)"
<body><h2>HTTP ERROR 400 Ambiguous URI encoding: AMBIGUOUS_PATH_ENCODING</h2>

So we have a few things to look into.

  1. do we want to add SUSPICIOUS_PATH_CHARACTERS to LEGACY (even though it doesn't decode like Jetty 9/10/11)
  2. do we want to add a new SUSPICIOUS_PATH_CHARACTERS_WITH_AUTODECODE (or some similar name) to have the behavior be the same as Jetty 9/10/11 ?
  3. why is the jetty.httpConfig.uriCompliance=LEGACY,SUSPICIOUS_PATH_CHARACTERS resetting the other prior LEGACY violations?
@joakime joakime added the Bug For general bugs on Jetty side label Feb 20, 2025
@joakime joakime changed the title Review UriCompliance.LEGACY and its omission of SUSPICIOUS_PATH_CHARACTERS Review UriCompliance.LEGACY and its omission of SUSPICIOUS_PATH_CHARACTERS Feb 20, 2025
@joakime joakime added the Sponsored This issue affects a user with a commercial support agreement label Feb 20, 2025
@gregw
Copy link
Contributor

gregw commented Feb 25, 2025

@joakime I think the issue might be that the LEGACY compliance is not set up correctly. If I change it to:

    public static final UriCompliance LEGACY = new UriCompliance("LEGACY",
        of(Violation.AMBIGUOUS_PATH_SEGMENT,
            Violation.AMBIGUOUS_PATH_SEPARATOR,
            Violation.AMBIGUOUS_PATH_ENCODING,
            Violation.BAD_UTF8_ENCODING,
            Violation.SUSPICIOUS_PATH_CHARACTERS,
            Violation.UTF16_ENCODINGS,
            Violation.USER_INFO));

Then the following test passes in EE9 and EE10 in jetty-12.0.x:

    static Stream<Arguments> suspiciousCharactersLegacy()
    {
        return Stream.of(
            Arguments.of(UriCompliance.DEFAULT, "o", "o"),
            Arguments.of(UriCompliance.DEFAULT, "%5C", "400"),
            Arguments.of(UriCompliance.DEFAULT, "%0A", "400"),
            Arguments.of(UriCompliance.DEFAULT, "%00", "400"),
            Arguments.of(UriCompliance.DEFAULT, "%01", "400"),
            Arguments.of(UriCompliance.DEFAULT, "%5F", "_"),
            Arguments.of(UriCompliance.DEFAULT, "%2F", "400"),
            Arguments.of(UriCompliance.DEFAULT, "%252F", "400"),
            Arguments.of(UriCompliance.DEFAULT, "//", "400"),

            // these results are from jetty-11 DEFAULT
            Arguments.of(UriCompliance.LEGACY, "o", "o"),
            Arguments.of(UriCompliance.LEGACY, "%5C", "\\"),
            Arguments.of(UriCompliance.LEGACY, "%0A", "\n"),
            Arguments.of(UriCompliance.LEGACY, "%00", "400"),
            Arguments.of(UriCompliance.LEGACY, "%01", "\u0001"),
            Arguments.of(UriCompliance.LEGACY, "%5F", "_"),
            Arguments.of(UriCompliance.LEGACY, "%2F", "/"),
            Arguments.of(UriCompliance.LEGACY, "%252F", "%2F"),
            Arguments.of(UriCompliance.LEGACY, "//", "400")
        );
    }

    @ParameterizedTest
    @MethodSource("suspiciousCharactersLegacy")
    public void testSuspiciousCharactersLegacy(UriCompliance compliance, String suspect, String expected) throws Exception
    {
        startServer(new HttpServlet()
        {
            @Override
            protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
            {
                if (expected.length() != 3 || !Character.isDigit(expected.charAt(0)))
                    assertThat(request.getPathInfo(), is("/test/fo" + expected + "bar"));
            }
        });

        _connector.getBean(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(compliance);
        if (compliance == UriCompliance.LEGACY)
            _server.getBean(ServletContextHandler.class).getServletHandler().setDecodeAmbiguousURIs(true);
        String request = "GET /test/fo" + suspect + "bar HTTP/1.0\r\n" +
            "Host: whatever\r\n" +
            "\r\n";
        String response = _connector.getResponse(request);

        if (expected.length() == 3 && Character.isDigit(expected.charAt(0)))
        {
            assertThat(response, startsWith("HTTP/1.1 " + expected + " "));
        }
        else
        {
            assertThat(response, startsWith("HTTP/1.1 200 OK"));
        }
    }

@joakime
Copy link
Contributor Author

joakime commented Feb 25, 2025

I see.

Seems like adding SUSPICIOUS_PATH_CHARACTERS to LEGACY is going to be safe.

But what about the other behavior discovered with jetty-start?

[jetty-12-demos-legacy]$ grep uriCompliance start.d/server.ini 
jetty.httpConfig.uriCompliance=LEGACY,SUSPICIOUS_PATH_CHARACTERS

Doing that seemingly undid the other violations declared for LEGACY.
Even though the jetty.httpConfig.uriCompliance is using UriCompliance.from(String)

<Set name="uriCompliance"><Call class="org.eclipse.jetty.http.UriCompliance" name="from"><Arg><Property name="jetty.httpConfig.uriCompliance" default="DEFAULT"/></Arg></Call></Set>

Look for those 2 lines in the original issue text at the top of this issue, and read the actions before/after those lines to see the behavior I'm talking about.

@gregw
Copy link
Contributor

gregw commented Feb 26, 2025

@joakime my test is not seeing those issues. It correctly continues to allow AMBIGUOUS_PATH_SEPARATOR and AMBIGUOUS_PATH_ENCODING in legacy mode.
Let me create a branch so that you can experiment...

@gregw
Copy link
Contributor

gregw commented Feb 26, 2025

@joakime see #12827 Can you reproduce any bad behaviour?

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 Sponsored This issue affects a user with a commercial support agreement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants