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.nio.ByteBuffer;
+import java.security.PublicKey;
+import java.security.Signature;
+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 {
+    /** 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;
+
+    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;
+
+    /** Minimum 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;

[pkotwicz, 2017/04/05 02:54:28]

Nit: This is likely more readable as:
512 * 1024

Please also mention the units. Is this the maximum size in bytes?

My understanding is that the maximum header side is 2Mb

[ScottK, 2017/04/05 20:14:27]

Done.

[pkotwicz, 2017/04/06 17:58:28]

What are the units?

[ScottK, 2017/04/07 21:57:08]

Done, I've also shortened to 64k which is more in line with other zip apps seek
for plus our WebAPKs don't have many files so it should never get even close to
that big.
The reason I used the larger one is I did find a zip implementation that would
search further for the end-of-central-directory, but I think it just makes it
slower for files that aren't apks, so keeping it short.
Also changing the name to be a little easier to understand it's purpose.

+
+    /** 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 signing key.
+     * An example is "webapk:0000:<hexvalues>". This pattern can appear anywhere
+     * in the comment but must be separated from any other parts with a
+     * separator that doesn't look like a hex character.
+     */
+    private static final Pattern WEBAPK_COMMENT_PATTERN =
+            Pattern.compile("webapk:\\d+:([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 string. */
+    private String mComment;
+
+    /**
+     * 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 contains metadata about a zip entry. */
+    private static class Block implements Comparable<Block> {
+        String mFilename;
+        int mPosition;
+        int mHeaderSize;
+        int mCompressedSize;
+
+        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();
+        }
+    }
+
+    /** Constructor 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 int read() {
+        try {
+            int err = readEOCD();
+            if (err != ERROR_OK) {
+                return err;
+            }
+            // Short circuit if no comment found.
+            if (parseCommentSignature(mComment) == null) {
+                return ERROR_SIGNATURE_NOT_FOUND;
+            }
+            err = readDirectory();
+            if (err != ERROR_OK) {
+                return err;
+            }
+        } catch (Exception 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.
+     */
+    public int verifySignature(PublicKey pub) {
+        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

[pkotwicz, 2017/04/05 02:54:28]

Please remove the annotation

[ScottK, 2017/04/05 20:14:27]

Done.

+            int err = calculateHash(signature);
+            if (err != ERROR_OK) {
+                return err;
+            }
+            return signature.verify(sig) ? ERROR_OK : ERROR_INCORRECT_SIGNATURE;
+        } catch (Exception e) {
+            Log.e(TAG, "Exception calculating signature", e);
+            return ERROR_INCORRECT_SIGNATURE;
+        }
+    }
+
+    /** @return Full APK/zip comment string. */
+    public String comment() {

[pkotwicz, 2017/04/05 02:54:28]

This method looks unused. 💣 it

[ScottK, 2017/04/05 20:14:28]

Done.

+        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 int calculateHash(Signature sig) throws Exception {
+        Collections.sort(mBlocks);
+        int metaInfCount = 0;
+        for (Block block : mBlocks) {
+            if (block.mFilename.indexOf("META-INF/") == 0) {
+                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 to prevent Horton principle violation.
+            byte[] filename = block.mFilename.getBytes();
+            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);
+            ByteBuffer 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/04/05 02:54:28]

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

[ScottK, 2017/04/05 20:14:27]

Done.

+        ByteBuffer buffer = ByteBuffer.allocate(4);
+        buffer.order(BIG_ENDIAN); // Since the ZIP is all little endian, adding this for clarity.

[pkotwicz, 2017/04/05 02:54:29]

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

[ScottK, 2017/04/05 20:14:27]

It matches the documentation and what the server does.
It's a bit too much work to be worthwhile now.

[pkotwicz, 2017/04/06 17:58:29]

It matches which documentation?

[ScottK, 2017/04/07 21:57:08]

go/webapk-comment-sig

[pkotwicz, 2017/04/10 14:43:58]

Implementation time (i.e. now) is the perfect time to change this to make things
less confusing

[ScottK, 2017/04/13 12:04:00]

Done.

+        buffer.putInt(value);
+        return buffer.array();
+    }
+
+    /**
+     * Extract the bytes of the signature from the comment. We expect
+     * "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(1);
+        return hexToBytes(s);
+    }
+
+    /**
+     * Reads the End of Central Directory Record.
+     * @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
+        seekDelta(4); // Size of central directory
+        mCentralDirOffset = read4(); // as bytes from start of file.
+        int commentLength = read2();
+        mComment = readString(commentLength);
+        return ERROR_OK;
+    }
+
+    /**
+     * Reads the central directory and populates {@link mBlocks} with data about each entry.
+     * @return ERROR_OK on success.
+     */
+    @Error
+    int readDirectory() {
+        mBlocks = new ArrayList<>(mRecordCount);
+        seek(mCentralDirOffset);
+        for (int i = 0; i < mRecordCount; i++) {
+            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);
+            seekDelta(extraLen + fileCommentLength);
+            if (extraLen > MAX_EXTRA_LENGTH) {
+                Log.w(TAG,
+                        String.format(
+                                "Extra field too large for file %s: %d bytes", filename, extraLen));

[pkotwicz, 2017/04/05 02:54:28]

Nit: You can remove this log. The log with the error code is printed in
verifyCommentSignedWebApk()

[ScottK, 2017/04/05 20:14:28]

Done.

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

[pkotwicz, 2017/04/05 02:54:28]

Nit: You can remove this log. The log with the error code is printed in
verifyCommentSignedWebApk()

[ScottK, 2017/04/05 20:14:27]

Done.

+                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) {
+            seek(block.mPosition);
+            int signature = read4();
+            if (signature != LFH_SIG) {
+                Log.d(TAG, "LFH Signature missing");
+                return ERROR_BAD_APK;
+            }
+            // ReaderVersion(2), Flags(2), Method(2),

[pkotwicz, 2017/04/05 02:54:28]

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

[ScottK, 2017/04/05 20:14:27]

Done.

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

[pkotwicz, 2017/04/05 02:54:28]

Nit: You can remove this log. The log with the error code is printed in
verifyCommentSignedWebApk()

[ScottK, 2017/04/05 20:14:28]

Done.

+                return ERROR_EXTRA_FIELD_TOO_LARGE;
+            }
+
+            block.mHeaderSize =
+                    (mBuffer.position() - block.mPosition) + 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;
+        int minSearchOffset = Math.max(0, offset - MAX_HEADER_SIZE);
+        for (; offset >= minSearchOffset; offset--) {
+            if (offset < 0) {
+                return -1;
+            }

[pkotwicz, 2017/04/05 02:54:28]

Nit: We can remove this if() statement now

[ScottK, 2017/04/05 20:14:27]

Done.

+            seek(offset);
+            if (read4() == EOCD_SIG) {
+                // found!
+                return 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) {
+            // Odd number of nibbles.
+            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;
+    }
+}