[tomcat] branch 7.0.x 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 7.0.x 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 7.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git


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

commit f15c077cdd1080d6f3d46be0d47f82978d9c03b9
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
---
 .../coyote/http11/InternalAprInputBuffer.java      | 13 ++--
 .../apache/coyote/http11/InternalInputBuffer.java  | 13 ++--
 .../coyote/http11/InternalNioInputBuffer.java      | 17 +++--
 .../coyote/http11/TestHttp11InputBufferCRLF.java   | 73 +++++++++++++++++++---
 webapps/docs/changelog.xml                         |  5 ++
 5 files changed, 99 insertions(+), 22 deletions(-)

diff --git a/java/org/apache/coyote/http11/InternalAprInputBuffer.java b/java/org/apache/coyote/http11/InternalAprInputBuffer.java
index b2e9a41..bfa769e 100644
--- a/java/org/apache/coyote/http11/InternalAprInputBuffer.java
+++ b/java/org/apache/coyote/http11/InternalAprInputBuffer.java
@@ -238,8 +238,12 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> {
                 // HTTP/0.9 style request. CR is optional. LF is not.
             } else if (buf[pos] == Constants.LF) {
                 // HTTP/0.9 style request
-                eol = true;
+                // Stop this processing loop
                 space = true;
+                // Set blank protocol (indicates HTTP/0.9)
+                request.protocol().setString("");
+                // Skip the protocol processing
+                eol = true;
                 if (buf[pos - 1] == Constants.CR) {
                     end = pos - 1;
                 } else {
@@ -314,12 +318,13 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> {
 
         if ((end - start) > 0) {
             request.protocol().setBytes(buf, start, end - start);
-        } else {
-            request.protocol().setString("");
         }
 
-        return true;
+        if (request.protocol().isNull()) {
+            throw new IllegalArgumentException(sm.getString("iib.invalidHttpProtocol"));
+        }
 
+        return true;
     }
 
 
diff --git a/java/org/apache/coyote/http11/InternalInputBuffer.java b/java/org/apache/coyote/http11/InternalInputBuffer.java
index e3f36d6..1050e57 100644
--- a/java/org/apache/coyote/http11/InternalInputBuffer.java
+++ b/java/org/apache/coyote/http11/InternalInputBuffer.java
@@ -192,8 +192,12 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> {
                 // HTTP/0.9 style request. CR is optional. LF is not.
             } else if (buf[pos] == Constants.LF) {
                 // HTTP/0.9 style request
-                eol = true;
+                // Stop this processing loop
                 space = true;
+                // Set blank protocol (indicates HTTP/0.9)
+                request.protocol().setString("");
+                // Skip the protocol processing
+                eol = true;
                 if (buf[pos - 1] == Constants.CR) {
                     end = pos - 1;
                 } else {
@@ -266,12 +270,13 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> {
 
         if ((end - start) > 0) {
             request.protocol().setBytes(buf, start, end - start);
-        } else {
-            request.protocol().setString("");
         }
 
-        return true;
+        if (request.protocol().isNull()) {
+            throw new IllegalArgumentException(sm.getString("iib.invalidHttpProtocol"));
+        }
 
+        return true;
     }
 
 
diff --git a/java/org/apache/coyote/http11/InternalNioInputBuffer.java b/java/org/apache/coyote/http11/InternalNioInputBuffer.java
index 6b80d01..3cdbdfa 100644
--- a/java/org/apache/coyote/http11/InternalNioInputBuffer.java
+++ b/java/org/apache/coyote/http11/InternalNioInputBuffer.java
@@ -322,10 +322,12 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> {
                     // HTTP/0.9 style request. CR is optional. LF is not.
                 } else if (buf[pos] == Constants.LF) {
                     // HTTP/0.9 style request
-                    parsingRequestLineEol = true;
+                    // Stop this processing loop
                     space = true;
+                    // Set blank protocol (indicates HTTP/0.9)
+                    request.protocol().setString("");
                     // Skip the protocol processing
-                    parsingRequestLinePhase = 6;
+                    parsingRequestLinePhase = 7;
                     if (buf[pos - 1] == Constants.CR) {
                         end = pos - 1;
                     } else {
@@ -352,7 +354,9 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> {
             } else {
                 request.requestURI().setBytes(buf, 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;
             }
         }
@@ -402,9 +406,12 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> {
 
             if ( (end - parsingRequestLineStart) > 0) {
                 request.protocol().setBytes(buf, 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 1acb03e..d674ac6 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<String>();
@@ -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<String>();
         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 b6f6239..5c75e31 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -75,6 +75,11 @@
         <code>URIEncoding</code> is not a superset of US-ASCII as required by
         RFC7230. (markt)
       </add>
+      <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]