Commment signed webapks working and verified.

chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java

Index: chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java
diff --git a/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java
index 14d656ffb7c440a998c69acc4cf8401e2f68feba..1f556aab4b359570c4e8de5bf178fb0866a20adf 100644
--- a/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java
+++ b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkValidator.java
@@ -5,6 +5,7 @@
 package org.chromium.webapk.lib.client;
 
 import static org.chromium.webapk.lib.common.WebApkConstants.WEBAPK_PACKAGE_PREFIX;
+import static org.chromium.webapk.lib.common.WebApkMetaDataKeys.START_URL;
 
 import android.content.Context;
 import android.content.Intent;
@@ -15,6 +16,16 @@ import android.content.pm.ResolveInfo;
 import android.content.pm.Signature;
 import android.util.Log;
 
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.MappedByteBuffer;
+import java.nio.channels.FileChannel;
+import java.security.KeyFactory;
+import java.security.NoSuchAlgorithmException;
+import java.security.PublicKey;
+import java.security.SignatureException;
+import java.security.spec.InvalidKeySpecException;
+import java.security.spec.X509EncodedKeySpec;
 import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.List;
@@ -24,16 +35,19 @@ import java.util.List;
  * Server.
  */
 public class WebApkValidator {
-
     private static final String TAG = "WebApkValidator";
+    private static final String KEY_FACTORY = "EC"; // aka "ECDSA"
+
     private static byte[] sExpectedSignature;
+    private static byte[] sCommentSignedPublicKeyBytes;
+    private static PublicKey sCommentSignedPublicKey;
 
     /**
-     * Queries the PackageManager to determine whether a WebAPK can handle the URL. Ignores
-     * whether the user has selected a default handler for the URL and whether the default
-     * handler is the WebAPK.
+     * Queries the PackageManager to determine whether a WebAPK can handle the URL. Ignores whether
+     * the user has selected a default handler for the URL and whether the default handler is the
+     * WebAPK.
      *
-     * NOTE(yfriedman): This can fail if multiple WebAPKs can match the supplied url.
+     * <p>NOTE(yfriedman): This can fail if multiple WebAPKs can match the supplied url.
      *
      * @param context The application context.
      * @param url The url to check.
@@ -45,16 +59,16 @@ public class WebApkValidator {
     }
 
     /**
-     * Queries the PackageManager to determine whether a WebAPK can handle the URL. Ignores
-     * whether the user has selected a default handler for the URL and whether the default
-     * handler is the WebAPK.
+     * Queries the PackageManager to determine whether a WebAPK can handle the URL. Ignores whether
+     * the user has selected a default handler for the URL and whether the default handler is the
+     * WebAPK.
      *
-     * NOTE: This can fail if multiple WebAPKs can match the supplied url.
+     * <p>NOTE: This can fail if multiple WebAPKs can match the supplied url.
      *
      * @param context The application context.
      * @param url The url to check.
      * @return Resolve Info of a WebAPK which can handle the URL. Null if the url should not be
-     * handled by a WebAPK.
+     *     handled by a WebAPK.
      */
     public static ResolveInfo queryResolveInfo(Context context, String url) {
         return findResolveInfo(context, resolveInfosForUrl(context, url));
@@ -90,7 +104,7 @@ public class WebApkValidator {
      * @param context The context to use to check whether WebAPK is valid.
      * @param infos The ResolveInfos to search.
      * @return Package name of the ResolveInfo which corresponds to a WebAPK. Null if none of the
-     * ResolveInfos corresponds to a WebAPK.
+     *     ResolveInfos corresponds to a WebAPK.
      */
     public static String findWebApkPackage(Context context, List<ResolveInfo> infos) {
         ResolveInfo resolveInfo = findResolveInfo(context, infos);
@@ -118,51 +132,175 @@ public class WebApkValidator {
 
     /**
      * Returns whether the provided WebAPK is installed and passes signature checks.
+     *
      * @param context A context
      * @param webappPackageName The package name to check
      * @return true iff the WebAPK is installed and passes security checks
      */
     public static boolean isValidWebApk(Context context, String webappPackageName) {
-        if (sExpectedSignature == null) {
-            Log.wtf(TAG, "WebApk validation failure - expected signature not set."
-                    + "missing call to WebApkValidator.initWithBrowserHostSignature");
-        }
-        if (!webappPackageName.startsWith(WEBAPK_PACKAGE_PREFIX)) {
-            return false;
+        long now = System.nanoTime();
+        if (sExpectedSignature == null || sCommentSignedPublicKeyBytes == null) {
+            Log.wtf(TAG,
+                    "WebApk validation failure - expected signature not set."
+                            + "missing call to WebApkValidator.initWithBrowserHostSignature");
         }
-        // check signature
         PackageInfo packageInfo = null;
         try {
             packageInfo = context.getPackageManager().getPackageInfo(webappPackageName,
-                    PackageManager.GET_SIGNATURES);
+                    PackageManager.GET_SIGNATURES | PackageManager.GET_META_DATA);

[pkotwicz, 2017/03/26 01:37:04]

Is this call slower when called with PackageManager.Get_SIGNATURES (I am unsure)

[ScottK, 2017/03/27 20:27:56]

Doesn't seem like much, although I only tested twice in each case was 0.73ms and
now about 0.76ms or about 0.03ms slower.

[pkotwicz, 2017/03/28 04:41:34]

That seems negligible

         } catch (NameNotFoundException e) {
             e.printStackTrace();
             Log.d(TAG, "WebApk not found");
             return false;
         }
+        if (isNotWebApkQuick(packageInfo)) {
+            logTime(now, "get packageInfo Quick");
+            return false;
+        }
+
+        if (isV1WebApk(packageInfo, webappPackageName)) {
+            logTime(now, "isV1WebApk");
+            return foundV1Signature(packageInfo);
+        }
+
+        logTime(now, "isCommentSignedWebApk");

[pkotwicz, 2017/03/26 01:37:04]

I think that it is useful to have a histogram which logs how long
WebappLauncherActivity#isValidWebApk() takes.

I don't think that the time logs here are useful. They are only useful for our
own testing (we can easily build a custom version of Chrome with the logging)

- The timing logs are not helpful when trying to debug bugs reported by users
- The timing logs are not helpful in debugging a crash
- The timing logs make Chrome bigger

I suggest removing the timing logs

[ScottK, 2017/03/27 20:27:56]

I've removed the timing logs.

+        try {
+            return verifyCommentSignedWebApk(packageInfo);
+        } catch (IOException e) {
+            Log.e(TAG, "WebApk IOException", e);
+        } catch (SignatureException e) {
+            Log.e(TAG, "WebApk SignatureException", e);
+        } catch (IllegalArgumentException e) {
+            Log.e(TAG, "WebApk IllegalArgument", e);
+        }
+        return false;
+    }
+
+    /** Determine quickly whether this is definitely not a WebAPK */
+    private static boolean isNotWebApkQuick(PackageInfo packageInfo) {
+        if (packageInfo.signatures == null || packageInfo.signatures.length == 0
+                || packageInfo.signatures.length > 2) {

[pkotwicz, 2017/03/26 01:37:04]

I don't think that this check is useful anymore. Isn't a comment signed WebAPK
still valid if the user decides to sign it many many times?

[ScottK, 2017/03/27 20:27:56]

If they have no signatures, that's bad.
If they have more than 2 signatures they are probably doing something wrong as
well.
I've update the code to allow a limit of 5 META-INF files.  Basically, dual
signing plus MANIFEST.MF and return an error if it exceeds that.

[pkotwicz, 2017/03/28 04:41:34]

We should let the user sign their APK however many times they want. We should
not burden developers with requirements which are not required by the Play
Store. If the Play Store enables a user to upload a WebAPK with no signatures
there is no reason that we shouldn't allow the user to launch the WebAPK

[ScottK, 2017/03/28 20:56:21]

You can't upload an APK with no signatures and I'm pretty sure it will also
won't let you have two or more signatures.
Additionally, there's a small concern that a hacker may try stuffing code in
META-INF, thus the extra checks.

[pkotwicz, 2017/03/31 03:59:23]

I think that the checks that you have for the amount of files in META-INF is
sufficient. Please remove the check for the number of signatures

[ScottK, 2017/04/03 17:44:17]

So the META-INF checks are only checked if it's a comment signed webapk. I think
we may still want to check the number of signatures for v1 signed webapks, what
do you think?

[pkotwicz, 2017/04/04 18:19:36]

I think that is reasonable. You have a separate check for the number of
signatures in isV1WebApk()

+            // Wrong number of signatures want 1 or 2.
+            return true;
+        }
+        if (packageInfo.applicationInfo == null || packageInfo.applicationInfo.metaData == null) {
+            Log.e(TAG, "no application info, or metaData retrieved.");
+            return true;
+        }
+        // Having the startURL in AndroidManifest.xml is a strong signal.
+        String startUrl = packageInfo.applicationInfo.metaData.getString(START_URL);
+        return (startUrl == null || startUrl.isEmpty());
+    }
 
-        final Signature[] arrSignatures = packageInfo.signatures;
-        if (arrSignatures != null && arrSignatures.length == 2) {
-            for (Signature signature : arrSignatures) {
-                if (Arrays.equals(sExpectedSignature, signature.toByteArray())) {
-                    Log.d(TAG, "WebApk valid - signature match!");
-                    return true;
-                }
+    private static boolean isV1WebApk(PackageInfo packageInfo, String webappPackageName) {
+        return packageInfo.signatures.length == 2
+                && webappPackageName.startsWith(WEBAPK_PACKAGE_PREFIX);
+    }
+
+    private static boolean foundV1Signature(PackageInfo packageInfo) {

[pkotwicz, 2017/03/26 01:37:04]

You can probably merge isV1WebApk() and foundV1Signature()

verifyV1WebApk() perhaps

[ScottK, 2017/03/27 20:27:55]

Done.

+        for (Signature signature : packageInfo.signatures) {
+            if (Arrays.equals(sExpectedSignature, signature.toByteArray())) {
+                Log.d(TAG, "WebApk valid - signature match!");
+                return true;
             }
         }
-        Log.d(TAG, "WebApk invalid");
         return false;
     }
 
+    private static long logTime(long start, String message) {
+        long now = System.nanoTime();
+        Log.d(TAG, String.format("%f ms: %s", (now - start) / 1E6, message));
+        return now;
+    }
+
+    /** Verify that the comment signed webapk matches the public key. */
+    private static boolean verifyCommentSignedWebApk(PackageInfo packageInfo)
+            throws IOException, SignatureException, IllegalArgumentException {
+        long now = System.nanoTime();
+
+        PublicKey CommentSignedPublicKey = getCommentSignedPublicKey();
+        if (CommentSignedPublicKey == null) {
+            Log.e(TAG, "WebApk validation failure - unable to decode public key");
+            return false;
+        }
+        if (packageInfo.applicationInfo == null || packageInfo.applicationInfo.sourceDir == null) {
+            Log.e(TAG, "WebApk validation failure - missing applicationInfo sourcedir");
+            return false;
+        }
+
+        String packageFilename = packageInfo.applicationInfo.sourceDir;
+        RandomAccessFile file = null;
+        FileChannel inChannel = null;
+        MappedByteBuffer buf = null;
+        WebApkVerifySignature.Error verified;
+        try {
+            file = new RandomAccessFile(packageFilename, "r");
+            inChannel = file.getChannel();
+            buf = inChannel.map(FileChannel.MapMode.READ_ONLY, 0, inChannel.size());
+            buf.load();
+
+            now = logTime(now, "opening file " + packageFilename);
+
+            WebApkVerifySignature v = new WebApkVerifySignature(buf);
+            verified = v.read();
+
+            now = logTime(now, "read");
+
+            if (verified != WebApkVerifySignature.Error.OK) {
+                Log.e(TAG, String.format("Failure reading %s: %s", packageFilename, verified));
+                return false;
+            }
+            verified = v.verifySignature(sCommentSignedPublicKey);
+            Log.d(TAG, "File " + packageFilename + ": " + verified);
+        } finally {
+            if (buf != null) {
+                buf.clear();
+            }
+            if (inChannel != null) {
+                inChannel.close();
+            }
+            if (file != null) {
+                file.close();
+            }
+            logTime(now, "verify");
+        }
+        return verified == WebApkVerifySignature.Error.OK;
+    }
+
     /**
-     * Initializes the WebApkValidator with the expected signature that WebAPKs must be signed
-     * with for the current host.
+     * Initializes the WebApkValidator with the expected signature that WebAPKs must be signed with
+     * for the current host.
+     *
      * @param expectedSignature
+     * @param v2PublicKeyBytes
      */
-    public static void initWithBrowserHostSignature(byte[] expectedSignature) {
-        if (sExpectedSignature != null) {
-            return;
+    public static void initWithBrowserHostSignature(
+            byte[] expectedSignature, byte[] v2PublicKeyBytes) {
+        if (sExpectedSignature == null) {
+            sExpectedSignature = Arrays.copyOf(expectedSignature, expectedSignature.length);
+        }
+        if (sCommentSignedPublicKeyBytes == null) {
+            sCommentSignedPublicKeyBytes = Arrays.copyOf(v2PublicKeyBytes, v2PublicKeyBytes.length);
+        }

[pkotwicz, 2017/03/26 01:37:04]

Is there a reason that we are copying the array?

My understanding is that we are copying the array because Java does not offer
const references the way that C++ does. I don't think that we need to modify the
array because this class is not mutating the array

[ScottK, 2017/03/27 20:27:56]

The original code has Arrays.copyOf() for sExpectedSignature, I just copied what
was done before.
I'll change it and test it.

[ScottK, 2017/03/28 20:56:21]

The try bots didn't like this change. I think the reason for this original
copying is to appease the try bots and because we can't call include
WebApkHostSignature from chromium.

+    }
+
+    /**
+     * Lazy evaluate the creation of the Public Key as the KeyFactories may not yet be initialized.
+     *
+     * @return The decoded PublicKey or null
+     */
+    private static PublicKey getCommentSignedPublicKey() {
+        if (sCommentSignedPublicKey == null) {
+            try {
+                sCommentSignedPublicKey = KeyFactory.getInstance(KEY_FACTORY)
+                                                  .generatePublic(new X509EncodedKeySpec(
+                                                          sCommentSignedPublicKeyBytes));
+            } catch (InvalidKeySpecException e) {

[pkotwicz, 2017/03/26 01:37:04]

Nit: You can probably catch the generic Exception instead and have a single
catch() statement.

You don't need to do any logging here because you already do logging in the
caller of getCommentSignedPublicKey()

[ScottK, 2017/03/27 20:27:56]

done.

+                Log.e(TAG, "Failed to understand public key: " + e.getMessage());
+            } catch (NoSuchAlgorithmException e) {
+                Log.e(TAG, "Failed to find KeyFactory: " + e.getMessage());
+            }
         }
-        sExpectedSignature = Arrays.copyOf(expectedSignature, expectedSignature.length);
+        return sCommentSignedPublicKey;
     }
 }