Add Digital Asset Links verification for postMessage API

chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java

Index: chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java
new file mode 100644
index 0000000000000000000000000000000000000000..ebcdd523d875458f66dc68b5bf1be8aeeae423cc
--- /dev/null
+++ b/chrome/android/java/src/org/chromium/chrome/browser/customtabs/OriginVerifier.java
@@ -0,0 +1,167 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.customtabs;
+
+import android.content.pm.PackageInfo;
+import android.content.pm.PackageManager;
+import android.net.Uri;
+import android.support.annotation.NonNull;
+
+import org.chromium.base.ContextUtils;
+import org.chromium.base.Log;
+import org.chromium.base.VisibleForTesting;
+import org.chromium.base.annotations.CalledByNative;
+import org.chromium.base.annotations.JNINamespace;
+import org.chromium.chrome.browser.IntentHandler;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.security.cert.CertificateEncodingException;
+import java.security.cert.CertificateException;
+import java.security.cert.CertificateFactory;
+import java.security.cert.X509Certificate;
+import java.util.HashMap;
+import java.util.Map;
+
+/**
+ * Used to verify postMessage origin for a designated package name.
+ *
+ * Uses Digital Asset Links to confirm that the given origin is associated with the package name as
+ * a postMessage origin. It caches any origin that has been verified during the current application
+ * lifecycle and reuses that without making any new network requests.
+ */
+@JNINamespace("customtabs")
+class OriginVerifier {

[nyquist, 2017/04/19 06:35:55]

What is the expected lifetime of this object? It deals with native, so when
should it be cleaned up by the owner? It looks like after it's created, it can
never be deleted, given that start(...) is the only public method? I guess in
some cases it might be deleted by native after a successful attempt, but since
that's not guaranteed, this feels a little bit iffy.

[Yusuf, 2017/04/26 00:51:35]

Made cleanup package protected and started calling it during session cleanup.
Added comments to reflect the current ownership.

+    private static final String TAG = "OriginVerifier";
+    private static final char[] HEX_CHAR_LOOKUP = "0123456789ABCDEF".toCharArray();
+    private static Map<String, Uri> sCachedOriginMap;
+    private long mNativeOriginVerifier = 0;

[nyquist, 2017/04/19 06:35:55]

Nit: Could you move this to after the final fields, to ensure it's easy to
figure out what can change and not over the lifetime of an instance of
OriginVerifier?

[Yusuf, 2017/04/26 00:51:35]

Done.

+    private final OriginVerificationListener mListener;
+    private final String mPackageName;
+    private final String mSignatureFingerprint;
+    private Uri mOrigin;
+
+    /**
+     * To be used for prepopulating verified origin for testing functionality.
+     * @param packageName The package name to prepopulate for.
+     * @param origin The origin to add as verified.
+     */
+    @VisibleForTesting
+    static void prePopulateVerifiedOriginForTesting(String packageName, Uri origin) {
+        cacheVerifiedOriginIfNeeded(packageName, origin);
+    }
+
+    private static Uri getPostMessageOriginFromVerifiedOrigin(
+            String packageName, Uri verifiedOrigin) {
+        return Uri.parse(IntentHandler.ANDROID_APP_REFERRER_SCHEME + "://"
+                + verifiedOrigin.getHost() + packageName);
+    }
+
+    private static void cacheVerifiedOriginIfNeeded(String packageName, Uri origin) {
+        if (sCachedOriginMap == null) sCachedOriginMap = new HashMap<>();
+        if (!sCachedOriginMap.containsKey(packageName)) {
+            sCachedOriginMap.put(packageName, origin);
+        }
+    }
+
+    private Uri getCachedOriginIfExists() {

[nyquist, 2017/04/19 06:35:55]

Nit: This is non-static. Any particular reason that this is all the way up here
before the constructor and public interfaces?

[Yusuf, 2017/04/26 00:51:35]

Nope. this was static before I added the mPackageName dependency. Moved it down.

+        if (sCachedOriginMap == null) return null;
+        return sCachedOriginMap.get(mPackageName);
+    }
+
+    public interface OriginVerificationListener {
+        void onOriginVerified(String packageName, Uri origin, boolean result);

[nyquist, 2017/04/19 06:35:55]

When is this called? Is this guaranteed to be posted, or can this be invoked
before the call to #start(...) finishes? From the sound of the description of
this CL we really want this result to always be posted back.

Also, it seems like there are cases where this is not called at all, which might
be confusing for the caller (params passed to native are bad for example)

Also, could you add JavaDoc to the params? Particularly the last one has an
unhelpful name. Or, if you don't want that, maybe just rename the variable to
|verified| or |success|?

[Yusuf, 2017/04/26 00:51:35]

Done.

+    }
+
+    /**
+     * Main constructor.
+     * Use {@link OriginVerifier#start(Uri)}
+     * @param listener The listener who will get the verification result.
+     * @param packageName The package for the Android application for verification.
+     */
+    public OriginVerifier(OriginVerificationListener listener, String packageName) {
+        mListener = listener;
+        mPackageName = packageName;
+        mSignatureFingerprint = getCertificateSHA256FingerprintForPackage(mPackageName);
+    }
+
+    /**
+     * Verify the claimed origin for the cached package name asynchronously.

[nyquist, 2017/04/19 06:35:55]

Nit: Could you mention that this will end up doing a network request? And also
mention with context that is used for the URLFetcher?

[Yusuf, 2017/04/26 00:51:35]

Done.

+     * @param origin The postMessage origin the application is claiming to have. Can't be null

[nyquist, 2017/04/19 06:35:56]

Nit: Missing period at the end.

[Yusuf, 2017/04/26 00:51:35]

Done.

+     */
+    public void start(@NonNull Uri origin) {

[nyquist, 2017/04/19 06:35:55]

Should this only be called from the main thread? It's changing internal state
that's later read in a callback from native (|mOrigin| for example). Or is the
plan here to protect this only by the fact that the class is package protected?

[Yusuf, 2017/04/26 00:51:35]

Yes, it should be on main thread only (Actually I might be crashing on the
current test because of that).

Made changes to that effect in the caller as well.

[nyquist, 2017/04/27 04:38:24]

Acknowledged.

+        mOrigin = origin;
+
+        // If this origin is cached as verified already, use that.
+        Uri cachedOrigin = getCachedOriginIfExists();
+        if (cachedOrigin != null && cachedOrigin.equals(origin)) {
+            originVerified(true);

[nyquist, 2017/04/19 06:35:56]

This looks like it's re-entrant. Should this be posted to the caller handler
instead to ensure that the API is consistent?

[Yusuf, 2017/04/26 00:51:35]

I posted back on the UI thread. But the native side worries me.

[nyquist, 2017/04/27 04:38:24]

I think you've fixed the native side too?

+            return;
+        }
+
+        if (mNativeOriginVerifier != 0) cleanUp();
+        mNativeOriginVerifier = nativeInit();
+        if (mNativeOriginVerifier == 0) return;

[nyquist, 2017/04/19 06:35:55]

This ends up not invoking the callback / listener at all. Is that intentional?

[Yusuf, 2017/04/26 00:51:35]

Yes, this is for non-integration tests to still create OriginVerifier with no
native component and test the logic for caching.

[nyquist, 2017/04/27 04:38:24]

Acknowledged.

+        nativeVerifyOrigin(
+                mNativeOriginVerifier, mPackageName, mSignatureFingerprint, mOrigin.toString());
+    }
+
+    private void cleanUp() {
+        nativeDestroy(mNativeOriginVerifier);
+        mNativeOriginVerifier = 0;
+    }
+
+    private String getCertificateSHA256FingerprintForPackage(String packageName) {

[nyquist, 2017/04/19 06:35:56]

Nit: What does this method return? I.e. what's the type of content?

[Yusuf, 2017/04/26 00:51:35]

The certificate for the apk in hex format:

Something like : 0F:D9:A0:CF:B0:7B:65:95:09.....

Were you saying we should document this? I would say the caller to
OriginVerifier shouldn't care much about these internals.

[nyquist, 2017/04/27 04:38:24]

What you did is fine.

+        PackageManager pm = ContextUtils.getApplicationContext().getPackageManager();

[nyquist, 2017/04/19 06:35:55]

Nit: Could you extract the package stuff into a private static PackageInfo
getPackageInfo(String packageName) {...} ?

[Yusuf, 2017/04/26 00:51:35]

Done.

+        PackageInfo packageInfo = null;
+        try {
+            packageInfo = pm.getPackageInfo(packageName, PackageManager.GET_SIGNATURES);
+        } catch (PackageManager.NameNotFoundException e) {
+            return null;
+        }
+        InputStream input = new ByteArrayInputStream(packageInfo.signatures[0].toByteArray());

[nyquist, 2017/04/19 06:35:55]

Could you extract out a method that takes packageInfo.signatures[0] (or the
byte[] if you want) as an argument to make it generic and easy to review for
security peeps? Maybe even make it package protected so you can easily test that
part?

[Yusuf, 2017/04/26 00:51:35]

See below, the one below already takes byte[]. Making that package protected.

[nyquist, 2017/04/27 04:38:24]

Acknowledged.

+        X509Certificate certificate = null;
+        String hexString = null;
+        try {
+            certificate =
+                    (X509Certificate) CertificateFactory.getInstance("X509").generateCertificate(

[nyquist, 2017/04/19 06:35:56]

Do you think it would be helpful for someone from chrome security to take a
quick peek at this part?

[Yusuf, 2017/04/26 00:51:35]

I can do that after you are comfortable for the state of the CL, but didn't
understand the security concerns here. We are calling PackageManager APIs for
other package names, so this is stuff pretty much all apps have access to. The
signature is not the developer signature, it is the APK signature which is
unique but public.

[nyquist, 2017/04/27 04:38:24]

Acknowledged.

+                            input);
+            hexString = byteArrayToHexString(
+                    MessageDigest.getInstance("SHA256").digest(certificate.getEncoded()));
+        } catch (CertificateEncodingException e) {
+            Log.w(TAG, "Certificate type X509 encoding failed");
+        } catch (CertificateException | NoSuchAlgorithmException e) {
+            // This shouldn't happen.
+        }
+        return hexString;
+    }
+
+    private static String byteArrayToHexString(byte[] byteArray) {

[nyquist, 2017/04/19 06:35:55]

What do you think about making this package protected and adding a test for it?

[Yusuf, 2017/04/26 00:51:35]

Done.

+        StringBuilder hexString = new StringBuilder(byteArray.length * 3 - 1);
+        for (int i = 0; i < byteArray.length; ++i) {
+            hexString.append(HEX_CHAR_LOOKUP[(byteArray[i] & 0xf0) >>> 4]);
+            hexString.append(HEX_CHAR_LOOKUP[byteArray[i] & 0xf]);
+            if (i < (byteArray.length - 1)) hexString.append(':');
+        }
+        return hexString.toString();
+    }
+
+    @CalledByNative
+    private void originVerified(boolean originVerified) {
+        if (originVerified) {
+            cacheVerifiedOriginIfNeeded(mPackageName, mOrigin);
+            mOrigin = getPostMessageOriginFromVerifiedOrigin(mPackageName, mOrigin);
+        }
+        mListener.onOriginVerified(mPackageName, mOrigin, originVerified);
+        cleanUp();
+    }
+
+    private native long nativeInit();
+    private native void nativeVerifyOrigin(long nativeOriginVerifier, String packageName,
+            String signatureFingerprint, String origin);
+    private native void nativeDestroy(long nativeOriginVerifier);
+}