File _service:tar_scm:CVE-2020-1935.patch of Package tomcat

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).
 </attribute>
 
- <attribute name="rejectIllegalHeaderName" required="false">
- 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">
+ 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.
 </attribute>
 
+ <attribute name="rejectIllegalHeaderName" required="false">
+ This attribute is deprecated. It will be removed in Tomcat 10 onwards.
+ It is now an alias for rejectIllegalHeader.
+ </attribute>
+
 <attribute name="relaxedPathChars" required="false">
 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