Skip to content
This repository has been archived by the owner on Dec 4, 2018. It is now read-only.

Commit

Permalink
Improve processing of chuck size from chunked headers. Avoid overflow…
Browse files Browse the repository at this point in the history 
… and use a bit shift instead of a multiplication as it is marginally faster.

This is the fix for CVE-2014-0075


git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1578337 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@markt-asf
markt-asf committed Mar 17, 2014
1 parent c245925 commit d49a037
Show file tree
Hide file tree
Showing 3 changed files with 191 additions and 10 deletions.
11 changes: 5 additions & 6 deletions java/org/apache/coyote/http11/filters/ChunkedInputFilter.java
View file
Expand Up @@ -319,7 +319,7 @@ protected boolean parseChunkHeader()

int result = 0;
boolean eol = false;
boolean readDigit = false;
int readDigit = 0;
boolean extension = false;

while (!eol) {
Expand All @@ -341,10 +341,9 @@ protected boolean parseChunkHeader()
} else if (!extension) {
//don't read data after the trailer
int charValue = HexUtils.getDec(buf[pos]);
if (charValue != -1) {
readDigit = true;
result *= 16;
result += charValue;
if (charValue != -1 && readDigit < 8) {
readDigit++;
result = (result << 4) | charValue;
} else {
//we shouldn't allow invalid, non hex characters
//in the chunked header
Expand All @@ -367,7 +366,7 @@ protected boolean parseChunkHeader()

}

if (!readDigit)
if (readDigit == 0 || result < 0)
return false;

if (result == 0)
Expand Down
185 changes: 181 additions & 4 deletions test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java
View file
Expand Up @@ -105,7 +105,7 @@ private void doTestChunkingCRLF(boolean chunkHeaderUsesCRLF,
Context ctx =
tomcat.addContext("", System.getProperty("java.io.tmpdir"));

EchoHeaderServlet servlet = new EchoHeaderServlet();
EchoHeaderServlet servlet = new EchoHeaderServlet(expectPass);
Tomcat.addServlet(ctx, "servlet", servlet);
ctx.addServletMapping("/", "servlet");

Expand Down Expand Up @@ -169,7 +169,7 @@ public void testTrailingHeadersSizeLimit() throws Exception {
Context ctx =
tomcat.addContext("", System.getProperty("java.io.tmpdir"));

Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet());
Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet(false));
ctx.addServletMapping("/", "servlet");

// Limit the size of the trailing header
Expand Down Expand Up @@ -233,7 +233,7 @@ private void doTestExtensionSizeLimit(int len, boolean ok) throws Exception {
Context ctx =
tomcat.addContext("", System.getProperty("java.io.tmpdir"));

Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet());
Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet(ok));
ctx.addServletMapping("/", "servlet");

tomcat.start();
Expand Down Expand Up @@ -282,7 +282,7 @@ public void testNoTrailingHeaders() throws Exception {
Context ctx =
tomcat.addContext("", System.getProperty("java.io.tmpdir"));

Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet());
Tomcat.addServlet(ctx, "servlet", new EchoHeaderServlet(true));
ctx.addServletMapping("/", "servlet");

tomcat.start();
Expand Down Expand Up @@ -311,11 +311,136 @@ public void testNoTrailingHeaders() throws Exception {
assertEquals("nullnull7nullnull", client.getResponseBody());
}

@Test
public void testChunkSizeZero() throws Exception {
doTestChunkSize(true, true, "", 10, 0);
}

@Test
public void testChunkSizeAbsent() throws Exception {
doTestChunkSize(false, false, SimpleHttpClient.CRLF, 10, 0);
}

@Test
public void testChunkSizeTwentyFive() throws Exception {
doTestChunkSize(true, true, "19" + SimpleHttpClient.CRLF
+ "Hello World!Hello World!!" + SimpleHttpClient.CRLF, 40, 25);
}

@Test
public void testChunkSizeEightDigit() throws Exception {
doTestChunkSize(true, true, "0000000C" + SimpleHttpClient.CRLF
+ "Hello World!" + SimpleHttpClient.CRLF, 20, 12);
}

@Test
public void testChunkSizeNineDigit() throws Exception {
doTestChunkSize(false, false, "00000000C" + SimpleHttpClient.CRLF
+ "Hello World!" + SimpleHttpClient.CRLF, 20, 12);
}

@Test
public void testChunkSizeLong() throws Exception {
doTestChunkSize(true, false, "7fFFffFF" + SimpleHttpClient.CRLF
+ "Hello World!" + SimpleHttpClient.CRLF, 10, 10);
}

@Test
public void testChunkSizeIntegerMinValue() throws Exception {
doTestChunkSize(false, false, "80000000" + SimpleHttpClient.CRLF
+ "Hello World!" + SimpleHttpClient.CRLF, 10, 10);
}

@Test
public void testChunkSizeMinusOne() throws Exception {
doTestChunkSize(false, false, "ffffffff" + SimpleHttpClient.CRLF
+ "Hello World!" + SimpleHttpClient.CRLF, 10, 10);
}

/**
* @param expectPass
* If the servlet is expected to process the request
* @param expectReadWholeBody
* If the servlet is expected to fully read the body and reliably
* deliver a response
* @param chunks
* Text of chunks
* @param readLimit
* Do not read more than this many bytes
* @param expectReadCount
* Expected count of read bytes
* @throws Exception
* Unexpected
*/
private void doTestChunkSize(boolean expectPass,
boolean expectReadWholeBody, String chunks, int readLimit,
int expectReadCount) throws Exception {
// Setup Tomcat instance
Tomcat tomcat = getTomcatInstance();

// Must have a real docBase - just use temp
Context ctx = tomcat.addContext("",
System.getProperty("java.io.tmpdir"));

BodyReadServlet servlet = new BodyReadServlet(expectPass, readLimit);
Tomcat.addServlet(ctx, "servlet", servlet);
ctx.addServletMapping("/", "servlet");

tomcat.start();

String request = "POST /echo-params.jsp HTTP/1.1"
+ SimpleHttpClient.CRLF + "Host: any" + SimpleHttpClient.CRLF
+ "Transfer-encoding: chunked" + SimpleHttpClient.CRLF
+ "Content-Type: text/plain" + SimpleHttpClient.CRLF;
if (expectPass) {
request += "Connection: close" + SimpleHttpClient.CRLF;
}
request += SimpleHttpClient.CRLF + chunks + "0" + SimpleHttpClient.CRLF
+ SimpleHttpClient.CRLF;

TrailerClient client = new TrailerClient(tomcat.getConnector()
.getLocalPort());
client.setRequest(new String[] { request });

Exception processException = null;
client.connect();
try {
client.processRequest();
} catch (Exception e) {
// Socket was probably closed before client had a chance to read
// response
processException = e;
}
if (expectPass) {
if (expectReadWholeBody) {
assertNull(processException);
}
if (processException == null) {
assertTrue(client.getResponseLine(), client.isResponse200());
assertEquals(String.valueOf(expectReadCount),
client.getResponseBody());
}
assertEquals(expectReadCount, servlet.getCountRead());
} else {
if (processException == null) {
assertTrue(client.getResponseLine(), client.isResponse500());
}
assertEquals(0, servlet.getCountRead());
assertTrue(servlet.getExceptionDuringRead());
}
}

private static class EchoHeaderServlet extends HttpServlet {
private static final long serialVersionUID = 1L;

private boolean exceptionDuringRead = false;

private final boolean expectPass;

public EchoHeaderServlet(boolean expectPass) {
this.expectPass = expectPass;
}

@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
Expand All @@ -334,6 +459,11 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp)
}
} catch (IOException ioe) {
exceptionDuringRead = true;
if (!expectPass) { // as expected
log(ioe.toString());
resp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
return;
}
throw ioe;
}

