Commment signed webapks working and verified.

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

+// 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;

[pkotwicz, 2017/03/28 04:31:03]

We might as well use the constants from ZipFile instead of redefining EOCD_SIG,
CD_SIG and LFH_SIG here

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

Ping on this comment

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

I'd rather not have a dependency, plus these constants won't change any time
soon.

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

OK

+
+    /** 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 {

[pkotwicz, 2017/03/28 04:31:03]

I think that we have a policy against using enums in Java code in Chromium. (At
least we used to). Yaron should be able to confirm or deny.

+        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 {
+        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) {

[pkotwicz, 2017/03/28 04:31:03]

Maybe simplify this to catch (Exception) {}

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

Ping on this comment

+            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 {

[pkotwicz, 2017/03/28 04:31:03]

You can simplify this to "throws Exception"

+        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) {

[pkotwicz, 2017/03/28 04:31:03]

Perhaps a better name for this function would be:
intToBigEndianBytes()

This function does not convert to an unsigned int. I suggest removing the uint
part from the function name because Java does not have unsigned ints

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

Ping on this comment

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

🚨🚨 Ping on this comment again 🚨🚨

+        ByteBuffer buffer = ByteBuffer.allocate(4);

[pkotwicz, 2017/03/28 04:31:03]

Can we make this little endian for the sake of consistency? (I don't think the
endianness matters as long as the encoder logic and the decoder logic on the
client match)

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

Ping on this comment

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

🚨🚨 Ping on this comment again 🚨🚨

+        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;

[pkotwicz, 2017/03/28 04:31:03]

You can replace this with seekDelta() too and move the seek() on line 293
outside of the for() loop.

It would be nice if we can get rid of the |curr| variable entirely

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

Ping on this comment

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

🚨🚨 Ping on this comment again 🚨🚨

[ScottK, 2017/04/04 21:03:02]

Done.

+            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;

[pkotwicz, 2017/03/28 04:31:03]

Maybe do this:

int headerStart = block.mPosition;

and then on line 342:
block.mHeaderSize = (mBuffer.position() - headerStart) + fileNameLength +
extraFieldLength;

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

Ping on this comment

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

🚨🚨 Ping on this comment again 🚨🚨

[ScottK, 2017/04/04 21:03:02]

More or less did the same thing, but avoided the temporary.

+            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 {
+        mBuffer.position(offset);
+    }
+
+    /**
+     * Skip forward this number of bytes.
+     *

[pkotwicz, 2017/03/28 04:31:03]

Nit: remove new line

[ScottK, 2017/04/04 21:03:02]

Done.

+     * @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. */

[pkotwicz, 2017/03/28 04:31:03]

Nit:
"*/" on new line

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

Ping on this comment

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

🚨🚨 Ping on this comment again 🚨🚨

[ScottK, 2017/04/04 21:03:02]

Done.

+    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;
+    }
+}