Bug 48158

Summary: warn that "per directory client certificate authentication" is harmful
Product: Tomcat 5 Reporter: Ralf Hauser <hauser>
Component: Connector:CoyoteAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED DUPLICATE    
Severity: critical CC: moreira
Priority: P2    
Version: Unknown   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: patch_48158_c5_wildCard.txt

Description Ralf Hauser 2009-11-07 08:09:26 UTC
in http://tomcat.apache.org/tomcat-5.5-doc/ssl-howto.html#Miscellaneous%20Tips%20and%20Bits

1) put a warning that not using clientAuth=false and using 
   <url-pattern>/something</url-pattern> is harmful
2) describe how to turn off TLS re-negotiation

see also  Bug 48157 and  Bug 34643 and http://forums.sun.com/thread.jspa?messageID=10857837
Comment 1 Mark Thomas 2009-11-07 09:10:34 UTC
Agreed the warning needs to go out although the docs aren't the best place for it. Please join the discussion on the dev list.
Comment 2 Ralf Hauser 2009-11-09 04:07:54 UTC
tomcat-dev-list:> BIO/NIO connectors using JSSE.
> Vulnerable when renegotiation is triggered by the client or server.
> We could prevent server initiated renegotiation (and probably break 
> the majority of configurations using CLIENT-CERT).
Couldn't you make this an optional server.xml attribute where each site can decide whether to use it or not (i.e. test themselves whether it affects them or not). We are quite advanced on migrating our site away from sub-directory/url-pattern based renegotiation. So, having Coyote not allowing for re-negotiation would be a great benefit for us and we obviously would report on difficulties we and our users encounter to optimize this approach!
> We can't do anything to prevent client initiated renegotiation.
Sure, but closing 2 out of 3 attack vectors is at least something, isn't it?
Comment 3 Mark Thomas 2009-11-09 06:15:15 UTC
(In reply to comment #2)
> Couldn't you make this an optional server.xml attribute
See the clientAuth connector attribute for options already available for limiting server side re-negotiation.

> > We can't do anything to prevent client initiated renegotiation.
> Sure, but closing 2 out of 3 attack vectors is at least something, isn't it?
In this case, I don't think it is. However, the options are already in place if you wish to use them.
Comment 4 Ralf Hauser 2009-11-12 08:59:24 UTC
see also http://marc.info/?t=125761336000001&r=1&w=2
Comment 5 Ralf Hauser 2009-11-12 09:52:53 UTC
(In reply to comment #3)
> > Couldn't you make this an optional server.xml attribute
> See the "clientAuth" connector attribute for options already available for
> limiting server side re-negotiation.
Hmm, the word "re-negotiation" doesn't really appear in http://tomcat.apache.org/tomcat-5.5-doc/ssl-howto.html#Edit%20the%20Tomcat%20Configuration%20File for the attribute nor does one know whether it is a mandatory or optional attribute (same for "truststoreFile" - is there a "*" wildcard option to accept any issuer?).

> > > We can't do anything to prevent client initiated renegotiation.
> > Sure, but closing 2 out of 3 attack vectors is at least something, isn't it?
> In this case, I don't think it is. However, the options are already in place 
> if you wish to use them.
looking at org.apache.tomcat.util.net.jsse.JSSESupport.getPeerCertificateChain(boolean force), I see 

if(jsseCerts.length <= 0 && force) {
     session.invalidate();
     handShake();
     session = ssl.getSession();
 }

isn't that the renegotiation we want to avoid?

Shouldn't this now be commented out for the time being?
(One scenario being that an MITM attacker in the handshake eliminates the optional "CertificateRequest*" as per section "7.3. Handshake Protocol overview
" http://www.ietf.org/rfc/rfc2246.txt
With that the client might first do a non-client-cert SSL session and the server might soon notice and trigger a TLS-re-handshake [just speculating...])

At least the javax.net.ssl.SSLSocket.startHandshake() executed inside the above "handShake()" doesn't dissipate my suspicions that there may be a problem:

/**
     * Starts an SSL handshake on this connection.
     */
    public void startHandshake() throws IOException {
	checkWrite();
	try {
	    if (getConnectionState() == cs_HANDSHAKE) {
		// do initial handshake
		performInitialHandshake();
	    } else {
		// start renegotiation
		kickstartHandshake();
	    }
	} catch (Exception e) {
	    // shutdown and rethrow (wrapped) exception as appropriate
	    handleException(e);
	}
    }

but maybe http://marc.info/?l=tomcat-dev&m=125796482429041&w=2 got there too...
Comment 6 Ralf Hauser 2009-11-13 01:40:04 UTC
see also Bug 48192
Comment 7 Luciana Moreira 2009-11-16 06:25:22 UTC
Created attachment 24542 [details]
patch_48158_c5_wildCard.txt

I have come up with a patch to allow accepting any client certificate on a per Connector basis.

In server.xml the following attribute should be added in the Connector element:

<Connector ... acceptAllCerts="true"/>

If this argument is present and set to true or yes, then the AcceptAllTrustManager will be used as Trust Manager.
Comment 8 Mark Thomas 2009-11-16 06:37:25 UTC
(In reply to comment #7)
> I have come up with a patch to allow accepting any client certificate on a per
> Connector basis.

Please don't hijack an unrelated bug report. You should create a new issue (mark it as an enhancement request) and attach your patch to the new issue.
Comment 9 Ralf Hauser 2009-11-17 00:47:30 UTC
Comment on attachment 24542 [details]
patch_48158_c5_wildCard.txt

new is attachment (id=24546) in bug 48208

(I had raised her "wildcard" argument in comment 5, so I don't feel 'hijacked', but I agree with Mark that it might benefit from being an issue on its own)
Comment 10 Mark Thomas 2009-11-19 13:38:59 UTC
The warning has already gone out to users@, dev@ and announce@

The actual patch is currently being tracked in bug48236. That eventual patch for bug48236 will be ported to 5.5.x and 6.0.x.

*** This bug has been marked as a duplicate of bug 48236 ***