Stricter header value parsing

apache · Feb 4, 2020 · 8bfb0ff · 8bfb0ff
1 parent 060ecc5
commit 8bfb0ff
Show file tree

Hide file tree

Showing 8 changed files with 163 additions and 42 deletions.
diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
@@ -145,27 +145,56 @@ public void setAllowHostHeaderMismatch(boolean allowHostHeaderMismatch) {
     }
 
 
-    private boolean rejectIllegalHeaderName = true;
+    private boolean rejectIllegalHeader = true;
     /**
-     * If an HTTP request is received that contains an illegal header name (i.e.
-     * the header name is not a token) will the request be rejected (with a 400
-     * response) or will the illegal header be ignored.
+     * If an HTTP request is received that contains an illegal header name or
+     * value (e.g. the header name is not a token) will the request be rejected
+     * (with a 400 response) or will the illegal header be ignored?
      *
      * @return {@code true} if the request will be rejected or {@code false} if
      *         the header will be ignored
      */
-    public boolean getRejectIllegalHeaderName() { return rejectIllegalHeaderName; }
+    public boolean getRejectIllegalHeader() { return rejectIllegalHeader; }
     /**
-     * If an HTTP request is received that contains an illegal header name (i.e.
-     * the header name is not a token) should the request be rejected (with a
-     * 400 response) or should the illegal header be ignored.
+     * If an HTTP request is received that contains an illegal header name or
+     * value (e.g. the header name is not a token) should the request be
+     * rejected (with a 400 response) or should the illegal header be ignored?
+     *
+     * @param rejectIllegalHeader   {@code true} to reject requests with illegal
+     *                              header names or values, {@code false} to
+     *                              ignore the header
+     */
+    public void setRejectIllegalHeader(boolean rejectIllegalHeader) {
+        this.rejectIllegalHeader = rejectIllegalHeader;
+    }
+    /**
+     * If an HTTP request is received that contains an illegal header name or
+     * value (e.g. the header name is not a token) will the request be rejected
+     * (with a 400 response) or will the illegal header be ignored?
+     *
+     * @return {@code true} if the request will be rejected or {@code false} if
+     *         the header will be ignored
+     *
+     * @deprecated Now an alias for {@link #getRejectIllegalHeader()}. Will be
+     *             removed in Tomcat 10 onwards.
+     */
+    @Deprecated
+    public boolean getRejectIllegalHeaderName() { return rejectIllegalHeader; }
+    /**
+     * If an HTTP request is received that contains an illegal header name or
+     * value (e.g. the header name is not a token) should the request be
+     * rejected (with a 400 response) or should the illegal header be ignored?
      *
      * @param rejectIllegalHeaderName   {@code true} to reject requests with
-     *                                  illegal header names, {@code false} to
-     *                                  ignore the header
+     *                                  illegal header names or values,
+     *                                  {@code false} to ignore the header
+     *
+     * @deprecated Now an alias for {@link #setRejectIllegalHeader(boolean)}.
+     *             Will be removed in Tomcat 10 onwards.
      */
+    @Deprecated
     public void setRejectIllegalHeaderName(boolean rejectIllegalHeaderName) {
-        this.rejectIllegalHeaderName = rejectIllegalHeaderName;
+        this.rejectIllegalHeader = rejectIllegalHeaderName;
     }
 
 

diff --git a/java/org/apache/coyote/http11/Http11InputBuffer.java b/java/org/apache/coyote/http11/Http11InputBuffer.java
@@ -66,7 +66,7 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
     private final MimeHeaders headers;
 
 
