Bug 57509

Summary: maxHttpHeaderSize maybe case ArrayIndexOutOfBoundsException
Product: Tomcat 8 Reporter: yangkun <yklovejava>
Component: ConnectorsAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 8.0.18   
Target Milestone: ----   
Hardware: PC   
OS: All   

Description yangkun 2015-01-28 08:16:03 UTC
Modify the server.xml, add the maxHttpHeaderSize attribute:

<Connector port="8080" protocol="HTTP/1.1"
               connectionTimeout="20000"
               redirectPort="8443" maxHttpHeaderSize="24"/>


  I deliberately modify the maxHttpHeaderSize to a small value, then I make a normal request, Sure, the request header is more than 24 bytes.

  Then the server is report following error:

28-Jan-2015 16:08:01.870 SEVERE [http-nio-8080-exec-1] org.apache.coyote.http11.AbstractHttp11Processor.endRequest Error finishing response
 java.lang.ArrayIndexOutOfBoundsException: 24
        at org.apache.coyote.http11.AbstractOutputBuffer.sendStatus(AbstractOutputBuffer.java:445)
        at org.apache.coyote.http11.AbstractHttp11Processor.prepareResponse(AbstractHttp11Processor.java:1554)
        at org.apache.coyote.http11.AbstractHttp11Processor.action(AbstractHttp11Processor.java:739)
        at org.apache.coyote.Response.action(Response.java:179)
 ...

  Is not check the maxHttpHeaderSize? Normal circumstances, it's should response 413-request entity is too large. I think this is better and reasonable.
Comment 1 Remy Maucherat 2015-01-28 08:23:32 UTC
The exception corresponds to what is actually occurring, so it looks fine to me as is.
Comment 2 Christopher Schultz 2015-02-03 14:20:59 UTC
It might be nice to do bounds-checking (or not) and use an application exception instead of AAOOBE. Getting an AAOOBE usually indicates to me that there is a software flaw, not a data flaw.
Comment 3 yangkun 2015-02-05 01:19:02 UTC
  If check the maxHttpHeaderSize careful, the error may be subdivide the following cases:

  1. the request uri more than maxHttpHeaderSize, it should be a 414-request url too long
  2. the request entity more than maxHttpHeaderSize, it should be a 413-request entity too large

  I think in this respect, Tomcat can be do better, :)
Comment 4 Konstantin Kolinko 2015-02-05 02:25:04 UTC
(In reply to yangkun from comment #3)

Your diagnosis is wrong. We are not talking about request here. It is response (the status line of a HTTP response) that does not fit the buffer used by status line and HTTP headers.

Generally, AbstractOutputBuffer.write(..) methods do perform a length check,
but numerous  headerBuffer[pos++] = (COLON|SP|CR|LF)  do not.

A simple fix may be to change AOB.checkLengthBeforeWrite(int length) method to assume that the usable buffer length is less by 4 bytes. There are no more than 4 bytes added directly to the buffer after the write.

(4 = 2 bytes for CR-LF + 2 bytes for CR-LF added by AOB.endHeaders())


The result will be that the checkLengthBeforeWrite() method will throw a org.apache.coyote.http11.HeadersTooLargeException  (a subclass of an IllegalStateException), essentially resulting in the same server-side error.
Comment 5 Konstantin Kolinko 2015-02-05 03:21:01 UTC
Fixed in trunk and Tomcat 8 (r1657460), will be in 8.0.19 onwards.
Comment 6 Konstantin Kolinko 2015-02-05 14:46:44 UTC
Fixed in Tomcat 7 by r1657591, will be in 7.0.60 onwards.
Comment 7 Shelson Ferrari 2015-02-10 20:06:19 UTC
 - 4 bytes added is the correct fixe ?


The method sendHeader -->

write(name);
headerBuffer[pos++] = Constants.COLON;
headerBuffer[pos++] = Constants.SP;
write(value);
headerBuffer[pos++] = Constants.CR;
headerBuffer[pos++] = Constants.LF;

call the method write(MessageBytes mb) 2 times and at the end of this method, call the method write(mb.getByteChunk()

this method call checkLengthBeforeWrite(length)

In the first call write(name) --> 
this block that increments 2 positions is not called
headerBuffer[pos++] = Constants.COLON;
headerBuffer[pos++] = Constants.SP;


The class AbstractHttp11Processor method private void prepareResponse()
call the class AOB -->
int size = headers.size();
for (int i = 0; i < size; i++) {
getOutputBuffer().sendHeader(headers.getName(i), headers.getValue(i));
}
getOutputBuffer().endHeaders();



in other words, I believe:


the method checkLengthBeforeWrite in the first call 
if (pos + length > headerBuffer.length)

the method checkLengthBeforeWrite in the second call 
if (pos + length + 2 > headerBuffer.length)

and if have a third scan after getOutputBuffer().endHeaders() - there is no such verification - and also would not make sense
if (pos + length + 4 > headerBuffer.length)

and would be after getOutputBuffer().endHeaders()


Please check.
Comment 8 Mark Thomas 2015-02-10 20:08:39 UTC
(In reply to Shelson Ferrari from comment #7)
>  - 4 bytes added is the correct fixe ?

Yes.