From 8bfb0ff7f25fe7555a5eb2f7984f73546c11aa26 Mon Sep 17 00:00:00 2001
From: Mark Thomas <markt@apache.org>
Date: Mon, 6 Jan 2020 20:53:25 +0000
Subject: [PATCH] Stricter header value parsing

---
 .../coyote/http11/AbstractHttp11Protocol.java | 51 ++++++++++---
 .../coyote/http11/Http11InputBuffer.java      | 51 +++++++++----
 .../apache/coyote/http11/Http11Processor.java |  2 +-
 .../apache/tomcat/util/http/MimeHeaders.java  |  2 +-
 .../tomcat/util/http/parser/HttpParser.java   | 11 +++
 .../coyote/http11/TestHttp11InputBuffer.java  | 74 +++++++++++++++----
 webapps/docs/config/http.xml                  | 11 ++-
 8 files changed, 164 insertions(+), 43 deletions(-)

diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
index e5ab885..9d10cbf 100644
--- a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
+++ b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
@@ -136,27 +136,56 @@ public abstract class AbstractHttp11Protocol<S> extends AbstractProtocol<S> {
     }
 
 
-    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
index 2dc7c17..57c670e 100644
--- a/java/org/apache/coyote/http11/Http11InputBuffer.java
+++ b/java/org/apache/coyote/http11/Http11InputBuffer.java
@@ -64,7 +64,7 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
     private final MimeHeaders headers;
 
 
-    private final boolean rejectIllegalHeaderName;
+    private final boolean rejectIllegalHeader;
 
     /**
      * State.
@@ -150,13 +150,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];
@@ -752,6 +752,8 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
         //
 
         byte chr = 0;
+        byte prevChr = 0;
+
         while (headerParsePos == HeaderParsePosition.HEADER_START) {
 
             // Read new bytes if needed
@@ -762,17 +764,23 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
                 }
             }
 
+            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) {
@@ -868,11 +876,22 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
                         }
                     }
 
+                    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++;
@@ -924,6 +943,9 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
         headerParsePos = HeaderParsePosition.HEADER_SKIPLINE;
         boolean eol = false;
 
+        byte chr = 0;
+        byte prevChr = 0;
+
         // Reading bytes until the end of the line
         while (!eol) {
 
@@ -935,21 +957,22 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
             }
 
             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",
                     new String(byteBuffer.array(), headerData.start,
                             headerData.lastSignificantChar - headerData.start + 1,
                             StandardCharsets.ISO_8859_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
index df002e0..86556ec 100644
--- a/java/org/apache/coyote/http11/Http11Processor.java
+++ b/java/org/apache/coyote/http11/Http11Processor.java
@@ -153,7 +153,7 @@ public class Http11Processor extends AbstractProcessor {
                 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
index 59504ee..b76b274 100644
--- a/java/org/apache/tomcat/util/http/MimeHeaders.java
+++ b/java/org/apache/tomcat/util/http/MimeHeaders.java
@@ -396,7 +396,7 @@ public class MimeHeaders {
      * 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
index 827f2c5..644b1d1 100644
--- 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 class HttpParser {
     }
 
 
+    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
index 131fa21..e60a474 100644
--- a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
+++ b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
@@ -163,13 +163,13 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
 
 
     @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();
         }
@@ -177,13 +177,38 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
 
 
     @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 class TestHttp11InputBuffer extends TomcatBaseTest {
     }
 
 
-    private void doTestBug51557Char(char s) {
+    private void doTestBug51557CharInName(char s) {
         Bug51557Client client =
             new Bug51557Client("X-Bug" + s + "51557", "invalid");
 
@@ -236,6 +261,29 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
         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.
      */
@@ -243,12 +291,12 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
 
         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) {
@@ -256,10 +304,10 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
         }
 
         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() {
@@ -273,8 +321,8 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
 
             try {
                 Connector connector = tomcat.getConnector();
-                connector.setProperty("rejectIllegalHeaderName",
-                        Boolean.toString(rejectIllegalHeaderName));
+                connector.setProperty("rejectIllegalHeader",
+                        Boolean.toString(rejectIllegalHeader));
                 tomcat.start();
                 setPort(connector.getLocalPort());
 
@@ -548,7 +596,7 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
 
             try {
                 Connector connector = tomcat.getConnector();
-                connector.setProperty("rejectIllegalHeaderName", "false");
+                connector.setProperty("rejectIllegalHeader", "false");
                 tomcat.start();
                 setPort(connector.getLocalPort());
 
diff --git a/webapps/docs/config/http.xml b/webapps/docs/config/http.xml
index ebb277d..3902c9a 100644
--- a/webapps/docs/config/http.xml
+++ b/webapps/docs/config/http.xml
@@ -551,14 +551,19 @@
       expected concurrent requests (synchronous and asynchronous).</p>
     </attribute>
 
-    <attribute name="rejectIllegalHeaderName" required="false">
-      <p>If an HTTP request is received that contains an illegal header name
-      (i.e. the header name is not a token) this setting determines if the
+    <attribute name="rejectIllegalHeader" required="false">
+      <p>If an HTTP request is received that contains an illegal header name or
+      value (e.g. the header name is not a token) this setting determines if the
       request will be rejected with a 400 response (<code>true</code>) or if the
       illegal header be ignored (<code>false</code>). The default value is
       <code>true</code> which will cause the request to be rejected.</p>
     </attribute>
 
+    <attribute name="rejectIllegalHeaderName" required="false">
+      <p>This attribute is deprecated. It will be removed in Tomcat 10 onwards.
+      It is now an alias for <strong>rejectIllegalHeader</strong>.</p>
+    </attribute>
+
     <attribute name="relaxedPathChars" required="false">
       <p>The <a href="https://tools.ietf.org/rfc/rfc7230.txt">HTTP/1.1
       specification</a> requires that certain characters are %nn encoded when
-- 
2.23.0

