Bug 54010 - Suggestion for code improvement (avoiding potential bug)
Suggestion for code improvement (avoiding potential bug)
Status: RESOLVED FIXED
Product: Tomcat 8
Classification: Unclassified
Component: Connectors
8.0.x-trunk
PC Linux
: P2 enhancement (vote)
: ----
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2012-10-16 02:20 UTC by Dongcai Shen
Modified: 2012-10-22 22:54 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dongcai Shen 2012-10-16 02:20:47 UTC
In connectors/jk/java/org/apache/jk/common/HandlerRequest.java

coyote.Request's schemeMB is assigned in 2 places.

1st place: 
400         boolean isSSL = msg.getByte() != 0;
401         if( isSSL ) {
402             // XXX req.setSecure( true );
403             req.scheme().setString("https");
404         }

2nd place:
518             case AjpConstants.SC_A_SSL_CERT     :
519                 req.scheme().setString( "https" );
and similar assignments for SC_A_SSL_CIPHER and SC_A_SSL_SESSION cases below.

It seems they do not make sense because the packet's 8-bit field is designated for telling whether it's SSL or not. So the 1st place is enough. Adding the 2nd place may pose potential bug in that a packet with the 8-bit SSL field being 0 and suffixes of SC_A_SSL_* key-value pairs can later incorrect trigger a wrong redirection message pointing to a https location.

A simple correction is to honor the 8-bit SSL-field in packet and delete the 3 lines of 2nd place assigning "https". 

Even though the chances of such spurious packet is low, but it's best we can have threat-free, semantic-correct tomcat code.

The same lines of code remain in 6.0 and 7.0.

But maybe I misunderstand the code, in which case please kindly point out. Thanks.
Comment 1 Mark Thomas 2012-10-22 22:40:27 UTC
Tomcat 5.5.x is no longer supported.

Looking at the mod_jk, I don't see how there could ever be a problem here but there is scope for removing the additional unnecessary calls.

Moving this to Tomcat 8.0.x and marking as an enhancement.

This will most likely get be applied to 8.0.x only, as part of the ongoing clean-up of the code base for the 8.0.x branch.
Comment 2 Mark Thomas 2012-10-22 22:49:15 UTC
Fixed in 8.0.x and will be included in 8.0.0 onwards.
Comment 3 Dongcai Shen 2012-10-22 22:54:43 UTC
Thank Mark for your efforts.

The credit of this vulnerability should be given to this bug report: 
https://issues.apache.org/bugzilla/show_bug.cgi?id=36883

They discovered the bug but only fixed in the httpd site and left the problem in tomcat intact. Now the problem at the tomcat part is also fixed.