-    private final boolean rejectIllegalHeaderName;
+    private final boolean rejectIllegalHeader;
 
     /**
      * State.
@@ -152,13 +152,13 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
     // ----------------------------------------------------------- Constructors
 
     public Http11InputBuffer(Request request, int headerBufferSize,
-            boolean rejectIllegalHeaderName, HttpParser httpParser) {
+            boolean rejectIllegalHeader, HttpParser httpParser) {
 
         this.request = request;
         headers = request.getMimeHeaders();
 
         this.headerBufferSize = headerBufferSize;
-        this.rejectIllegalHeaderName = rejectIllegalHeaderName;
+        this.rejectIllegalHeader = rejectIllegalHeader;
         this.httpParser = httpParser;
 
         filterLibrary = new InputFilter[0];
@@ -762,6 +762,8 @@ private HeaderParseStatus parseHeader() throws IOException {
         //
 
         byte chr = 0;
+        byte prevChr = 0;
+
         while (headerParsePos == HeaderParsePosition.HEADER_START) {
 
             // Read new bytes if needed
@@ -772,17 +774,23 @@ private HeaderParseStatus parseHeader() throws IOException {
                 }
             }
 
+            prevChr = chr;
             chr = byteBuffer.get();
 
-            if (chr == Constants.CR) {
-                // Skip
-            } else if (chr == Constants.LF) {
+            if (chr == Constants.CR && prevChr != Constants.CR) {
+                // Possible start of CRLF - process the next byte.
+            } else if (prevChr == Constants.CR && chr == Constants.LF) {
                 return HeaderParseStatus.DONE;
             } else {
-                byteBuffer.position(byteBuffer.position() - 1);
+                if (prevChr == 0) {
+                    // Must have only read one byte
+                    byteBuffer.position(byteBuffer.position() - 1);
+                } else {
+                    // Must have read two bytes (first was CR, second was not LF)
+                    byteBuffer.position(byteBuffer.position() - 2);
+                }
                 break;
             }
-
         }
 
         if (headerParsePos == HeaderParsePosition.HEADER_START) {
@@ -879,11 +887,22 @@ private HeaderParseStatus parseHeader() throws IOException {
                         }
                     }
 
+                    prevChr = chr;
                     chr = byteBuffer.get();
                     if (chr == Constants.CR) {
-                        // Skip
-                    } else if (chr == Constants.LF) {
+                        // Possible start of CRLF - process the next byte.
+                    } else if (prevChr == Constants.CR && chr == Constants.LF) {
                         eol = true;
+                    } else if (prevChr == Constants.CR) {
+                        // Invalid value
+                        // Delete the header (it will be the most recent one)
+                        headers.removeHeader(headers.size() - 1);
+                        return skipLine();
+                    } else if (chr != Constants.HT && HttpParser.isControl(chr)) {
+                        // Invalid value
+                        // Delete the header (it will be the most recent one)
+                        headers.removeHeader(headers.size() - 1);
+                        return skipLine();
                     } else if (chr == Constants.SP || chr == Constants.HT) {
                         byteBuffer.put(headerData.realPos, chr);
                         headerData.realPos++;
@@ -935,6 +954,9 @@ private HeaderParseStatus skipLine() throws IOException {
         headerParsePos = HeaderParsePosition.HEADER_SKIPLINE;
         boolean eol = false;
 
+        byte chr = 0;
+        byte prevChr = 0;
+
         // Reading bytes until the end of the line
         while (!eol) {
 
@@ -946,20 +968,21 @@ private HeaderParseStatus skipLine() throws IOException {
             }
 
             int pos = byteBuffer.position();
-            byte chr = byteBuffer.get();
+            prevChr = chr;
+            chr = byteBuffer.get();
             if (chr == Constants.CR) {
                 // Skip
-            } else if (chr == Constants.LF) {
+            } else if (prevChr == Constants.CR && chr == Constants.LF) {
                 eol = true;
             } else {
                 headerData.lastSignificantChar = pos;
             }
         }
-        if (rejectIllegalHeaderName || log.isDebugEnabled()) {
+        if (rejectIllegalHeader || log.isDebugEnabled()) {
             String message = sm.getString("iib.invalidheader",
                     HeaderUtil.toPrintableString(byteBuffer.array(), headerData.lineStart,
                             headerData.lastSignificantChar - headerData.lineStart + 1));
-            if (rejectIllegalHeaderName) {
+            if (rejectIllegalHeader) {
                 throw new IllegalArgumentException(message);
             }
             log.debug(message);

diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
@@ -156,7 +156,7 @@ public Http11Processor(AbstractHttp11Protocol<?> protocol, Adapter adapter) {
                 protocol.getRelaxedQueryChars());
 
         inputBuffer = new Http11InputBuffer(request, protocol.getMaxHttpHeaderSize(),
-                protocol.getRejectIllegalHeaderName(), httpParser);
+                protocol.getRejectIllegalHeader(), httpParser);
         request.setInputBuffer(inputBuffer);
 
         outputBuffer = new Http11OutputBuffer(response, protocol.getMaxHttpHeaderSize());

diff --git a/java/org/apache/tomcat/util/http/MimeHeaders.java b/java/org/apache/tomcat/util/http/MimeHeaders.java
@@ -395,7 +395,7 @@ public void removeHeader(String name) {
      * reset and swap with last header
      * @param idx the index of the header to remove.
      */
