Bug 60716

Summary: SSL certificate CRLDP section is ignored, when NIO connection is used
Product: Tomcat 8 Reporter: Kirill <kegorof>
Component: ConnectorsAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: major    
Priority: P2    
Version: 8.5.11   
Target Milestone: ----   
Hardware: PC   
OS: Linux   

Description Kirill 2017-02-09 11:51:07 UTC
Environment:
Java(TM) SE Runtime Environment (build 1.8.0_66-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.66-b17, mixed mode)

Java started with following additional parameters:
-Dcom.sun.net.ssl.checkRevocation=true -Dcom.sun.security.enableCRLDP=true

Connector configuration:
<Connector port="8443"
           protocol="org.apache.coyote.http11.Http11Nio2Protocol"
           SSLEnabled="true"
           maxThreads="150"
           scheme="https"
           secure="true">
    <SSLHostConfig certificateVerification="optional"
                   protocols="TLSv1.2"
                   truststoreFile="${catalina.home}/conf/.truststore"
                   truststorePassword="password">
        <Certificate certificateKeystoreFile="${catalina.base}/conf/.keystore"
                     certificateKeystorePassword="password"
                     certificateKeyAlias="tomcat"
                     certificateKeyPassword="password" />
    </SSLHostConfig>
</Connector>

How to reproduce:
1. Root CA is imported into truststore. Client has valid certificate signed by the root CA, this certificate has CRLDistributionPoint section. Client has access to the application using https protocol.
2. Then client's certificate is revoked. Wait until certificate revocation list is updated, check that CRL contains client's certificate serial number. 
3. User has access to the application! 
4. Check communication using tcpdump, no connections to CRLDP host. 
5. Try jvm paramter -Djava.security.debug=certpath -- no CRL checks.

This problem can't be reproduced in tomcat 8.0.x and 7.x. I think the problem is in the org.apache.tomcat.util.net.jsse.JSSEUtil#getParameters method. If SSLHostConfig doesn't contain certificateRevocationListFile parameter, then xparams.setRevocationEnabled(false) invoked (http://svn.apache.org/viewvc/tomcat/tc8.5.x/trunk/java/org/apache/tomcat/util/net/jsse/JSSEUtil.java?diff_format=h&revision=1780997&view=markup#l341). 
So without crlFile, jvm parameter -Dcom.sun.net.ssl.checkRevocation is ignored. But if crlFile is set, then certificate's CRLDP section is ignored. 
Anyway, crlFile is read only once and all updates are ignored, thus it can't be used as workaround.
Comment 1 Remy Maucherat 2017-02-09 17:53:35 UTC
Ok, I see r1757578 added an explicit setRevocationEnabled(false). I think this may need to be improved.

But overall I think this won't be reverted, as the use and continued support of proprietary system properties like that is never good nor guaranteed. Especially since it's JVM wide while we now need to have per vhost configuration, in case you haven't noticed.
Comment 2 Kirill 2017-02-09 19:24:27 UTC
(In reply to Remy Maucherat from comment #1)
> Ok, I see r1757578 added an explicit setRevocationEnabled(false). I think
> this may need to be improved.
> 
> But overall I think this won't be reverted, as the use and continued support
> of proprietary system properties like that is never good nor guaranteed.
> Especially since it's JVM wide while we now need to have per vhost
> configuration, in case you haven't noticed.

I agree, that these properties are proprietary. But without such properties I can't check certificate revocation using CRLDP. Also OCSP check doesn't work too...
And there is no backward compatibility with tomcat 7.x and 8.0.x.

Of cause, it's possible to implement my own TrustManager and check CRL and OCSP there. But this way is quite complicated. 
Why you can't remove setRevocationEnabled(false) in the else section or parameterize it? Without it default jvm configuration will be used, so user can enable/disable revocation check using jvm properties.
Comment 3 Mark Thomas 2017-02-14 12:37:28 UTC
Another example of why configuration via system property is just wrong. Sigh.

The call to setRevocationEnabled(false) is necessary when no revocation is configured since the default is true. Without it, all certs fail validation.

I think the simplest solution is a new JSSE property on SSLHostConfig - revocationEnabled. It will be ignored / assumed to be true if certificateRevocationListFile is set. Default will be false (current behaviour). If you need to configure revocation via proprietary methods for your JSSE provider then you can do so and set the new attribute to true.

I should have a patch for this shortly.
Comment 4 Mark Thomas 2017-02-14 12:51:50 UTC
Fixed in:
- trunk for 9.0.0.M18 onwards
- 8.5.x for 8.5.12 onwards
Comment 5 Kirill 2017-02-14 12:56:33 UTC
(In reply to Mark Thomas from comment #3)
> Another example of why configuration via system property is just wrong. Sigh.
> 
> The call to setRevocationEnabled(false) is necessary when no revocation is
> configured since the default is true. Without it, all certs fail validation.
> 
> I think the simplest solution is a new JSSE property on SSLHostConfig -
> revocationEnabled. It will be ignored / assumed to be true if
> certificateRevocationListFile is set. Default will be false (current
> behaviour). If you need to configure revocation via proprietary methods for
> your JSSE provider then you can do so and set the new attribute to true.
> 
> I should have a patch for this shortly.

Agree with you. Thank you.
Comment 6 Christopher Schultz 2017-02-14 18:21:40 UTC
(In reply to Mark Thomas from comment #3)
> I think the simplest solution is a new JSSE property on SSLHostConfig -
> revocationEnabled.

That's a confusing configuration attribute: it implies that revocation is actually happening instead of just being consulted. Can we get a better name for that?
Comment 7 Mark Thomas 2017-02-15 09:24:29 UTC
(In reply to Christopher Schultz from comment #6)
> That's a confusing configuration attribute: it implies that revocation is
> actually happening instead of just being consulted. Can we get a better name
> for that?

I re-used the JVM's internal attribute name. My thinking was if it was ever exposed, having the same name would be helpful.

I see your point that the name could be better. I don't have any better suggestions though. If you can think of one before the next 9.0.x tag then feel free to change it.