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..e99fcd4e45843ee31db403413558849a65108842
--- /dev/null
+++ b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkVerifySignature.java
@@ -0,0 +1,405 @@
+// 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 {

[pkotwicz, 2017/03/26 01:37:04]

Can you split this class into two. One class which does the generic ZIP file
reading (reading the central directory) and one which does the verification and
signature computation.

ZipCentralDirectoryReader perhaps with two methods

String ZipCentralDirectoryReader#getFileComment()
ArrayList<ZipCentralDirectoryReader.Block>
ZipCentralDirectoryReader#readEntries()

[ScottK, 2017/03/27 20:27:56]

I don't think it's that useful and am concerned someone might take the code to
be a 'general' zip handling class, when it really is only useful for this one
thing.
I can still split if if you feel strongly about it.

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

I will defer to dominickn@ on this point

+    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;
+
+    /** 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 */
+    private static final Pattern WEBAPK_COMMENT_PATTERN =
+            Pattern.compile("webapk:\\s*(\\d[^:]+:\\s*)?([a-fA-F0-9]*)");

[pkotwicz, 2017/03/26 01:37:05]

Nit: webapk -> chrome-webapk

Is there a place which describes the format of the signature? In particular what
is the purpose of the characters before the second ':' is. It looks like
WebApkVerifySignature.java ignores those characters

It looks like this code assumes that the signature is at the end of the file
comment. If this is the case you can probably allow the signature to have
unicode values between 0-255 and simplify hexToBytes()

We should also document that we assume that the signature is at the end of the
file comment

[ScottK, 2017/03/27 20:27:56]

I'm not sure if the "chrome-" part really makes sense, we may need to change
this in the WAM server in the future, especially for x-browser. I think for now
we should just search for "webapk:".
You are right that I haven't updated our documentation (todo). The first number
is the "algorithm version" and the second set of digits is the "key id". This
was suggested by the security folks. 
Internally, b/34620500 is the bug for the crypto id. 
Actually, the code does allow for the signature to be anywhere in the comment. I
prefer to use hex as some byte sequences aren't valid UTF-8 and many zip
utilites don't support non-ascii characters very well.
There isn't really an assumption about where the signature is, but I've updated
the pattern to be more clear? and added a few more tests.

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

I'm not sure if the "chrome-" part really makes sense, we may need to change
this in the WAM server in the future, especially for x-browser. I think for now
we should just search for "webapk:".

