Follow-up changes to Android certificate verification logic.

- Handle devices without an "AndroidCAStore" KeyStore. Add a histogram to
  record when this happens.
- Remove resolved TODO.
- Unnecessary exception check (relevant function already CalledByNative, not
  CalledByNativeUnchecked)

BUG=116838
TEST=none

Review URL: https://codereview.chromium.org/144153002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@248587 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 50 insertions, 24 deletions

diff --git a/net/android/java/src/org/chromium/net/X509Util.java b/net/android/java/src/org/chromium/net/X509Util.java
index 788f226..97eaa9d 100644
--- a/net/android/java/src/org/chromium/net/X509Util.java
+++ b/net/android/java/src/org/chromium/net/X509Util.java
@@ -152,16 +152,23 @@ public class X509Util {
     private static Set<Pair<X500Principal, PublicKey>> sSystemTrustRoots;
 
     /**
+     * True if the system trust roots were initialized. (sSystemTrustRoots may
+     * still be null if system trust roots cannot be distinguished from
+     * user-installed ones.)
+     */
+    private static boolean sLoadedSystemTrustRoots;
+
+    /**
      * Lock object used to synchronize all calls that modify or depend on the trust managers.
      */
     private static final Object sLock = new Object();
 
-    /*
-     * Allow disabling registering the observer for the certificat 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.
+    /**
+     * Allow disabling registering the observer and recording histograms 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.
      */
-    private static boolean sDisableCertificateObservationForTest = false;
+    private static boolean sDisableNativeCodeForTest = false;
 
     /**
      * Ensures that the trust managers and certificate factory are initialized.
@@ -175,8 +182,18 @@ public class X509Util {
             if (sDefaultTrustManager == null) {
                 sDefaultTrustManager = X509Util.createTrustManager(null);
             }
-            if (sSystemTrustRoots == null) {
-                sSystemTrustRoots = buildSystemTrustRootSet();
+            if (!sLoadedSystemTrustRoots) {
+                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);
+                }
+                if (!sDisableNativeCodeForTest)
+                    nativeRecordCertVerifyCapabilitiesHistogram(sSystemTrustRoots != null);
+                sLoadedSystemTrustRoots = true;
             }
             if (sTestKeyStore == null) {
                 sTestKeyStore = KeyStore.getInstance(KeyStore.getDefaultType());
@@ -189,8 +206,7 @@ public class X509Util {
             if (sTestTrustManager == null) {
                 sTestTrustManager = X509Util.createTrustManager(sTestKeyStore);
             }
-            if (!sDisableCertificateObservationForTest &&
-                    sTrustStorageListener == null) {
+            if (!sDisableNativeCodeForTest && sTrustStorageListener == null) {
                 sTrustStorageListener = new TrustStorageListener();
                 nativeGetApplicationContext().registerReceiver(sTrustStorageListener,
                         new IntentFilter(KeyChain.ACTION_STORAGE_CHANGED));
@@ -240,7 +256,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);
@@ -268,6 +284,7 @@ public class X509Util {
             NoSuchAlgorithmException, CertificateException {
         sDefaultTrustManager = null;
         sSystemTrustRoots = null;
+        sLoadedSystemTrustRoots = false;
         nativeNotifyKeyChainChanged();
         ensureInitialized();
     }
@@ -404,7 +421,7 @@ 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(),
@@ -416,8 +433,8 @@ public class X509Util {
         }
     }
 
-    public static void setDisableCertificateObservationForTest(boolean disabled) {
-        sDisableCertificateObservationForTest = disabled;
+    public static void setDisableNativeCodeForTest(boolean disabled) {
+        sDisableNativeCodeForTest = disabled;
     }
     /**
      * Notify the native net::CertDatabase instance that the system database has been updated.
@@ -425,6 +442,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();
diff --git a/net/android/javatests/src/org/chromium/net/X509UtilTest.java b/net/android/javatests/src/org/chromium/net/X509UtilTest.java
index 6e27b1a..612f886 100644
--- a/net/android/javatests/src/org/chromium/net/X509UtilTest.java
+++ b/net/android/javatests/src/org/chromium/net/X509UtilTest.java
@@ -65,7 +65,7 @@ public class X509UtilTest extends InstrumentationTestCase {
 
     @Override
     public void setUp() {
-        X509Util.setDisableCertificateObservationForTest(true);
+        X509Util.setDisableNativeCodeForTest(true);
     }
 
     @MediumTest
diff --git a/net/android/network_library.cc b/net/android/network_library.cc
index 37ab18e..ea9ec4a4 100644
--- a/net/android/network_library.cc
+++ b/net/android/network_library.cc
@@ -12,7 +12,6 @@
 #include "jni/AndroidNetworkLibrary_jni.h"
 
 using base::android::AttachCurrentThread;
-using base::android::ClearException;
 using base::android::ConvertJavaStringToUTF8;
 using base::android::ConvertUTF8ToJavaString;
 using base::android::GetApplicationContext;
@@ -46,10 +45,6 @@ void VerifyX509CertChain(const std::vector<std::string>& cert_chain,
   ScopedJavaLocalRef<jobject> result =
       Java_AndroidNetworkLibrary_verifyServerCertificates(
           env, chain_byte_array.obj(), auth_string.obj(), host_string.obj());
-  if (ClearException(env)) {
-    *status = android::VERIFY_FAILED;
-    return;
-  }
 
   ExtractCertVerifyResult(result.obj(),
                           status, is_issued_by_known_root, verified_chain);
diff --git a/net/cert/cert_verify_proc_android.cc b/net/cert/cert_verify_proc_android.cc
index 590bb79..bd74726 100644
--- a/net/cert/cert_verify_proc_android.cc
+++ b/net/cert/cert_verify_proc_android.cc
@@ -144,11 +144,6 @@ int CertVerifyProcAndroid::VerifyInternal(
   if (IsCertStatusError(verify_result->cert_status))
     return MapCertStatusToNetError(verify_result->cert_status);
 
-  // TODO(ppi): Implement missing functionality: yielding the constructed trust
-  // chain, public key hashes of its certificates and |is_issued_by_known_root|
-  // flag. All of the above require specific support from the platform, missing
-  // in the Java APIs. See also: http://crbug.com/116838
-
   return OK;
 }
 
diff --git a/net/cert/x509_util_android.cc b/net/cert/x509_util_android.cc
index 1f6c3c6..128ea85 100644
--- a/net/cert/x509_util_android.cc
+++ b/net/cert/x509_util_android.cc
@@ -4,7 +4,9 @@
 
 #include "net/cert/x509_util_android.h"
 
+#include "base/android/build_info.h"
 #include "base/android/jni_android.h"
+#include "base/metrics/histogram.h"
 #include "jni/X509Util_jni.h"
 #include "net/cert/cert_database.h"
 
@@ -14,6 +16,17 @@ void NotifyKeyChainChanged(JNIEnv* env, jclass clazz) {
   CertDatabase::GetInstance()->OnAndroidKeyChainChanged();
 }
 
+void RecordCertVerifyCapabilitiesHistogram(JNIEnv* env,
+                                           jclass clazz,
+                                           jboolean found_system_trust_roots) {
+  // Only record the histogram for 4.2 and up. Before 4.2, the platform doesn't
+  // return the certificate chain anyway.
+  if (base::android::BuildInfo::GetInstance()->sdk_int() >= 17) {
+    UMA_HISTOGRAM_BOOLEAN("Net.FoundSystemTrustRootsAndroid",
+                          found_system_trust_roots);
+  }
+}
+
 jobject GetApplicationContext(JNIEnv* env, jclass clazz) {
   return base::android::GetApplicationContext();
 }