[tomcat] branch master updated: Process HTTP/0.9 requests with extra request line data as HTTP/1.1

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

[tomcat] branch master updated: Process HTTP/0.9 requests with extra request line data as HTTP/1.1

markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/master by this push:
     new f4c3950  Process HTTP/0.9 requests with extra request line data as HTTP/1.1
f4c3950 is described below

commit f4c39506d8b0d5e37ea9f404d16b236cd1f7de70
Author: Mark Thomas <[hidden email]>
AuthorDate: Tue Mar 24 09:50:02 2020 +0000

    Process HTTP/0.9 requests with extra request line data as HTTP/1.1
   
    https://bz.apache.org/bugzilla/show_bug.cgi?id=64240
---
 .../apache/coyote/http11/Http11InputBuffer.java    | 19 ++++--
 .../coyote/http11/TestHttp11InputBufferCRLF.java   | 73 +++++++++++++++++++---
 webapps/docs/changelog.xml                         |  5 ++
 3 files changed, 83 insertions(+), 14 deletions(-)

diff --git a/java/org/apache/coyote/http11/Http11InputBuffer.java b/java/org/apache/coyote/http11/Http11InputBuffer.java
index 7be3e01..289a0bc 100644
--- a/java/org/apache/coyote/http11/Http11InputBuffer.java
+++ b/java/org/apache/coyote/http11/Http11InputBuffer.java
@@ -462,6 +462,9 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
                     request.protocol().setString(Constants.HTTP_11);
                     throw new IllegalArgumentException(sm.getString("iib.invalidRequestTarget"));
                 }
+                if (chr == '<') {
+                    System.out.println("debug");
+                }
                 if (chr == Constants.SP || chr == Constants.HT) {
                     space = true;
                     end = pos;
@@ -471,9 +474,10 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
                     // HTTP/0.9 style request
                     // Stop this processing loop
                     space = true;
+                    // Set blank protocol (indicates HTTP/0.9)
+                    request.protocol().setString("");
                     // Skip the protocol processing
-                    parsingRequestLinePhase = 6;
-                    parsingRequestLineEol = true;
+                    parsingRequestLinePhase = 7;
                     if (prevChr == Constants.CR) {
                         end = pos - 1;
                     } else {
@@ -504,7 +508,9 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
                 request.requestURI().setBytes(byteBuffer.array(), parsingRequestLineStart,
                         end - parsingRequestLineStart);
             }
-            if (!parsingRequestLineEol) {
+            // HTTP/0.9 processing jumps to stage 7.
+            // Don't want to overwrite that here.
+            if (parsingRequestLinePhase == 4) {
                 parsingRequestLinePhase = 5;
             }
         }
@@ -557,9 +563,12 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
             if ((end - parsingRequestLineStart) > 0) {
                 request.protocol().setBytes(byteBuffer.array(), parsingRequestLineStart,
                         end - parsingRequestLineStart);
-            } else {
-                request.protocol().setString("");
+                parsingRequestLinePhase = 7;
             }
+            // If no protocol is found, the ISE below will be triggered.
+        }
+        if (parsingRequestLinePhase == 7) {
+            // Parsing is complete. Return and clean-up.
             parsingRequestLine = false;
             parsingRequestLinePhase = 0;
             parsingRequestLineEol = false;
diff --git a/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
index 82315f5..ee033a1 100644
--- a/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
+++ b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
@@ -45,32 +45,74 @@ public class TestHttp11InputBufferCRLF extends TomcatBaseTest {
 
         // Requests to exercise code that allows HT in place of SP
         parameterSets.add(new Object[] { Boolean.FALSE, new String[] {
-                "GET\thttp://localhost:8080/test\tHTTP/1.1" + CRLF +
+                "GET\t/test\tHTTP/1.1" + CRLF +
                 "Host: localhost:8080" + CRLF +
-                "Connection: close" + CRLF +
-                CRLF } } );
+               "Connection: close" + CRLF +
+                CRLF }, Boolean.TRUE } );
 
         // Requests to simulate package boundaries
         // HTTP/0.9 request
         addRequestWithSplits("GET /test" + CRLF, Boolean.TRUE, parameterSets);
 
+        // HTTP/0.9 request with space
+        // Either malformed but acceptable HTTP/0.9 or invalid HTTP/1.1
+        // Tomcat opts for invalid HTTP/1.1
+        addRequestWithSplits("GET /test " + CRLF, Boolean.FALSE, Boolean.FALSE, parameterSets);
+
         // HTTP/0.9 request (no optional CR)
         addRequestWithSplits("GET /test" + LF, Boolean.TRUE, parameterSets);
 