Ok, for the sake of future proofing, let's make this webapk: instead of
chrome-webapk (Let's update the other comments in this file appropriately)

You are right that I haven't updated our documentation (todo). The first number
is the "algorithm version" and the second set of digits is the "key id". This
was suggested by the security folks. 
Internally, b/34620500 is the bug for the crypto id.

According to the bug, the format is
chrome-webapk:<algorithm_version>:<key_id>:<bytes>
The matcher seems to be for the format:
chrome-webapk:<algorithm_version>:<bytes>

There isn't really an assumption about where the signature is, but I've updated
the pattern to be more clear? and added a few more tests.

If there is a hex value after the signature, won't the matcher assume that the
hex value is part of the signature too?

Can we make the format requirement stricter:
- disallow whitespace between "algorithm version" and "hash"
- make "algorithm version" block non optional

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

I've simplified the regex and am not forcing the key id, which is a number.

The bug did mention having <algorithm version>:<key id>:<bytes>, but since the
algorithm version was a 1 digit number we just put it altogether as one big four
digit identifier.

Since we control what we put in the comment it think it's ok to allow the bytes
to trail off and assume there will be a separator character (added some more
documentation).

The chrome- prefix might make sense if we have a different private key used for
chrome vs other browsers. However, with the desire supporting x-browsers I agree
with you we should make it general and not have the chrome- prefix.

+
+    /** 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,
+    }
+
+    /**
+     * 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);
+            calculateHash(signature);
+            return signature.verify(sig) ? Error.OK : Error.INCORRECT_SIGNATURE;
+        } catch (InvalidKeyException | NoSuchAlgorithmException | SignatureException 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 void calculateHash(Signature sig) throws SignatureException, IllegalArgumentException {
+        for (Block block : mBlocks) {
+            seek(block.mPosition + block.mHeaderSize);
+            byte[] bytes = new byte[block.mCompressedSize];

[pkotwicz, 2017/03/26 01:37:05]

You might be allocating an array which is many kilobytes.

- Should you call Signature#update() several times to avoid copying so much
data?

- Can you avoid the copy:
  - Does ByteBuffer#hasArray() return true?
  - If it doesn't you can probably use ByteBuffer#slice() combined with
ByteBuffer#position() and ByteBuffer#limit() 

[ScottK, 2017/03/27 20:27:56]

A good optimization, thanks.

+            mBuffer.get(bytes);
+            try {
+                byte[] filename = block.mFilename.getBytes("UTF-8");
+                sig.update(toUInt32BigEndian(filename.length));

[pkotwicz, 2017/03/26 01:37:04]

Can you document why you are saving the length?

[ScottK, 2017/03/27 20:27:56]

internally b/34716053 discussed the reasons why.
Added comments.

+                sig.update(filename);
+            } catch (UnsupportedEncodingException e) {
+                Log.wtf(TAG, "Don't know UTF-8", e);
+                continue;
+            }
+            sig.update(toUInt32BigEndian(bytes.length));
+            sig.update(bytes);
+        }
+    }
+
+    /**
+     * 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);

[pkotwicz, 2017/03/26 01:37:04]

I think that BIG_ENDIAN is the default for ByteBuffer.

(Just double checking that you are calling ByteBuffer#order() for clarity)

[ScottK, 2017/03/27 20:27:56]

Added a comment.

+        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;
+            }
+            // Signature(4), CreatorVersion(2), ReaderVersion(2), Flags(2), Method(2)

[pkotwicz, 2017/03/26 01:37:05]

Nit: Method(2) -> CompressionMethod(2)

[ScottK, 2017/03/27 20:27:56]

done.

+            // ModifiedTime(2), ModifiedDate(2), CRC32(4) = 20 bytes
+            curr += 20;
+            seek(curr);

[pkotwicz, 2017/03/26 01:37:04]

It would be useful to have a seekDelta() method which does

mBuffer.position(mBuffer.position() + delta)

You can do this instead of the reads on lines 279 - 281

[ScottK, 2017/03/27 20:27:57]

Thanks! Done.

+            int compressedSize = read4();
+            read4(); // uncompressed size
+            int fileNameLength = read2();
+            int extraLen = read2();
+            int fileCommentLength = read2();
+            read2(); // DiskNumberStart
+            read2(); // Internal Attrs
+            read4(); // External Attrs
+            int offset = read4();
+            String filename = readString(fileNameLength);
+            curr = mBuffer.position() + extraLen + fileCommentLength;

[pkotwicz, 2017/03/26 01:37:05]

I would store the extra size and the comment size as a field in the Block
object.

This way you separate the reading of the zip central directory (and whether this
is a valid zip file) from checking our custom requirements for the zip file

[ScottK, 2017/03/27 20:27:56]

I believe the reason I did this is that the the file comment size and extra size
is often 0 here yet when decoding the block it not zero, ugh.

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

Thank you for the explanation. I think that we should check the extra and
comment length in either both the "central directory record" and the "local file
header" or in neither

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

Ping on this comment

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

I fixed this earlier, guess I didn't publish in time?

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

Your are correct, I should check also in the local file header in case they lied
earlier - 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;
+            }
+            if (filename.startsWith("META-INF/")) {
+                // Files that begin with META-INF/ are skipped.
+                continue;

[pkotwicz, 2017/03/26 01:37:04]

This logic belongs in calculateHash()

[ScottK, 2017/03/27 20:27:56]

agreed, done.

+            }
+            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;
+            }
+            // Skip Signature(4), ReaderVersion(2), Flags(2), Method(2),
+            // ModifiedTime (2), ModifiedDate(2), CRC32(4), CompressedSize(4),
+            // UncompressedSize(4) = 26 bytes
+            curr += 26;
+            seek(curr);

[pkotwicz, 2017/03/26 01:37:04]

seekDelta() would be useful here too

[ScottK, 2017/03/27 20:27:56]

done.

+            int fileNameLength = read2();
+            int extraFieldLength = read2();

[pkotwicz, 2017/03/26 01:37:05]

If we care about checking the size of the extra data shouldn't we check that the
extra data length is the same as the length specified in the central directory
entry?

[ScottK, 2017/03/27 20:27:56]

It often isn't. I think some zip implementations are lazy and just put zeros in
the central directory.

+            // 26 + 4 = 30 bytes
+            block.mHeaderSize = 30 + fileNameLength + extraFieldLength;
+        }
+        Collections.sort(mBlocks);

[pkotwicz, 2017/03/26 01:37:04]

Can you move the sorting of the blocks to computeHash()?

[ScottK, 2017/03/27 20:27:56]

Agreed, done.

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

[pkotwicz, 2017/03/26 01:37:05]

Nit: No new line

[ScottK, 2017/03/27 20:27:56]

done.

+     * @return value read.
+     */
+    private int read4() {
+        return mBuffer.getInt();
+    }
+
+    /** Read `length` many bytes into a string */

[pkotwicz, 2017/03/26 01:37:05]

'length' -> {@link length}

[ScottK, 2017/03/27 20:27:56]

Done.

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

[pkotwicz, 2017/03/26 01:37:05]

Can you make your comment more specific. In particular, why is String#getBytes()
not good enough?

[ScottK, 2017/03/27 20:27:56]

Added a comment.

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