Follow-up changes to Android certificate verification logic.

net/android/java/src/org/chromium/net/X509Util.java

Index: net/android/java/src/org/chromium/net/X509Util.java
diff --git a/net/android/java/src/org/chromium/net/X509Util.java b/net/android/java/src/org/chromium/net/X509Util.java
index 788f226b922a54e5f41eda91e6cbe9ec1aa8191a..18ab61333d56802f4151c0d1165a9125b15f27cd 100644
--- a/net/android/java/src/org/chromium/net/X509Util.java
+++ b/net/android/java/src/org/chromium/net/X509Util.java
@@ -157,7 +157,7 @@ public class X509Util {
     private static final Object sLock = new Object();
 
     /*
-     * Allow disabling registering the observer for the certificat changes. Net unit tests do not
+     * Allow disabling registering the observer for the certificate changes. Net unit tests do not
      * load native libraries which prevent this to succeed. Moreover, the system does not allow to
      * interact with the certificate store without user interaction.
      */
@@ -176,7 +176,14 @@ public class X509Util {
                 sDefaultTrustManager = X509Util.createTrustManager(null);
             }
             if (sSystemTrustRoots == null) {
-                sSystemTrustRoots = buildSystemTrustRootSet();
+                try {
+                    sSystemTrustRoots = buildSystemTrustRootSet();
+                } catch (KeyStoreException e) {
+                    // If the device does not have an "AndroidCAStore" KeyStore, don't make the
+                    // failure fatal. Instead default conservatively to setting isIssuedByKnownRoot
+                    // to false everywhere.
+                    Log.w(TAG, "Could not load system trust root set", e);

[Yaron, 2014/01/21 20:57:14]

Just want to double check that this error doesn't include anything sensitive or
private.  The logs get output to the shared logcat which any app (with requested
permission) can read.

[davidben, 2014/01/22 17:51:15]

It shouldn't. On my device, if I change the string to one that isn't there, it
says:

W/X509Util( 1447): Could not load system trust root set
W/X509Util( 1447): java.security.KeyStoreException:
java.security.NoSuchAlgorithmException: KeyStore AndroidCAStorez implementation
not found
W/X509Util( 1447): 	at java.security.KeyStore.getInstance(KeyStore.java:119)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.buildSystemTrustRootSet(X509Util.java:211)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.ensureInitialized(X509Util.java:180)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.createCertificateFromBytes(X509Util.java:287)
W/X509Util( 1447): 	at
org.chromium.net.X509Util.verifyServerCertificates(X509Util.java:370)
W/X509Util( 1447): 	at
org.chromium.net.AndroidNetworkLibrary.verifyServerCertificates(AndroidNetworkLibrary.java:214)
W/X509Util( 1447): 	at dalvik.system.NativeStart.run(Native Method)
W/X509Util( 1447): Caused by: java.security.NoSuchAlgorithmException: KeyStore
AndroidCAStorez implementation not found
W/X509Util( 1447): 	at
org.apache.harmony.security.fortress.Engine.notFound(Engine.java:177)
W/X509Util( 1447): 	at
org.apache.harmony.security.fortress.Engine.getInstance(Engine.java:151)
W/X509Util( 1447): 	at java.security.KeyStore.getInstance(KeyStore.java:116)
W/X509Util( 1447): 	... 6 more

so that's not especially interesting.

Though, because it's emitted at every certificate verify now, it does leak
whether the browser did an SSL handshake (but not with what site). Is that
considered a problem for logcat? I can suppress all but the first error. Or
perhaps add a boolean so, if it can't load sSystemTrustRoots, it doesn't try
again until reloadDefaultTrustManager is called. Possibly not a bad plan anyway.

+                }
             }
             if (sTestKeyStore == null) {
                 sTestKeyStore = KeyStore.getInstance(KeyStore.getDefaultType());
@@ -240,7 +247,7 @@ public class X509Util {
         for (TrustManager tm : tmf.getTrustManagers()) {
             if (tm instanceof X509TrustManager) {
                 try {
-                    if (Build.VERSION.SDK_INT >= 17) {
+                    if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN_MR1) {
                         return new X509TrustManagerJellyBean((X509TrustManager) tm);
                     } else {
                         return new X509TrustManagerIceCreamSandwich((X509TrustManager) tm);
@@ -404,13 +411,15 @@ public class X509Util {
             }
 
             boolean isIssuedByKnownRoot = false;
-            if (verifiedChain.size() > 0) {
+            if (sSystemTrustRoots != null && verifiedChain.size() > 0) {
                 X509Certificate root = verifiedChain.get(verifiedChain.size() - 1);
                 isIssuedByKnownRoot = sSystemTrustRoots.contains(
                         new Pair<X500Principal, PublicKey>(root.getSubjectX500Principal(),
                                                            root.getPublicKey()));
             }
 
+            nativeRecordCertVerifyCapabilitiesHistogram(sSystemTrustRoots != null);
+
             return new AndroidCertVerifyResult(CertVerifyStatusAndroid.VERIFY_OK,
                                                isIssuedByKnownRoot, verifiedChain);
         }
@@ -425,6 +434,12 @@ public class X509Util {
     private static native void nativeNotifyKeyChainChanged();
 
     /**
+     * Record histograms on the platform's certificate verification capabilities.
+     */
+    private static native void nativeRecordCertVerifyCapabilitiesHistogram(
+        boolean foundSystemTrustRoots);
+
+    /**
      * Returns the application context.
      */
     private static native Context nativeGetApplicationContext();