EE10 multipart parsing may include '\r' at the front under certain conditions

[Boris-de]

Jetty version(s)
12.0.3

Jetty Environment
ee10

Java version/vendor

openjdk version "21.0.1" 2023-10-17 LTS
OpenJDK Runtime Environment Temurin-21.0.1+12 (build 21.0.1+12-LTS)
OpenJDK 64-Bit Server VM Temurin-21.0.1+12 (build 21.0.1+12-LTS, mixed mode, sharing)

OS type/version
Arch Linux

Description
Under certain conditions the multi-part form parsing will add a wrong '\r' character at the front of the content. The issue seems to only occur for requests in "chunked" transfer encoding and depends on the position in the multi-part data (see tests below).

I am not 100% sure about the specifics of mime-multipart, so it could also be an error in the request. But the issue did not occur in previous versions of jetty (at the very least not in version 11 where this works), it also does not occur in "ee9" environment and not when sending to the real target (some old Perl-CGI).

How to reproduce?

Adding this test to MultiPartServletTest.java will fail at assertEquals("abcde", content1) because content1 will contain "\rabcde" instead of the expected "abcde".

    // to be used in org.eclipse.jetty.ee10.servlet.MultiPartServletTest
    @Test
    public void testCRLFChunkedBug() throws Exception
    {
        start(new HttpServlet()
        {
            @Override
            protected void service(HttpServletRequest request, HttpServletResponse response1) throws ServletException, IOException
            {
                assertEquals(2, request.getParts().size());
                Part part = request.getPart("file_upload");
                final String content1 = IO.toString(part.getInputStream(), UTF_8);
                assertEquals("abcde", content1);
            }
        }, null, false);

        try (Socket socket = new Socket("localhost", connector.getLocalPort()))
        {
            OutputStream output = socket.getOutputStream();

            String content = """
              90\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
              Content-Disposition: form-data; name="az"\r
              Content-Type: text/plain; charset=ISO-8859-1\r
              \r
              upload_file\r
              \r
              94\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
              Content-Disposition: form-data; name="file_upload"; filename="testUpload.test"\r
              Content-Type: text/plain\r
              \r
              5\r
              abcde\r
              2\r
              \r
              28\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6--\r
              0\r
              \r
              """;
            String header = """
                POST / HTTP/1.1\r
                Host: localhost\r
                Transfer-encoding: chunked\r
                Content-Type: multipart/form-data; boundary=908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
                \r
                """;

            output.write(header.getBytes(UTF_8));
            output.write(content.getBytes(UTF_8));
            output.flush();

            HttpTester.Response response = HttpTester.parseResponse(socket.getInputStream());
            assertNotNull(response);
            assertEquals(HttpStatus.OK_200, response.getStatus());
        }
    }

If you move the file_upload part within the multipart-section to the top, it will work and give me "abcde". The code for this is included in the folded section below.

Additionally I created a testcase for the MultiPartServletTest.java of the ee9 module where the same request will have the expected result "abcde". This is also included in the folded section below.

More testcases

    // to be used in org.eclipse.jetty.ee10.servlet.MultiPartServletTest
    @Test
    public void testCRLFChunkedBug() throws Exception
    {
        start(new HttpServlet()
        {
            @Override
            protected void service(HttpServletRequest request, HttpServletResponse response1) throws ServletException, IOException
            {
                assertEquals(2, request.getParts().size());
                Part part = request.getPart("file_upload");
                final String content1 = IO.toString(part.getInputStream(), UTF_8);
                assertEquals("abcde", content1);
            }
        }, null, false);

        try (Socket socket = new Socket("localhost", connector.getLocalPort()))
        {
            OutputStream output = socket.getOutputStream();

            String content = """
              94\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
              Content-Disposition: form-data; name="file_upload"; filename="testUpload.test"\r
              Content-Type: text/plain\r
              \r
              5\r
              abcde\r
              2\r
              \r
              90\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
              Content-Disposition: form-data; name="az"\r
              Content-Type: text/plain; charset=ISO-8859-1\r
              \r
              upload_file\r
              \r
              28\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6--\r
              0\r
              \r
              """;
            String header = """
                POST / HTTP/1.1\r
                Host: localhost\r
                Transfer-encoding: chunked\r
                Content-Type: multipart/form-data; boundary=908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
                \r
                """;

            output.write(header.getBytes(UTF_8));
            output.write(content.getBytes(UTF_8));
            output.flush();

            HttpTester.Response response = HttpTester.parseResponse(socket.getInputStream());
            assertNotNull(response);
            assertEquals(HttpStatus.OK_200, response.getStatus());
        }
    }

