Commment signed webapks working and verified.

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

 // Copyright 2016 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.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;
 import android.content.pm.PackageInfo;
 import android.content.pm.PackageManager;
 import android.content.pm.PackageManager.NameNotFoundException;
 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;
 
 /**
  * Checks whether a URL belongs to a WebAPK, and whether a WebAPK is signed by the WebAPK Minting
  * Server.
  */
 public class WebApkValidator {
+    private static final String TAG = "WebApkValidator";
+    private static final String KEY_FACTORY = "EC"; // aka "ECDSA"
 
-    private static final String TAG = "WebApkValidator";
     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.
      * @return Package name of WebAPK which can handle the URL. Null if the url should not be
      * handled by a WebAPK.
      */
     public static String queryWebApkPackage(Context context, String url) {
         return findWebApkPackage(context, resolveInfosForUrl(context, url));
     }
 
     /**
-     * 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));
     }
 
     /**
      * Fetches the list of resolve infos from the PackageManager that can handle the URL.
      *
      * @param context The application context.
      * @param url The url to check.
@@ skipping 15 matching lines @@
             selector.setComponent(null);
         }
         return context.getPackageManager().queryIntentActivities(
                 intent, PackageManager.GET_RESOLVED_FILTER);
     }
 
     /**
      * @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);
         if (resolveInfo != null) {
             return resolveInfo.activityInfo.packageName;
         }
         return null;
     }
 
     /**
      * @param context The context to use to check whether WebAPK is valid.
      * @param infos The ResolveInfos to search.
      * @return ResolveInfo which corresponds to a WebAPK. Null if none of the ResolveInfos
      * corresponds to a WebAPK.
      */
     private static ResolveInfo findResolveInfo(Context context, List<ResolveInfo> infos) {
         for (ResolveInfo info : infos) {
             if (info.activityInfo != null
                     && isValidWebApk(context, info.activityInfo.packageName)) {
                 return info;
             }
         }
         return null;
     }
 
     /**
      * 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 (sExpectedSignature == null || sCommentSignedPublicKeyBytes == null) {
+            Log.wtf(TAG,
+                    "WebApk validation failure - expected signature not set."
+                            + "missing call to WebApkValidator.initWithBrowserHostSignature");
         }
-        if (!webappPackageName.startsWith(WEBAPK_PACKAGE_PREFIX)) {
-            return false;
-        }
-        // 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;
-                }
-            }
+        try {
+            return verifyCommentSignedWebApk(packageInfo);
+        } catch (IOException e) {
+            Log.e(TAG, "WebApk IOException", e);
+        } catch (SignatureException e) {
+            Log.e(TAG, "WebApk SignatureException", e);
+        } catch (IllegalArgumentException | InvalidKeySpecException | NoSuchAlgorithmException e) {
+            Log.e(TAG, "WebApk Error", e);
         }
-        Log.d(TAG, "WebApk invalid");
         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) {
+            // 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());
+    }
+
+    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;
+            }
+        }
+        return false;
+    }
+
+    /** Verify that the comment signed webapk matches the public key. */
+    private static boolean verifyCommentSignedWebApk(PackageInfo packageInfo)
+            throws IOException, SignatureException, IllegalArgumentException,
+                   InvalidKeySpecException, NoSuchAlgorithmException {

[pkotwicz, 2017/03/28 18:35:41]

This method should not throw any exceptions

[ScottK, 2017/03/29 17:59:12]

done

+        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();
+
+            WebApkVerifySignature v = new WebApkVerifySignature(buf);
+            verified = v.read();
+
+            if (verified != WebApkVerifySignature.Error.OK) {
+                Log.e(TAG, String.format("Failure reading %s: %s", packageFilename, verified));
+                return false;
+            }
+            verified = v.verifySignature(sCommentSignedPublicKey);

[pkotwicz, 2017/03/28 18:35:41]

According to
http://stackoverflow.com/questions/65035/does-finally-always-execute-in-java
you can make |verified| local to the try {} block and call return from within
the try {} statement.

Maybe rename |verified| to |result|. I would expect |verified| to be a boolean.
It is confusing that it is not a boolean

Note that the default value for |verified| is WebApkVerifySignature.Error.OK.
Hence, if an exception is thrown inside the try{} statement this function
returns true

[ScottK, 2017/03/29 17:59:12]

Returning here.

+            Log.d(TAG, "File " + packageFilename + ": " + verified);
+        } finally {
+            if (buf != null) {
+                buf.clear();

[pkotwicz, 2017/03/28 18:35:41]

Is this necessary? Based on the documentation Buffer#clear() does very little

[ScottK, 2017/03/29 17:59:12]

Guess not, thought it might be a hint to the gc.

+            }
+            if (inChannel != null) {
+                inChannel.close();
+            }
+            if (file != null) {
+                file.close();
+            }
+        }
+        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);
         }
-        sExpectedSignature = Arrays.copyOf(expectedSignature, expectedSignature.length);
+        if (sCommentSignedPublicKeyBytes == null) {
+            sCommentSignedPublicKeyBytes = Arrays.copyOf(v2PublicKeyBytes, v2PublicKeyBytes.length);
+        }
+    }
+
+    /**
+     * Lazy evaluate the creation of the Public Key as the KeyFactories may not yet be initialized.
+     *

[pkotwicz, 2017/03/28 18:35:41]

Nit: Remove the new line

+     * @return The decoded PublicKey or null
+     */
+    private static PublicKey getCommentSignedPublicKey()
+            throws InvalidKeySpecException, NoSuchAlgorithmException {

[pkotwicz, 2017/03/28 18:35:41]

You can simplify this to "throws Exception"

[ScottK, 2017/03/29 17:59:12]

done

+        if (sCommentSignedPublicKey == null) {
+            sCommentSignedPublicKey =
+                    KeyFactory.getInstance(KEY_FACTORY)
+                            .generatePublic(new X509EncodedKeySpec(sCommentSignedPublicKeyBytes));
+        }
+        return sCommentSignedPublicKey;
     }
 }