+        // HTTP/0.9 request with space (no optional CR)
+        // Either malformed but acceptable HTTP/0.9 or invalid HTTP/1.1
+        // Tomcat opts for invalid HTTP/1.1
+        addRequestWithSplits("GET /test " + LF, Boolean.FALSE, Boolean.FALSE, parameterSets);
+
         // Standard HTTP/1.1 request
-        addRequestWithSplits("GET http://localhost:8080/test HTTP/1.1" + CRLF +
+        addRequestWithSplits("GET /test HTTP/1.1" + CRLF +
+                "Host: localhost:8080" + CRLF +
+                "Connection: close" + CRLF +
+                CRLF,
+                Boolean.FALSE, parameterSets);
+
+        // Invalid HTTP/1.1 request
+        addRequestWithSplits("GET /te<st HTTP/1.1" + CRLF +
+                "Host: localhost:8080" + CRLF +
+                "Connection: close" + CRLF +
+                CRLF,
+                Boolean.FALSE, Boolean.FALSE, parameterSets);
+
+        // Standard HTTP/1.1 request with a query string
+        addRequestWithSplits("GET /test?a=b HTTP/1.1" + CRLF +
                 "Host: localhost:8080" + CRLF +
                 "Connection: close" + CRLF +
                 CRLF,
                 Boolean.FALSE, parameterSets);
 
+        // Standard HTTP/1.1 request with a query string that includes ?
+        addRequestWithSplits("GET /test?a=?b HTTP/1.1" + CRLF +
+                "Host: localhost:8080" + CRLF +
+                "Connection: close" + CRLF +
+                CRLF,
+                Boolean.FALSE, parameterSets);
+
+        // Standard HTTP/1.1 request with an invalid query string
+        addRequestWithSplits("GET /test?a=<b HTTP/1.1" + CRLF +
+                "Host: localhost:8080" + CRLF +
+                "Connection: close" + CRLF +
+                CRLF,
+                Boolean.FALSE, Boolean.FALSE, parameterSets);
+
         return parameterSets;
     }
 
 
     private static void addRequestWithSplits(String request, Boolean isHttp09, List<Object[]> parameterSets) {
+        addRequestWithSplits(request, isHttp09, Boolean.TRUE, parameterSets);
+    }
+
+    private static void addRequestWithSplits(String request, Boolean isHttp09, Boolean valid, List<Object[]> parameterSets) {
         // Add as a single String
-        parameterSets.add(new Object[] { isHttp09, new String[] { request } } );
+        parameterSets.add(new Object[] { isHttp09, new String[] { request }, valid });
 
         // Add with all CRLF split between the CR and LF
         List<String> parts = new ArrayList<>();
@@ -82,14 +124,14 @@ public class TestHttp11InputBufferCRLF extends TomcatBaseTest {
             pos = request.indexOf("\n", lastPos + 1);
         }
         parts.add(request.substring(lastPos));
-        parameterSets.add(new Object[] { isHttp09, parts.toArray(new String[0]) });
+        parameterSets.add(new Object[] { isHttp09, parts.toArray(new String[0]), valid });
 
         // Add with a split between each character
         List<String> chars = new ArrayList<>();
         for (char c : request.toCharArray()) {
             chars.add(Character.toString(c));
         }
-        parameterSets.add(new Object[] { isHttp09, chars.toArray(new String[0]) });
+        parameterSets.add(new Object[] { isHttp09, chars.toArray(new String[0]), valid });
     }
 
     @Parameter(0)
@@ -98,13 +140,26 @@ public class TestHttp11InputBufferCRLF extends TomcatBaseTest {
     @Parameter(1)
     public String[] request;
 
+    @Parameter(2)
+    public boolean valid;
+
+
     @Test
     public void testBug54947() {
 
         Client client = new Client(request, isHttp09);
 
-        client.doRequest();
-        Assert.assertTrue(client.isResponseBodyOK());
+        Exception e = client.doRequest();
+
+        if (valid) {
+            Assert.assertTrue(client.isResponseBodyOK());
+        } else if (e == null){
+            Assert.assertTrue(client.isResponse400());
+        } else {
+            // The invalid request was detected before the entire request had
+            // been sent to Tomcat reset the connection when the client tried to
+            // continue sending the invalid request.
+        }
     }
 
 
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 1e8e357..14738ab 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -89,6 +89,11 @@
         Avoid always retrieving the NIO poller selection key when processing
         to reduce sync. (remm)
       </fix>
+      <fix>
+        <bug>64240</bug>: Ensure that HTTP/0.9 requests that contain additional
+        data on the request line after the URI are treated consistently. Such
+        requests will now always be treated as HTTP/1.1. (markt)
+      </fix>
     </changelog>
   </subsection>
 </section>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]