Testcase for org.eclipse.jetty.ee9.servlet.MultiPartServletTest (this will work because the bug does not occur for ee9)

    // to be used in org.eclipse.jetty.ee9.servlet.MultiPartServletTest
    @Test
    public void testCRLFChunkedForEE9_NoBug() throws Exception
    {
        MultipartConfigElement config = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), MAX_FILE_SIZE, -1, 1);
        ServletHolder servletHolder = new ServletHolder(new HttpServlet() {
            @Override
            protected void service(HttpServletRequest request, HttpServletResponse resp) throws ServletException, IOException {
                assertEquals(2, request.getParts().size());
                Part part = request.getPart("file_upload");
                final String content1 = IO.toString(part.getInputStream(), UTF_8);
                assertEquals("abcde", content1);
            }
        });
        servletHolder.getRegistration().setMultipartConfig(config);
        contextHandler.addServlet(servletHolder, "/testCRLFChunkedBug");

        try (Socket socket = new Socket("localhost", connector.getLocalPort())) {
            OutputStream output = socket.getOutputStream();

            String content = """
              90\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
              Content-Disposition: form-data; name="az"\r
              Content-Type: text/plain; charset=ISO-8859-1\r
              \r
              upload_file\r
              \r
              94\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
              Content-Disposition: form-data; name="file_upload"; filename="testUpload.test"\r
              Content-Type: text/plain\r
              \r
              5\r
              abcde\r
              2\r
              \r
              28\r
              --908d442b-2c7d-401a-ab46-7c6ec6f89fe6--\r
              0\r
              \r
              """;
            String header = """
              POST /testCRLFChunkedBug HTTP/1.1\r
              Host: localhost\r
              Transfer-encoding: chunked\r
              Content-Type: multipart/form-data; boundary=908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r
              \r
              """;

            output.write(header.getBytes(UTF_8));
            output.write(content.getBytes(UTF_8));
            output.flush();

            HttpTester.Response response = HttpTester.parseResponse(socket.getInputStream());
            assertNotNull(response);
            assertEquals(HttpStatus.OK_200, response.getStatus());
        }
    }

[sbordet]

What generated the multipart content?
Is it Jetty's HttpClient generating that multipart content?

I ask because if I am not mistaken, the first chunk declares a length of 0x90 (144) that ends at upload_file\r\n, so there appear to be one extra \r\n after upload_file\r\n, just before the second chunk that starts with 94\r\n.

[sbordet]

I was mistaken.

Lost in the jungle of \r\n from both multipart and chunking 😄

[Boris-de]

No worries, thanks for having a look. I can relate, this combination is confusing 😀

And a good question, I should have mentioned that. The HTTP Client is the java.net.http.HttpClient class, but the multipart format generator is custom code. So the chunking is done by Java HttpClient and should be fine.

But to be honest: I changed the request to make it a bit smaller for the testcase, I wanted it to be as minimal as possibly and I could have broken something with that.
That being said: if the chunking is off, jetty will generate a different error and not even enter the servlet, so I would assume it is fine.

[sbordet]

@Boris-de care to try #10921?

[Boris-de]

@sbordet of course. I compiled with the change from #10921 and with this change all my tests are working. I also tested different boundary sizes to see if anything triggers this again, but everything I tried worked.
Thanks for your work.

[sbordet]

Fixed by #10921.