[Cronet] Public key pinning for Java API

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

+// Copyright 2015 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.net;
+
+import java.net.IDN;
+import java.util.regex.Pattern;
+
+/**
+ * A set of generic utility methods.
+ */
+class CronetUtil {
+    // Expression that defines valid IPv4 decimal number in range [0, 255].
+    private static final String VALID_IP_NUMBER =
+            "([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])";
+    // Expression that defines valid IPv4 address, which is the sequence of four
+    // |VALID_IP_NUMBER| numbers separated by '.'.
+    private static final String VALID_IP_EXPR =
+            "^(" + VALID_IP_NUMBER + "\\.){3}" + VALID_IP_NUMBER + "$";
+    private static final Pattern VALID_IP_PATTERN = Pattern.compile(VALID_IP_EXPR);
+
+    private CronetUtil() {}
+
+    /**
+     * Checks whether a given string that represents a host name is valid. The method
+     * does not verify the length of the host name labels, the total length of

[nharper, 2015/11/19 23:47:54]

documentation nit: RFC 3490 section 4.1 states that ToASCII (as defined to
operate on a label) checks that the number of code points is in the range 1 to
63 inclusive. Does the IDN toASCII method not do this check?

[kapishnikov, 2015/11/20 16:38:03]

You are right. I have changed the comments and added tests to verify the length
of the host labels and the total length of the hostname.

+     * the host name and the validity of the top level domain.
+     *
+     * 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.
+     *
+     * @param hostName host name to check.
+     * @return true if the string is a valid host name.
+     */
+    static boolean isValidHostName(String hostName) {
+        try {
+            IDN.toASCII(hostName, IDN.USE_STD3_ASCII_RULES);
+        } catch (IllegalArgumentException ex) {
+            // The hostname is illegal according to RFC 1122 and RFC 1123.
+            return false;
+        }
+        return true;
+    }
+
+    /**
+     * Checks whether a given string that represents an IPv4 address is valid.
+     *
+     * @param addr IPv4 address to check.
+     * @return true if the string is a valid IPv4 address.
+     */
+    static boolean isValidIPv4(String addr) {
+        return VALID_IP_PATTERN.matcher(addr).matches();

[nharper, 2015/11/19 23:47:54]

Chrome interprets plenty of things that don't match that regex as IPv4
addresses. The following all resolve to 127.0.0.1:

2130706433
0177.0.0.1
127.0.00.1

Instead of reimplementing the isValidHostName and isValidIPv4 checks here, can
we reuse the code that already exists?

[kapishnikov, 2015/11/20 16:38:03]

To validate the host name, isValidHostName() method is using IDN class that was
in Android since API 9.

Regarding IPv4 validation. RFC 3986 Section 3.2.2. Host says:


   A host identified by an IPv4 literal address is represented in
   dotted-decimal notation (a sequence of four decimal numbers in the
   range 0 to 255, separated by "."), as described in [RFC1123] by
   reference to [RFC0952].  Note that other forms of dotted notation may
   be interpreted on some platforms, as described in Section 7.4, but
   only the dotted-decimal form of four octets is allowed by this
   grammar.

      IPv4address = dec-octet "." dec-octet "." dec-octet "." dec-octet

      dec-octet   = DIGIT                 ; 0-9
                  / %x31-39 DIGIT         ; 10-99
                  / "1" 2DIGIT            ; 100-199
                  / "2" %x30-34 DIGIT     ; 200-249
                  / "25" %x30-35          ; 250-255

isValidIPv4() method verifies that the address satisfies the above mentioned
grammar. Unfortunately, I couldn't find any existing implementation that does
it. I have added comments to the method to emphasize the required format.

[nharper, 2015/11/20 19:02:47]

(This comment really belongs on isValidHostNameForPinning in CronetEngine.java.)
My concern here isn't that isValidIPv4 doesn't do what it's name says (I believe
that this method correctly checks whether the provided string matches
IPv4address as defined in RFC 3986). Instead, my concern is that the combination
of isValidHostname && !isValidIPv4 will let through something that isn't a valid
IP address, but can be interpreted as one. The existing native code uses
https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&l=443&...
to check if the host of the url for a request is an IP address, which allows for
all sorts of awful things not in RFC 3986.

Even if we accept an IP address-like hostname, it won't be used for pinning
because 1) it's not canonicalized, and 2) (more importantly) a request coming in
for an IP will never be checked against the pin set. isValidHostNameForPinning
should fail so that addPublicKeyPins will throw an exception if an IP
address-like hostname is passed in. Maybe the IP address check should be relaxed
to something like ^[0-9.]*$ for hostnames to reject? That regex is very ad-hoc,
but I can't think of anything that would match it that could actually be a valid
domain (unless ICANN starts making gTLDs that consist of only digits).

[kapishnikov, 2015/11/20 20:40:09]

It is a good point. I agree that we should make the hostname validation more
restrictive since we know that IPv4-like hostnames are not going to work (even
if they are perfectly valid according to RFC 3986). I will fix it and add more
tests that reject host names like '2130706433', '0177.0.0.1', etc. I guess the
suggested ^[0-9.]*$ expression should be good.

[kapishnikov, 2015/11/23 16:48:45]

Done with corresponding tests in PkpTest.java. I have deleted CronetUtil and
CronetUtilTest classes since they are not needed anymore.

+    }
+}