SSL/TLS certificate verification disabled

[harbulot]

The SSL/TLS certificate verification is disabled. This is a security issue (which makes connections vulnerable to MITM attacks).

Here is a quick test that can be tested with a server that has a self-signed certificate or issued by a CA that's not in the default truststore. URLConnection will fail as expected (sun.security.validator.ValidatorException: PKIX path building failed), whereas SimpleAsyncHttpClient will work just fine.

String hostname = "https://somehost/";

try {
    URLConnection connection = new URL(hostname).openConnection();
    connection.connect();
} catch (Exception e) {
    e.printStackTrace();
}

SimpleAsyncHttpClient client = new SimpleAsyncHttpClient.Builder()
        .setUrl(hostname).build();
client.get(new BodyConsumer() {
    @Override
    public void consume(ByteBuffer buffer) throws IOException {
        System.out.print(new String(buffer.array(), "UTF-8"));
    }

    @Override
    public void close() throws IOException {
    }
});

Please remove anything that has to do with the TrustingSSLSocketFactory in com.ning.http.client.providers.apache.TrustingSSLSocketFactory.

[slandelle]

There are several related issues: #146 and #197.

Here are some interesting resources:

• http://kevinlocke.name/bits/2012/10/03/ssl-certificate-verification-in-dispatch-and-asynchttpclient/

• http://stackoverflow.com/questions/8839541/hostname-in-certificate-didnt-match

• http://stackoverflow.com/questions/13315623/netty-ssl-hostname-verification-support

• http://stackoverflow.com/questions/13390964/java-ssl-fatal-error-80-unwrapping-net-record-after-adding-the-https-en

  I don't think we can change/fix this before AHC v2, as it would change behavior (AHC would stop working with self-signed certificates).

[harbulot]

The fact that AHC works out of the box with self-signed certificates isn't a feature, it's a security bug.

People who want their self-signed certificates to work should add them to their trust store (either programmatically, which I'm not sure is possible with AHC, or via -Djavax.net.ssl.truststore=..., which will affect the default SSLContext for that application).

There's a discussion on this problem here: http://security.stackexchange.com/questions/22965/what-is-the-potential-impact-of-these-ssl-certificate-validation-vulnerabilities

Many developers using libraries like this assume that it has sensible security defaults. Unfortunately, it seems that AHC doesn't. This impacts a number of projects that uses this library. (To be honest, I don't use AHC myself, but I have noticed that an application I've used was able to connect to an URL that uses a certificate that wouldn't normally have been validated with my trust store.)

[harbulot]

I've just had a quick look at the fragment of code affected by the patch in issue #351.

I think the expected default settings don't quite make sense.

SSLConfig config = new SSLConfig();
if (config.keyStoreLocation == null
    || config.trustStoreLocation == null) {
     context = getLooseSSLContext();
} else {
     context = getStrictSSLContext(config);
}

Firstly, not specifying a keystore location is actually quite common, since, from a client point of view, the keystore is only used when using client certificates. There isn't even a default value for the keystore in the default JSSE settings (see JSSE Customization section). Assuming that when no client certificates are used, no server certificate validation should take places is a bid odd.

Secondly, of course, assuming that, when no truststore location is specified, no server certificate validation should take place either (not even with the default JRE truststore/trustmanager) is certainly a default that will catch many library users by surprise. In addition, it's not always possible to find what the default trust manager uses (see: http://stackoverflow.com/questions/13657299/how-to-find-current-truststore-on-disk-programatically/13660336#13660336).

[slandelle]

Are you willing to contribute? I personally won't be able to work on this before several weeks at least.

[vmalladi]

Hi AHC team,
@rlubke @jfarcand @slandelle @oleksiys
Can you comment if this security issue is with all AHC providers or specific to say default provider (netty/grizzly/JDK)?

Thanks.

[wsargent]

I think the logic behind SSLUtils in creating only a single static SSLContext is incorrect, because it means that the system properties that were in effect at the time of creation are static. In reality, system properties can be changed after a program runs.

In addition, getStrictSSLContext replicates exactly the logic that would be executed by the default JSSE SSLContextImpl in any case -- if you called sslContext.init(null, null, null) then it would look like the strict version. As such, it's repeating the functionality without any new benefit.

Not only that, but SSLUtil relies on static methods for functionality. Even if the goal were to use a singleton, there's no way to reinitialize the method or override a static method without using reflection or Unsafe.

Finally, getLooseSSLContext should not exist, at all. If people need to pass in a loose SSL context, they should do that by hand.

[slandelle]

I mostly agree, except regarding getLooseSSLContext that can perfectly sit there as a public helper. It's the one I'd use for Gatling, I don't really care about security there, I need raw performance.

Do you think you could handle this? My hands are pretty full at the moment...

[wsargent]

Poking at it right now.

[wsargent]

@slandelle if you're using getLooseSSLContext for Gatling, then you're probably getting inaccurate numbers for your load tests, as adding / removing certificate verification is going to alter the CPU / memory times. I think it's much better to remove it -- the real pain isn't in TLS certificate verification, it's that creating certificates and sharing them is not something people know how to do.

JEP 166 should let users set up multiple keystores like this:
http://cr.openjdk.java.net/~vinnie/8007755/webrev.00/raw_files/new/test/sun/security/provider/KeyStore/domains.cfg

[wsargent]

@slandelle PR submitted.