-    private void removeHeader(int idx) {
+    public void removeHeader(int idx) {
         MimeHeaderField mh = headers[idx];
 
         mh.recycle();

diff --git a/java/org/apache/tomcat/util/http/parser/HttpParser.java b/java/org/apache/tomcat/util/http/parser/HttpParser.java
@@ -317,6 +317,17 @@ public static boolean isQuery(int c) {
     }
 
 
+    public static boolean isControl(int c) {
+        // Fast for valid control characters, slower for some incorrect
+        // ones
+        try {
+            return IS_CONTROL[c];
+        } catch (ArrayIndexOutOfBoundsException ex) {
+            return false;
+        }
+    }
+
+
     // Skip any LWS and position to read the next character. The next character
     // is returned as being able to 'peek()' it allows a small optimisation in
     // some cases.

diff --git a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
@@ -163,27 +163,52 @@ public void testBug51557NoColon() {
 
 
     @Test
-    public void testBug51557Separators() throws Exception {
+    public void testBug51557SeparatorsInName() throws Exception {
         char httpSeparators[] = new char[] {
                 '\t', ' ', '\"', '(', ')', ',', '/', ':', ';', '<',
                 '=', '>', '?', '@', '[', '\\', ']', '{', '}' };
 
         for (char s : httpSeparators) {
-            doTestBug51557Char(s);
+            doTestBug51557CharInName(s);
             tearDown();
             setUp();
         }
     }
 
 
     @Test
-    public void testBug51557Ctl() throws Exception {
+    public void testBug51557CtlInName() throws Exception {
         for (int i = 0; i < 31; i++) {
-            doTestBug51557Char((char) i);
+            doTestBug51557CharInName((char) i);
+            tearDown();
+            setUp();
+        }
+        doTestBug51557CharInName((char) 127);
+    }
+
+
+    @Test
+    public void testBug51557CtlInValue() throws Exception {
+        for (int i = 0; i < 31; i++) {
+            if (i == '\t') {
+                // TAB is allowed
+                continue;
+            }
+            doTestBug51557InvalidCharInValue((char) i);
+            tearDown();
+            setUp();
+        }
+        doTestBug51557InvalidCharInValue((char) 127);
+    }
+
+
+    @Test
+    public void testBug51557ObsTextInValue() throws Exception {
+        for (int i = 128; i < 255; i++) {
+            doTestBug51557ValidCharInValue((char) i);
             tearDown();
             setUp();
         }
-        doTestBug51557Char((char) 127);
     }
 
 
@@ -226,7 +251,7 @@ public void testBug51557BoundaryEnd() {
     }
 
 
-    private void doTestBug51557Char(char s) {
+    private void doTestBug51557CharInName(char s) {
         Bug51557Client client =
             new Bug51557Client("X-Bug" + s + "51557", "invalid");
 
@@ -236,30 +261,53 @@ private void doTestBug51557Char(char s) {
         Assert.assertTrue(client.isResponseBodyOK());
     }
 
+
+    private void doTestBug51557InvalidCharInValue(char s) {
+        Bug51557Client client =
+            new Bug51557Client("X-Bug51557-Invalid", "invalid" + s + "invalid");
+
+        client.doRequest();
+        Assert.assertTrue("Testing [" + (int) s + "]", client.isResponse200());
+        Assert.assertEquals("Testing [" + (int) s + "]", "abcd", client.getResponseBody());
+        Assert.assertTrue(client.isResponseBodyOK());
+    }
+
+
+    private void doTestBug51557ValidCharInValue(char s) {
+        Bug51557Client client =
+            new Bug51557Client("X-Bug51557-Valid", "valid" + s + "valid");
+
+        client.doRequest();
+        Assert.assertTrue("Testing [" + (int) s + "]", client.isResponse200());
+        Assert.assertEquals("Testing [" + (int) s + "]", "valid" + s + "validabcd", client.getResponseBody());
+        Assert.assertTrue(client.isResponseBodyOK());
+    }
+
+
     /**
      * Bug 51557 test client.
      */
     private class Bug51557Client extends SimpleHttpClient {
 
         private final String headerName;
         private final String headerLine;
-        private final boolean rejectIllegalHeaderName;
+        private final boolean rejectIllegalHeader;
 
         public Bug51557Client(String headerName) {
             this.headerName = headerName;
             this.headerLine = headerName;
-            this.rejectIllegalHeaderName = false;
+            this.rejectIllegalHeader = false;
         }
 
         public Bug51557Client(String headerName, String headerValue) {
             this(headerName, headerValue, false);
         }
 
         public Bug51557Client(String headerName, String headerValue,
-                boolean rejectIllegalHeaderName) {
+                boolean rejectIllegalHeader) {
             this.headerName = headerName;
             this.headerLine = headerName + ": " + headerValue;
-            this.rejectIllegalHeaderName = rejectIllegalHeaderName;
+            this.rejectIllegalHeader = rejectIllegalHeader;
         }
 
         private Exception doRequest() {
@@ -274,7 +322,7 @@ private Exception doRequest() {
             try {
                 Connector connector = tomcat.getConnector();
                 Assert.assertTrue(connector.setProperty(
-                        "rejectIllegalHeaderName", Boolean.toString(rejectIllegalHeaderName)));
+                        "rejectIllegalHeader", Boolean.toString(rejectIllegalHeader)));
                 tomcat.start();
                 setPort(connector.getLocalPort());
 
@@ -548,7 +596,7 @@ private Exception doRequest() {
 
             try {
                 Connector connector = tomcat.getConnector();
-                Assert.assertTrue(connector.setProperty("rejectIllegalHeaderName", "false"));
+                Assert.assertTrue(connector.setProperty("rejectIllegalHeader", "false"));
                 tomcat.start();
                 setPort(connector.getLocalPort());
 

diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -169,6 +169,11 @@
         the <code>Transfer-Encoding</code> header were ignored rather than
         treated as an error. (markt)
       </fix>
+      <fix>
+        Rename the HTTP Connector attribute <code>rejectIllegalHeaderName</code>
+        to <code>rejectIllegalHeader</code> and expand the underlying
+        implementation to include header values as well as names. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">