Commment signed webapks working and verified.

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

Index: chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java
diff --git a/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java
new file mode 100644
index 0000000000000000000000000000000000000000..ce397f242d4be94d34b15b6f418f78ed691fac57
--- /dev/null
+++ b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java
@@ -0,0 +1,433 @@
+// 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.webapk.lib.client;
+
+import static java.nio.ByteOrder.BIG_ENDIAN;
+import static java.nio.ByteOrder.LITTLE_ENDIAN;
+
+import android.util.Log;
+
+import java.io.UnsupportedEncodingException;
+import java.nio.ByteBuffer;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
+import java.security.PublicKey;
+import java.security.Signature;
+import java.security.SignatureException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * WebApkVerifySignature reads in the APK file and verifies the WebApk signature. It reads the
+ * signature from the zip comment and verifies that it was signed by the public key passed.
+ */
+public class WebApkVerifySignature {
+    private static final String TAG = "WebApkVerifySignature";
+
+    /** End Of Central Directory Signature */
+    private static final long EOCD_SIG = 0x06054b50;
+
+    /** Central Directory Signature */
+    private static final long CD_SIG = 0x02014b50;
+
+    /** Local File Header Signature */
+    private static final long LFH_SIG = 0x04034b50;
+
+    /** Max end-of-central-directory size, including variable length file comment.. */
+    private static final int MIN_EOCD_SIZE = 22;
+
+    /** Max local file header size, including long filename. */
+    private static final int MAX_HEADER_SIZE = 64 * 8192;
+
+    /** Maximum number of META-INF/ files (allowing for dual signing). */
+    private static final int MAX_META_INF_FILES = 5;
+
+    /** The signature algorithm used (must also match with HASH) */
+    private static final String SIGNING_ALGORITHM = "SHA256withECDSA";
+
+    /**
+     * The pattern we look for in the APK/zip comment for singing key.
+     * An example is "chrome-webapk:0000:<pairs-of-hexvalues>"
+     */
+    private static final Pattern WEBAPK_COMMENT_PATTERN =
+            Pattern.compile("(?:chrome-)?webapk:\\s*(\\d+:\\s*)?((?:[a-fA-F0-9][a-fA-F0-9])+)");
+
+    /** Maximum comment length permitted */
+    private static final int MAX_COMMENT_LENGTH = 0;
+
+    /** Maximum extra field length permitted */
+    private static final int MAX_EXTRA_LENGTH = 8;
+
+    /** The memory buffer we are going to read the zip from */
+    private final ByteBuffer mBuffer;
+
+    /** Number of total central directory (zip entry) records */
+    private int mRecordCount;
+
+    /** Byte offset from the start where the central directory is found */
+    private int mCentralDirOffset;
+
+    /** The zip archive comment as a UTF-8 strings */
+    private String mComment;
+
+    /** Errors codes */
+    public enum Error {
+        OK,
+        BAD_APK,
+        EXTRA_FIELD_TOO_LARGE,
+        COMMENT_TOO_LARGE,
+        INCORRECT_SIGNATURE,
+        SIGNATURE_NOT_FOUND,
+        TOO_MANY_META_INF_FILES,
+    }
+
+    /**
+     * Sorted list of 'blocks' of memory we will cryptographically hash. We sort the blocks by
+     * filename to ensure a repeatable order.
+     */
+    private ArrayList<Block> mBlocks;
+
+    /** Block is the offset and size of a compressed zip entry. */
+    private static class Block implements Comparable<Block> {
+        Block(String filename, int position, int compressedSize) {
+            mFilename = filename;
+            mPosition = position;
+            mHeaderSize = 0;
+            mCompressedSize = compressedSize;
+        }
+
+        /** added for Comparable, sort lexicographically. */
+        @Override
+        public int compareTo(Block o) {
+            return mFilename.compareTo(o.mFilename);
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (!(o instanceof Block)) return false;
+            return mFilename.equals(((Block) o).mFilename);
+        }
+        @Override
+        public int hashCode() {
+            return mFilename.hashCode();
+        }
+
+        String mFilename;
+        int mPosition;
+        int mHeaderSize;
+        int mCompressedSize;
+    }
+
+    /** CTOR simply 'connects' to buffer passed. */
+    public WebApkVerifySignature(ByteBuffer buffer) {
+        mBuffer = buffer;
+        mBuffer.order(LITTLE_ENDIAN);
+    }
+
+    /**
+     * Read in the comment and directory. If there is no parseable comment we won't read the
+     * directory as there is no point (for speed). On success, all of our private variables will be
+     * set.
+     *
+     * @return OK on success.
+     */
+    public Error read() throws IllegalArgumentException {

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

Can you post this function in a try/catch statement{} that way this function
does not need to throw an Exception

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

done

+        Error err = readEOCD();
+        if (err != Error.OK) {
+            Log.d(TAG, "Missing EOCD Signature");
+            return err;
+        }
+        // Short circuit if no comment found.
+        if (parseCommentSignature(mComment) == null) {
+            return Error.SIGNATURE_NOT_FOUND;
+        }
+        err = readDirectory();
+        if (err != Error.OK) {
+            Log.d(TAG, "Error reading directory");
+            return err;
+        }
+        return Error.OK;
+    }
+
+    /**
+     * verifySignature hashes all the files and then verifies the signature.
+     *
+     * @param pub The public key that it should be verified against.
+     * @return OK if the public key signature verifies.
+     * @throws SignatureException for various reasons.
+     */
+    public Error verifySignature(PublicKey pub) throws SignatureException {
+        byte[] sig = parseCommentSignature(mComment);
+        if (sig == null || sig.length == 0) {
+            return Error.SIGNATURE_NOT_FOUND;
+        }
+        try {
+            Signature signature = Signature.getInstance(SIGNING_ALGORITHM);
+            signature.initVerify(pub);
+            Error err = calculateHash(signature);
+            if (err != Error.OK) {
+                return err;
+            }
+            return signature.verify(sig) ? Error.OK : Error.INCORRECT_SIGNATURE;
+        } catch (InvalidKeyException | NoSuchAlgorithmException | SignatureException
+                | UnsupportedEncodingException e) {
+            throw new SignatureException(
+                    "Failed to verify generated signature using public key from certificate", e);
+        }
+    }
+
+    /** @return Full APK/zip comment string. */
+    public String comment() {
+        return mComment;
+    }
+
+    /**
+     * calculateHash goes through each file listed in blocks and calculates the SHA-256
+     * cryptographic hash.
+     *
+     * @param sig Signature object you can call update on.
+     */
+    public Error calculateHash(Signature sig)
+            throws SignatureException, IllegalArgumentException, UnsupportedEncodingException {
+        byte[] filename;
+        ByteBuffer slice;
+        Collections.sort(mBlocks);
+        int metaInfCount = 0;
+        for (Block block : mBlocks) {
+            if (block.mFilename.startsWith("META-INF/")) {
+                metaInfCount++;
+                if (metaInfCount > MAX_META_INF_FILES) {
+                    return Error.TOO_MANY_META_INF_FILES;
+                }
+
+                // Files that begin with META-INF/ are not part of the hash.
+                // This is because these signatures are added after we comment signed the rest of
+                // the APK.
+                continue;
+            }
+
+            // Hash the filename length and filename as well to prevent Horton principle
+            // violation.
+            filename = block.mFilename.getBytes("UTF-8");
+            sig.update(toUInt32BigEndian(filename.length));
+            sig.update(filename);
+
+            // Also hash the block length for the same reason.
+            sig.update(toUInt32BigEndian(block.mCompressedSize));
+
+            seek(block.mPosition + block.mHeaderSize);
+            slice = mBuffer.slice();
+            slice.limit(block.mCompressedSize);
+            sig.update(slice);
+        }
+        return Error.OK;
+    }
+
+    /**
+     * toUInt32BigEndian converts an integer to a big endian array of bytes.
+     *
+     * @param value Integer value to convert.
+     * @return Array of bytes.
+     */
+    private byte[] toUInt32BigEndian(int value) {
+        ByteBuffer buffer = ByteBuffer.allocate(4);
+        buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity.
+        buffer.putInt(value);
+        return buffer.array();
+    }
+
+    /**
+     * Extract the bytes of the signature from the comment. We expect
+     * "chrome-webapk:0000:<hexvalues>" comment followed by hex values. Currently we ignore the key
+     * id which is always "0000".
+     *
+     * @return the bytes of the signature.
+     */
+    static byte[] parseCommentSignature(String comment) {
+        Matcher m = WEBAPK_COMMENT_PATTERN.matcher(comment);
+        if (!m.find()) {
+            return null;
+        }
+        String s = m.group(2);
+        if (s.length() == 0) {
+            return null;
+        }
+        return hexToBytes(s);
+    }
+
+    /**
+     * Reads the End of Central Directory Records, returns false if it can't find it.
+     *
+     * @return OK on success.
+     */
+    private Error readEOCD() throws IllegalArgumentException {
+        int start = findEOCDStart();
+        if (start < 0) {
+            return Error.BAD_APK;
+        }
+        //  Signature(4), Disk Number(2), Start disk number(2), Records on this disk (2)
+        seek(start + 10);
+        mRecordCount = read2(); // Number of Central Directory records
+        read4(); // Size of central directory
+        mCentralDirOffset = read4(); // as bytes from start of file.
+        int commentLength = read2();
+        mComment = readString(commentLength);
+        return Error.OK;
+    }
+
+    /**
+     * readDirectory goes through the central directory and the local file header blocks. This is
+     * used to calculate the offset and size of each block. We also get the filenames for sorting
+     * purposes.
+     *
+     * @return OK on success.
+     */
+    Error readDirectory() {
+        mBlocks = new ArrayList<>(mRecordCount);
+        int curr = mCentralDirOffset;
+        for (int i = 0; i < mRecordCount; i++) {
+            seek(curr);
+            int signature = read4();
+            if (signature != CD_SIG) {
+                Log.d(TAG, "Missing Central Directory Signature");
+                return Error.BAD_APK;
+            }
+            // CreatorVersion(2), ReaderVersion(2), Flags(2), CompressionMethod(2)
+            // ModifiedTime(2), ModifiedDate(2), CRC32(4) = 16 bytes
+            seekDelta(16);
+            int compressedSize = read4();
+            seekDelta(4); // uncompressed size
+            int fileNameLength = read2();
+            int extraLen = read2();
+            int fileCommentLength = read2();
+            seekDelta(8); // DiskNumberStart(2), Internal Attrs(2), External Attrs(4)
+            int offset = read4();
+            String filename = readString(fileNameLength);
+            curr = mBuffer.position() + extraLen + fileCommentLength;
+            if (extraLen > MAX_EXTRA_LENGTH) {
+                Log.w(TAG,
+                        String.format(
+                                "Extra field too large for file %s: %d bytes", filename, extraLen));
+                return Error.EXTRA_FIELD_TOO_LARGE;
+            }
+            if (fileCommentLength > MAX_COMMENT_LENGTH) {
+                Log.w(TAG,
+                        String.format("Unexpected comment field file %s: %d bytes", filename,
+                                fileCommentLength));
+                return Error.COMMENT_TOO_LARGE;
+            }
+            mBlocks.add(new Block(filename, offset, compressedSize));
+        }
+
+        // Read the 'local file header' block to the size of the header in bytes.
+        for (Block block : mBlocks) {
+            curr = block.mPosition;
+            seek(curr);
+            int signature = read4();
+            if (signature != LFH_SIG) {
+                Log.d(TAG, "LFH Signature missing");
+                return Error.BAD_APK;
+            }
+            // ReaderVersion(2), Flags(2), Method(2),
+            // ModifiedTime (2), ModifiedDate(2), CRC32(4), CompressedSize(4),
+            // UncompressedSize(4) = 22 bytes
+            seekDelta(22);
+            int fileNameLength = read2();
+            int extraFieldLength = read2();
+            // 26 + 4 = 30 bytes
+            block.mHeaderSize = 30 + fileNameLength + extraFieldLength;
+        }
+        return Error.OK;
+    }
+
+    /**
+     * We search buffer for EOCD_SIG and return the location where we found it. If the file has no
+     * comment it should seek only once.
+     *
+     * @return Offset from start of buffer or -1 if not found.
+     */
+    private int findEOCDStart() throws IllegalArgumentException {
+        int offset = mBuffer.limit() - MIN_EOCD_SIZE;
+        seek(offset);
+        for (int count = 0; count < MAX_HEADER_SIZE; count++) {
+            if (read4() == EOCD_SIG) {
+                // found!
+                return offset;
+            }
+            offset--;
+            if (offset < 0) {
+                return -1;
+            }
+            seek(offset);
+        }
+        return -1;
+    }
+
+    /**
+     * Seek to this position.
+     *
+     * @param offset offset from start of file.
+     */
+    private void seek(int offset) throws IllegalArgumentException {

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

I don't think that the throws is useful here. IllegalArgumentException is an
unchecked exception (The compiler does not complain if you don't handle it)

Every single method in this file can throw an Exception. For instance,
ByteBuffer#getInt() can throw BufferUnderflowException. I suggest not marking
any functions in this file as throwing an Exception explicitly and have a
try/catch block in both read() and verifySignature()

You can do any appropriate logging in the catch{} block

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

I think I've done what you suggest. Thanks!

+        mBuffer.position(offset);
+    }
+
+    /**
+     * Skip forward this number of bytes.
+     *
+     * @param delta number of bytes to seek forward.
+     */
+    private void seekDelta(int delta) throws IllegalArgumentException {
+        mBuffer.position(mBuffer.position() + delta);
+    }
+
+    /**
+     * Reads two bytes in little endian format.
+     * @return short value read (as an int).
+     */
+    private int read2() {
+        return mBuffer.getShort();
+    }
+
+    /**
+     * Reads four bytes in little endian format.
+     * @return value read.
+     */
+    private int read4() {
+        return mBuffer.getInt();
+    }
+
+    /** Read {@link length} many bytes into a string */
+    private String readString(int length) {
+        if (length <= 0) {
+            return "";
+        }
+        byte[] bytes = new byte[length];
+        mBuffer.get(bytes);
+        return new String(bytes);
+    }
+
+    /** Convert a hex string into bytes. We store hex in the signature as zip tools often don't *
+     * like binary strings. */
+    static byte[] hexToBytes(String s) {
+        int len = s.length();
+        if (len % 2 != 0) {
+            Log.d(TAG, "Got an odd number of hex nibbles.");
+            return null;
+        }
+        if (len == 0) {
+            Log.d(TAG, "Got no hex values.");
+            return null;
+        }
+        byte[] data = new byte[len / 2];
+        for (int i = 0; i < len; i += 2) {
+            data[i / 2] = (byte) ((Character.digit(s.charAt(i), 16) << 4)
+                    + Character.digit(s.charAt(i + 1), 16));
+        }
+        return data;
+    }
+}