Expand All @@ -358,6 +488,53 @@ private void dumpHeader(String headerName, HttpServletRequest req,
}
}

private static class BodyReadServlet extends HttpServlet {
private static final long serialVersionUID = 1L;

private boolean exceptionDuringRead = false;
private int countRead = 0;
private final boolean expectPass;
private final int readLimit;

public BodyReadServlet(boolean expectPass, int readLimit) {
this.expectPass = expectPass;
this.readLimit = readLimit;
}

@Override
protected void doPost(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException {
resp.setContentType("text/plain");
PrintWriter pw = resp.getWriter();

// Read the body - quick and dirty
InputStream is = req.getInputStream();
try {
while (is.read() > -1 && countRead < readLimit) {
countRead++;
}
} catch (IOException ioe) {
exceptionDuringRead = true;
if (!expectPass) { // as expected
log(ioe.toString());
resp.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
return;
}
throw ioe;
}

pw.write(Integer.valueOf(countRead).toString());
}

public boolean getExceptionDuringRead() {
return exceptionDuringRead;
}

public int getCountRead() {
return countRead;
}
}

private static class TrailerClient extends SimpleHttpClient {

public TrailerClient(int port) {
Expand Down
5 changes: 5 additions & 0 deletions webapps/docs/changelog.xml
View file
Expand Up @@ -149,6 +149,11 @@
Add experimental NIO2 connector. Based on code developed by
Nabil Benothman. (remm)
</add>
<fix>
Improve processing of chuck size from chunked headers. Avoid overflow
and use a bit shift instead of a multiplication as it is marginally
faster. (markt/kkolinko)
</fix>
</changelog>
</subsection>
<subsection name="Jasper">
Expand Down

0 comments on commit d49a037

Please sign in to comment.