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.support.annotation.IntDef;
+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.. */

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

How about: "Minimum end-of-central-directory size."

The constant is for the minimum not the maximum

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

Done.

+    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 "webapk:0000:<hexvalues>". This pattern can appear anywher in the comment but
+     * must be separated from any other parts with a separator that doesn't look like a hex
+     * character.

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

Cool! The signature checking code has a "singing function".

anywher -> anywhere

"with a separator that doesn't look like a hex character." -> "non hex
character."

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

Done.

+     */
+    private static final Pattern WEBAPK_COMMENT_PATTERN =
+            Pattern.compile("webapk:(\\d+):([a-fA-F0-9]+)");

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

Can you remove the brackets around \\d+ (You don't need to extract the "key id"
yet)

Thank you. The pattern is a lot simpler now

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

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

Nit: "." at the end of the sentence (Here and in other comments in this file)

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

Done.

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

Done.

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

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

"strings" -> "string."

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

Done.

+    private String mComment;
+
+    /** Errors codes */
+    @IntDef({
+            ERROR_OK, ERROR_BAD_APK, ERROR_EXTRA_FIELD_TOO_LARGE, ERROR_COMMENT_TOO_LARGE,
+            ERROR_INCORRECT_SIGNATURE, ERROR_SIGNATURE_NOT_FOUND, ERROR_TOO_MANY_META_INF_FILES,
+    })
+    public @interface Error {}
+    public static final int ERROR_OK = 0;
+    public static final int ERROR_BAD_APK = 1;
+    public static final int ERROR_EXTRA_FIELD_TOO_LARGE = 2;
+    public static final int ERROR_COMMENT_TOO_LARGE = 3;
+    public static final int ERROR_INCORRECT_SIGNATURE = 4;
+    public static final int ERROR_SIGNATURE_NOT_FOUND = 5;
+    public static final int ERROR_TOO_MANY_META_INF_FILES = 6;

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

Nit: public static constants should be declared first

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

Moved to the top.

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

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

How about: Block contains meta data about a zip entry.

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

Done.

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

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

Nit: added -> Added

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

Done.

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

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

Nit: New line

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

Done.

+        @Override
+        public int hashCode() {
+            return mFilename.hashCode();
+        }
+
+        String mFilename;
+        int mPosition;
+        int mHeaderSize;
+        int mCompressedSize;

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

Member variables should come before functions.

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

Getting confused with C++ :-)

+    }
+
+    /** CTOR simply 'connects' to buffer passed. */

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

CTOR -> Constructor

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

Done.

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

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

Nit: remove new line (here and in other function comments)

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

Done.

+     * @return OK on success.
+     */
+    public int read() {
+        try {
+            @Error

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

I haven't seen other places which annotate ints which are enums in this way. (I
have seen annotations in method parameters but nowhere else)

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

Removed.

+            int err = readEOCD();
+            if (err != ERROR_OK) {
+                Log.d(TAG, "Missing EOCD Signature");

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

Log is unnecessary because calling function will log error code

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

Removed.

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

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

Log is unnecessary because calling function will log error code

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

Removed.

+                return err;
+            }
+        } catch (Exception e) {
+            Log.e(TAG, "Error reading directory", e);
+            return ERROR_BAD_APK;
+        }
+        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 ERROR_OK if the public key signature verifies.
+     * @throws SignatureException for various reasons.

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

This function should not throw an exception. If an exception is thrown, the
function should return an error code

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

Done.

+     */
+    public int 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
+            int 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/31 03:59:25]

catch (Exception e) {} instead for the sake of sanity

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

Done.

+            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

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

calculateHash -> calculateHash()

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

Done.

+     * cryptographic hash.
+     *
+     * @param sig Signature object you can call update on.
+     */
+    public int calculateHash(Signature sig)
+            throws SignatureException, IllegalArgumentException, UnsupportedEncodingException {

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

This function should throw Exception only for the sake of sanity

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

Done.

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

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

Remove 'as well'

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

Done.

+            filename = block.mFilename.getBytes("UTF-8");

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

- Is it worth storing mFilename as a String? Can it be stored as an array of
bytes instead?
  You can use Arrays#hashCode() to get the hash of the byte array in
Block#hashCode()
  readString() would become byte[] readBytes(int numBytes)
- You should be consistent. If you are passing the charset to String#getBytes()
you should also pass the charset to String#String() in readString().

- Might as well move the declaration of the |filename| variable to here.
byte[] filename = ...

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

It sounds like a good idea but ended up having some repercussions that made it
not worthwhile, I think:
* There's no compareTo() for byte arrays that I could find.
* There's no startsWith(), although the binary search could work.
* For the regular expression search for webapk:0000: etc. we'd have to convert
it to a String.

Instead I just used getBytes() instead of UTF-8 specifically.

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

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

Might as well move the declaration of the |slice| variable to here.

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

Moved.

+            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

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

How about: "The signature should have the format webapk:0000:<hexvalues>.
Currently ..."

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

Done.

+     * id which is always "0000".

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

Nit: Add quotes around "key id"

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

Done.

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

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

Can you remove this conditional?

- It should be impossible for |s| to be empty.
- If |s| is empty, verifySignature() will fail anyways

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

Good point.

+            return null;
+        }
+        return hexToBytes(s);
+    }
+
+    /**
+     * Reads the End of Central Directory Records, returns false if it can't find it.

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

Records -> Record

Remove the part of the comment with "returns false if ...". You are already
documenting this in the @return part of the comment

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

Done.

+     *
+     * @return ERROR_OK on success.
+     */
+    private int readEOCD() {
+        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

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

seekDelta(4)

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

Oops missed one,thanks.

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

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

How about: Reads the central directory and populates {@link mBlocks} with data
about each entry.

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

Done.

+     *
+     * @return ERROR_OK on success.
+     */
+    @Error
+    int 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();
+            if (extraFieldLength > MAX_EXTRA_LENGTH) {
+                Log.w(TAG, String.format("Extra field too large: %d bytes", extraFieldLength));
+                return ERROR_EXTRA_FIELD_TOO_LARGE;
+            }
+
+            // 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() {
+        int offset = mBuffer.limit() - MIN_EOCD_SIZE;

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

Nit: You need to handle |offset| being smaller than 0 (Though I guess you would
get an exception when you do the seek.)

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

My unit tests do cover this, however being explicit it probably good.

+        seek(offset);
+        for (int count = 0; count < MAX_HEADER_SIZE; count++) {

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

Can you do this:

int minSearchOffset = Math.max(0, offset - MAX_HEADER_SIZE);
for (; offset >= minSearchOffset; offset--) {
  seek(offset);
  ...
}

This enables merging the seek on line 355 and 365

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

Done, much cleaner.

+            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) {
+        mBuffer.position(offset);
+    }
+
+    /**
+     * Skip forward this number of bytes.
+     *
+     * @param delta number of bytes to seek forward.
+     */
+    private void seekDelta(int delta) {
+        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.");

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

Can we remove this log message?

We are already logging ERROR_SIGNATURE_NOT_FOUND

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

Done.

+            return null;
+        }
+        if (len == 0) {

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

Can we remove this conditional? (For the reasons mentioned in
parseCommentSignature())

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

Done.

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