Skip to content

Commit

Permalink
Stricter header value parsing
Browse files Browse the repository at this point in the history
  • Loading branch information
@markt-asf
markt-asf committed Feb 4, 2020
1 parent 959f1df commit 8fbe2e9
Show file tree
Hide file tree
Showing 8 changed files with 163 additions and 42 deletions.
51 changes: 40 additions & 11 deletions java/org/apache/coyote/http11/AbstractHttp11Protocol.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,27 +145,56 @@ public void setAllowHostHeaderMismatch(boolean allowHostHeaderMismatch) {
}


private boolean rejectIllegalHeaderName = false;
private boolean rejectIllegalHeader = false;
/**
* 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;
}


Expand Down
51 changes: 37 additions & 14 deletions java/org/apache/coyote/http11/Http11InputBuffer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
private final MimeHeaders headers;


private final boolean rejectIllegalHeaderName;
private final boolean rejectIllegalHeader;

/**
* State.
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -760,6 +760,8 @@ private HeaderParseStatus parseHeader() throws IOException {
//

byte chr = 0;
byte prevChr = 0;

while (headerParsePos == HeaderParsePosition.HEADER_START) {

// Read new bytes if needed
Expand All @@ -770,17 +772,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) {
Expand Down Expand Up @@ -877,11 +885,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++;
Expand Down Expand Up @@ -933,6 +952,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) {

Expand All @@ -944,20 +966,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);
Expand Down
2 changes: 1 addition & 1 deletion java/org/apache/coyote/http11/Http11Processor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public Http11Processor(AbstractHttp11Protocol<?> protocol, AbstractEndpoint<?> e
protocol.getRelaxedQueryChars());

inputBuffer = new Http11InputBuffer(request, protocol.getMaxHttpHeaderSize(),
protocol.getRejectIllegalHeaderName(), httpParser);
protocol.getRejectIllegalHeader(), httpParser);
request.setInputBuffer(inputBuffer);

outputBuffer = new Http11OutputBuffer(response, protocol.getMaxHttpHeaderSize(),
Expand Down
2 changes: 1 addition & 1 deletion java/org/apache/tomcat/util/http/MimeHeaders.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
11 changes: 11 additions & 0 deletions java/org/apache/tomcat/util/http/parser/HttpParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,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.
Expand Down
72 changes: 60 additions & 12 deletions test/org/apache/coyote/http11/TestHttp11InputBuffer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}


Expand Down Expand Up @@ -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");

Expand All @@ -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() {
Expand All @@ -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());

Expand Down Expand Up @@ -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());

Expand Down
5 changes: 5 additions & 0 deletions webapps/docs/changelog.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,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">
Expand Down
Loading

0 comments on commit 8fbe2e9

Please sign in to comment.