[Cronet] Public key pinning for Java API

components/cronet/android/api/src/org/chromium/net/CronetEngine.java

Index: components/cronet/android/api/src/org/chromium/net/CronetEngine.java
diff --git a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
index 6e7d1e0940ccc2fa4668cc24373cd866e90866ea..ccd01adc68124359169146cdbc913466cfba873e 100644
--- a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
+++ b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
@@ -6,6 +6,7 @@ package org.chromium.net;
 
 import android.content.Context;
 import android.support.annotation.IntDef;
+import android.util.Base64;
 import android.util.Log;
 
 import org.json.JSONArray;
@@ -16,13 +17,19 @@ import java.io.File;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.reflect.Constructor;
+import java.net.IDN;
 import java.net.Proxy;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
+import java.util.Collection;
+import java.util.Date;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.Executor;
+import java.util.regex.Pattern;
 
 /**
  * An engine to process {@link UrlRequest}s, which uses the best HTTP stack
@@ -35,6 +42,8 @@ public abstract class CronetEngine {
      * then {@link #build} is called to create the {@code CronetEngine}.
      */
     public static class Builder {
+        private static final Pattern INVALID_PKP_HOST_NAME = Pattern.compile("^[0-9\\.]*$");
+
         private final JSONObject mConfig;
         private final Context mContext;
 
@@ -303,6 +312,113 @@ public abstract class CronetEngine {
         }
 
         /**
+         * Adds public key pins for a given host.

[pauljensen, 2015/12/01 20:50:12]

Do we allow intermediate certificates signed by the pinned certificate?

[pauljensen, 2015/12/01 20:50:12]

I think we need to make this a lot more verbose.  I had no idea what a "public
key pin" was until recently.  Maybe something along the lines of:

Pins a set of public keys for a given host.  By pinning a set of public keys,
{@code pinsSha256}, communication with {@code hostName} is required to
authenticate with a certificate with a public key from the set of pinned ones. 
Authentication will fail and secure communication will not be established
otherwise, even if the host attempts to authenticate with a certificate allowed
by the device's trusted store of certificates.

[pauljensen, 2015/12/01 20:50:12]

This would probably be a good place to include a link to the relevant RFC in
case embedders want to read more about how it works.

[kapishnikov, 2015/12/02 22:11:36]

Yes, we do allow intermediate certificates signed by the pinned certificate.
That is according to the spec. This Wikipedia page
(https://en.wikipedia.org/wiki/HTTP_Public_Key_Pinning) nicely summarizes RFC
7469 Section 4 - Security Considerations: "A website operator can choose to
either pin the root certificate public key of a particular root certificate
authority, allowing only that certificate authority (and all intermediate
authorities signed by its key) to issue valid certificates for the website's
domain, and/or to pin the key(s) of one or more intermediate issuing
certificates, or to pin the end-entity public key."

What we don't mandate is having the backup public key, i.e. a key that does not
present in the server's certificate chain. When a user agent gets an HTTP
response from a server that contains pinning request, it should verify that
there is the backup key. Since Cronet is a programmatic API that is called
before any response from the server is received, we cannot check that the user
supplied a backup key to addPublicKeyPins() method. 

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         *
+         * @param hostName name of the host to which the public keys should be pinned. A host that
+         *                 consists of digits and the dot character only is treated as invalid.

[pauljensen, 2015/12/01 20:50:13]

nit: of digits and the dot character only->of only digits and the dot character

[pauljensen, 2015/12/01 20:50:13]

Actually I think we might want to get rid of this sentence as it's a little
unexplained, you could replace it with something saying IP addresses are not
allowed.  At a minimum we should explain why they're explicitly unallowed.

[kapishnikov, 2015/12/02 22:11:36]

Done.

[kapishnikov, 2015/12/02 22:11:36]

The problem here is that the API not only restricts IP addresses but also some
valid host names that are interpreted as IPv4 addresses by the native
implementation. E.g. host name "2130706433" is perfectly valid according to RFC
3986 Section 3.2.2. however it will be translated to 127.0.0.1 IPv4 address by
the native code. If we don't want to overload the user API with internal
implementation details, it should be reasonable just to state this limitation
without going into details. I assume that in practice the host names will
contain a top-level-domain that cannot consist of digits, therefore, this
limitation should not have significant impact.

+         * @param pinsSha256 a collection of pins. Each pin is the SHA-256 cryptographic
+         *                   hash of DER-encoded ASN.1 representation of Subject Public

[pauljensen, 2015/12/01 20:50:12]

of->of the

[kapishnikov, 2015/12/02 22:11:36]

Done. I also added comments regarding the backup key:
"Although, the method does not mandate the presence of the backup pin that can
be used if the control of the primary private key has been lost, it is highly
recommended to supply one".

+         *                   Key Info (SPKI) of the host X.509 certificate. Use

[pauljensen, 2015/12/01 20:50:13]

host->host's

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         *                   {@link java.security.cert.Certificate#getPublicKey()
+         *                   Certificate.getPublicKey} and

[pauljensen, 2015/12/01 20:50:13]

}->()}

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         *                   {@link java.security.Key#getEncoded() Key.getEncoded}

[pauljensen, 2015/12/01 20:50:12]

ditto

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         *                   to obtain DER-encoded ASN.1 representation of SPKI.

[pauljensen, 2015/12/01 20:50:12]

SPKI->the SPKI

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         * @param includeSubdomains indicates whether the pinning policy should be applied to
+         *                          subdomains of {@code hostName}.
+         * @param expirationDate specifies the expiration date for the pins.

[pauljensen, 2015/12/01 20:50:12]

What happens when they expire?  do we fall back to allowing certs that the
device's trusted store allows?

[kapishnikov, 2015/12/02 22:11:36]

Yes, that is correct. When the pins expire, the client stops validating them.
The device's trusted store is always used regardless whether the a host is
pinned or not. Pinning is the extra layer of security on top of the trust store.

+         * @return the builder to facilitate chaining.
+         * @throws NullPointerException if one of the input parameters is null.

[pauljensen, 2015/12/01 20:50:12]

null->{@code null}

[pauljensen, 2015/12/01 20:50:13]

one->any
is->are

[kapishnikov, 2015/12/02 22:11:36]

Done.

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         * @throws IllegalArgumentException if the given host name is invalid or the

[pauljensen, 2015/12/01 20:50:12]

or the->or

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         *                                  {@code pinsSha256} collection contains a byte array

[pauljensen, 2015/12/01 20:50:12]

collection contains->contains

[kapishnikov, 2015/12/02 22:11:36]

Done.

+         *                                  that does not represent a valid SHA-256 hash.
+         */
+        public Builder addPublicKeyPins(String hostName, Collection<byte[]> pinsSha256,
+                boolean includeSubdomains, Date expirationDate) {
+            if (hostName == null) {
+                throw new NullPointerException("The hostname cannot be null");
+            }
+            if (pinsSha256 == null) {
+                throw new NullPointerException("The collection of SHA256 pins cannot be null");
+            }

[pauljensen, 2015/12/01 20:50:12]

null check expirationDate also, and test

[kapishnikov, 2015/12/02 22:11:36]

Done. Added tests to check the exception when any of the parameters is null. 

+            String idnHostName = validateHostNameForPinningAndConvert(hostName);
+            try {
+                // Add PKP_LIST JSON array element if it is not present.
+                JSONArray pkpList = mConfig.optJSONArray(CronetEngineBuilderList.PKP_LIST);
+                if (pkpList == null) {
+                    pkpList = new JSONArray();
+                    mConfig.put(CronetEngineBuilderList.PKP_LIST, pkpList);
+                }
+
+                // Convert the pin to BASE64 encoding.
+                Set<String> hashes = new HashSet<>(pinsSha256.size());

[pauljensen, 2015/12/01 20:50:13]

I assume you're using HashSet to eliminate duplicates?  Might want to add a
comment mentioning this implicit action.

[kapishnikov, 2015/12/02 22:11:36]

Good point. From the client point of view it does not really matter if we remove
duplicate pins or now. Pinning a host to the same pin second time doesn't change
anything. However, we remove duplicates as the optimization step. I agree that
we should indicate to the client that there should be no duplicates; therefore,
I have changed the signature of the method to accept a set instead of the
collection. I think it is semantically more correct even though in HTTP PKP the
pins are provided in a form of a list.

Also added the following related statement to the method: "Calling this method
multiple times with the same host name overrides the previously set pins for the
host."

+                for (byte[] pinSha256 : pinsSha256) {
+                    hashes.add(convertSha256ToBase64WithPrefix(pinSha256));
+                }
+
+                // Add new element to PKP_LIST JSON array.
+                JSONObject pkp = new JSONObject();
+                pkp.put(CronetEngineBuilderList.PKP_HOST, idnHostName);
+                pkp.put(CronetEngineBuilderList.PKP_PIN_HASHES, new JSONArray(hashes));
+                pkp.put(CronetEngineBuilderList.PKP_INCLUDE_SUBDOMAINS, includeSubdomains);
+                // The expiration time is passed as a double, in seconds since January 1, 1970.
+                pkp.put(CronetEngineBuilderList.PKP_EXPIRATION_DATE,
+                        (double) expirationDate.getTime() / 1000);
+                pkpList.put(pkp);
+            } catch (JSONException e) {
+                // This exception should never happen.
+                throw new RuntimeException(
+                        "Failed to add pubic key pins with the given arguments", e);
+            }
+            return this;
+        }
+
+        /**
+         * Converts a given SHA256 array of bytes to BASE64 encoded string and prepends
+         * {@code sha256/} prefix to it. The format corresponds to the format that is expected by
+         * {@code net::HashValue} class.
+         *
+         * @param sha256 SHA256 bytes to convert to BASE64.
+         * @return the BASE64 encoded SHA256 with the prefix.
+         * @throws IllegalArgumentException if the provided pin is invalid.
+         */
+        private static String convertSha256ToBase64WithPrefix(byte[] sha256) {
+            if (sha256 == null || sha256.length != 32) {
+                throw new IllegalArgumentException("Public key pin is invalid");
+            }
+            return "sha256/" + Base64.encodeToString(sha256, Base64.NO_WRAP);
+        }
+
+        /**
+         * Checks whether a given string represents a valid host name for PKP and converts it
+         * to ASCII Compatible Encoding representation according to RFC 1122, RFC 1123 and
+         * RFC 3490. This method is more restrictive than required by RFC 7469. Thus, a host
+         * that contains digits and the dot character only is considered invalid.
+         *
+         * Note: Currently Cronet doesn't have native implementation of host name validation that
+         *       can be used. There is code that parses a provided URL but doesn't ensure its
+         *       correctness. The implementation relies on {@code getaddrinfo} function.
+         *
+         * @see <a href="https://tools.ietf.org/html/rfc7469#section-2.3.3>RFC 7469</a>
+         * @param hostName host name to check and convert.
+         * @return true if the string is a valid host name.
+         * @throws IllegalArgumentException if the the given string does not represent a valid
+         *                                  hostname.
+         */
+        private static String validateHostNameForPinningAndConvert(String hostName)
+                throws IllegalArgumentException {
+            if (INVALID_PKP_HOST_NAME.matcher(hostName).matches()) {
+                throw new IllegalArgumentException("Hostname " + hostName + " is illegal."
+                        + " A hostname should not consist of digits and/or dots only.");
+            }
+            try {
+                return IDN.toASCII(hostName, IDN.USE_STD3_ASCII_RULES);
+            } catch (IllegalArgumentException ex) {
+                throw new IllegalArgumentException("Hostname " + hostName + " is illegal."
+                        + " The name of the host does not comply with RFC 1122 and RFC 1123.");
+            }
+        }
+
+        /**
          * Sets experimental options to be used in Cronet.
          *
          * @param options JSON formatted experimental options.