backport c83fe47 to fix CVE-2023-45648

tomitribe · Nov 13, 2023 · f33e1c3 · f33e1c3
1 parent 89b7017
commit f33e1c3
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 6 deletions.
diff --git a/java/org/apache/coyote/http11/InternalAprInputBuffer.java b/java/org/apache/coyote/http11/InternalAprInputBuffer.java
@@ -467,7 +467,7 @@ private boolean parseHeader() throws IOException {
                     headers.removeHeader(headers.size() - 1);
                     skipLine(start);
                     return true;
-                } else if (chr != Constants.HT && HttpParser.isControl(chr)) {
+                } else if (HttpParser.isControl(chr) && chr != Constants.HT) {
                     // Invalid value
                     // Delete the header (it will be the most recent one)
                     headers.removeHeader(headers.size() - 1);

diff --git a/java/org/apache/coyote/http11/InternalInputBuffer.java b/java/org/apache/coyote/http11/InternalInputBuffer.java
@@ -421,7 +421,7 @@ private boolean parseHeader() throws IOException {
                     // Delete the header (it will be the most recent one)
                     skipLine(start,true);
                     return true;
-                } else if (chr != Constants.HT && HttpParser.isControl(chr)) {
+                } else if (HttpParser.isControl(chr) && chr != Constants.HT) {
                     // Invalid value
                     // Delete the header (it will be the most recent one)
                     skipLine(start,true);

diff --git a/java/org/apache/coyote/http11/InternalNioInputBuffer.java b/java/org/apache/coyote/http11/InternalNioInputBuffer.java
@@ -521,6 +521,12 @@ public boolean parseHeaders()
     private HeaderParseStatus parseHeader()
         throws IOException {
 
+        /*
+         * Implementation note: Any changes to this method probably need to be echoed in
+         * ChunkedInputFilter.parseHeader(). Why not use a common implementation? In short, this code uses non-blocking
+         * reads whereas ChunkedInputFilter using blocking reads. The code is just different enough that a common
+         * implementation wasn't viewed as practical.
+         */
         while (headerParsePos == HeaderParsePosition.HEADER_START) {
 
             // Read new bytes if needed
@@ -651,7 +657,7 @@ private HeaderParseStatus parseHeader()
                     } else if (prevChr == Constants.CR) {
                         // Invalid value - also need to delete header
                         return skipLine(true);
-                    } else if (chr != Constants.HT && HttpParser.isControl(chr)) {
+                    } else if (HttpParser.isControl(chr) && chr != Constants.HT) {
                         // Invalid value - also need to delete header
                         return skipLine(true);
                     } else if (chr == Constants.SP || chr == Constants.HT) {

diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java
@@ -31,6 +31,7 @@
 import org.apache.tomcat.util.buf.MessageBytes;
 import org.apache.tomcat.util.http.MimeHeaders;
 import org.apache.tomcat.util.res.StringManager;
+import org.apache.tomcat.util.http.parser.HttpParser;
 
 /**
  * Chunked input filter. Parses chunked data according to
@@ -453,7 +454,13 @@ protected void parseEndChunk() throws IOException {
         }
     }
 
-
+    /*
+     * Implementation note: Any changes to this method probably need to be echoed in
+     * Http11InputBuffer.parseHeader(). Why not use a common implementation? In short, this code uses blocking
+     * reads whereas Http11InputBuffer using non-blocking reads. The code is just different enough that a common
+     * implementation wasn't viewed as practical.
+     */
+
     private boolean parseHeader() throws IOException {
 
         MimeHeaders headers = request.getMimeHeaders();
@@ -500,6 +507,9 @@ private boolean parseHeader() throws IOException {
 
             if (chr == Constants.COLON) {
                 colon = true;
+            } else if (!HttpParser.isToken(chr)) {
+                // Non-token characters are illegal in header names
+                throw new IOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName"));
             } else {
                 trailingHeaders.append(chr);
             }
@@ -561,7 +571,9 @@ private boolean parseHeader() throws IOException {
                 if (chr == Constants.CR || chr == Constants.LF) {
                     parseCRLF(true);
                     eol = true;
-                } else if (chr == Constants.SP) {
+                } else if (HttpParser.isControl(chr) && chr != Constants.HT) {
+                    throw new IOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderValue"));
+                } else if (chr == Constants.SP || chr == Constants.HT) {
                     trailingHeaders.append(chr);
                 } else {
                     trailingHeaders.append(chr);

diff --git a/java/org/apache/coyote/http11/filters/LocalStrings.properties b/java/org/apache/coyote/http11/filters/LocalStrings.properties
@@ -24,4 +24,7 @@ chunkedInputFilter.invalidHeader=Invalid chunk header
 chunkedInputFilter.maxExtension=maxExtensionSize exceeded
 chunkedInputFilter.maxTrailer=maxTrailerSize exceeded
 
-inputFilter.maxSwallow=maxSwallowSize exceeded
+inputFilter.maxSwallow=maxSwallowSize exceeded
+
+chunkedInputFilter.invalidTrailerHeaderName=Invalid trailer header name (non-token character in name)
+chunkedInputFilter.invalidTrailerHeaderValue=Invalid trailer header value (control character in value)
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
@@ -332,6 +332,9 @@
       <fix>
         Make timing attacks against the Realm implementations harder. (schultz)
       </fix>
+      <fix>
+        Align validation of HTTP trailer fields with standard fields. (markt)
+      </fix>
     </changelog>
   </subsection>
   <subsection name="Jasper">