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 36840fc0358cb5be575b5454089d30d1f02ee302..4dda8ffb24c3626c57f78541b43fa31cc6ca02ac 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;
@@ -17,6 +18,14 @@ import android.util.Log;
 
 import org.chromium.base.annotations.SuppressFBWarnings;
 
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.MappedByteBuffer;
+import java.nio.channels.FileChannel;
+import java.security.KeyFactory;
+import java.security.PublicKey;
+import java.security.SignatureException;
+import java.security.spec.X509EncodedKeySpec;
 import java.util.Arrays;
 import java.util.LinkedList;
 import java.util.List;
@@ -26,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.
@@ -47,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));
@@ -92,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);
@@ -120,52 +132,154 @@ public class WebApkValidator {
 
     /**
      * Returns whether the provided WebAPK is installed and passes signature checks.
+     *

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

Nit: Remove the new line

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

done.

      * @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;
+        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);
         } catch (NameNotFoundException e) {
             e.printStackTrace();
             Log.d(TAG, "WebApk not found");
             return false;
         }
+        if (isNotWebApkQuick(packageInfo)) {
+            return false;
+        }
+        if (verifyV1WebApk(packageInfo, webappPackageName)) {
+            return true;
+        }
 
-        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;
-                }
+        return verifyCommentSignedWebApk(packageInfo);
+    }
+
+    /** 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) {
+            // 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());

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

You can use TextUtils.isEmpty() to do this

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

On 2017/03/31 at 03:59:24, pkotwicz
wrote:chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java
Thanks, I vaguely remember there was a tool for that.

+    }
+
+    private static boolean verifyV1WebApk(PackageInfo packageInfo, String webappPackageName) {
+        if (packageInfo.signatures.length != 2
+                || !webappPackageName.startsWith(WEBAPK_PACKAGE_PREFIX)) {
+            return false;
+        }
+        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;
     }
 
+    /** Verify that the comment signed webapk matches the public key. */
+    private static boolean verifyCommentSignedWebApk(PackageInfo packageInfo) {
+        PublicKey commentSignedPublicKey;
+        try {
+            commentSignedPublicKey = getCommentSignedPublicKey();
+        } catch (Exception e) {
+            Log.e(TAG, "WebApk failed to get Public Key", e);
+            return false;
+        }
+        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;

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

Can |buf| be moved inside to try {} statement?

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

Done.

+        try {
+            file = new RandomAccessFile(packageFilename, "r");
+            inChannel = file.getChannel();
+            buf = inChannel.map(FileChannel.MapMode.READ_ONLY, 0, inChannel.size());
+            buf.load();
+
+            WebApkVerifySignature v = new WebApkVerifySignature(buf);
+            @WebApkVerifySignature.Error

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

I haven't seen annotations on enum ints anywhere but in function parameters.
Remove the annotation?

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

sure.

+            int result = v.read();
+            if (result != WebApkVerifySignature.ERROR_OK) {
+                Log.e(TAG, String.format("Failure reading %s: %s", packageFilename, result));
+                return false;
+            }
+            result = v.verifySignature(commentSignedPublicKey);
+            Log.d(TAG, "File " + packageFilename + ": " + result);
+            return result == WebApkVerifySignature.ERROR_OK;
+        } catch (SignatureException e) {
+            Log.e(TAG, "WebApk SignatureException for file " + packageFilename, e);
+            return false;
+        } catch (IllegalArgumentException | IOException e) {

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

You can just check the generic exception here - catch (Exception e) {}. For
instance, you are not catching FileNotFoundException which can be thrown when
constructing the RandomAccessFile.

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

Thanks!

+            Log.e(TAG, "WebApk file error for file " + packageFilename, e);
+            return false;
+        } finally {
+            if (inChannel != null) {
+                try {
+                    inChannel.close();
+                } catch (IOException e) {
+                }
+            }
+            if (file != null) {
+                try {
+                    file.close();
+                } catch (IOException e) {
+                }
+            }
+        }
+    }
+
     /**
-     * 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.
+     *

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

Nit: Remove new line

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

Done.

      * @param expectedSignature
+     * @param v2PublicKeyBytes

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

Remove @params if you are not documenting them

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

Done.

      */
     @SuppressFBWarnings("EI_EXPOSE_STATIC_REP2")
-    public static void initWithBrowserHostSignature(byte[] expectedSignature) {
-        if (sExpectedSignature != null) {
-            return;
+    public static void initWithBrowserHostSignature(
+            byte[] expectedSignature, byte[] v2PublicKeyBytes) {
+        if (sExpectedSignature == null) {
+            sExpectedSignature = expectedSignature;
+        }
+        if (sCommentSignedPublicKeyBytes == null) {
+            sCommentSignedPublicKeyBytes = v2PublicKeyBytes;
+        }
+    }
+
+    /**
+     * 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() throws Exception {
+        if (sCommentSignedPublicKey == null) {
+            sCommentSignedPublicKey =
+                    KeyFactory.getInstance(KEY_FACTORY)
+                            .generatePublic(new X509EncodedKeySpec(sCommentSignedPublicKeyBytes));
         }
-        sExpectedSignature = expectedSignature;
+        return sCommentSignedPublicKey;
     }
 }