Reject invalid IPv4 addresses in ipv4_from_asc

The old scanf-based parser accepted all kinds of invalid inputs like:
"1.2.3.4.5"
"1.2.3.4 "
"1.2.3. 4"
" 1.2.3.4"
"1.2.3.4."
"1.2.3.+4"
"1.2.3.4.example.test"
"1.2.3.01"
"1.2.3.0x1"

Thanks to Amir Mohamadi for pointing this out in
https://boringssl-review.googlesource.com/c/boringssl/+/68167. This is a
different implementation since patching sscanf doesn't quite catch all
the cases. Add a bunch of tests, some imported from Amr's patch to
OpenSSL upstream, plus a bunch of my own. (IPv6 parsing is complicated!)

Update-Note: The deprecated (and dangerous) string-based APIs for
configuring X.509 extensions will no longer silently misinterpret some
invalid inputs as IPv4 addresses. This was run through TGP internally
without any issue.

Change-Id: I66e223a466cc3e74df9f9ddc8aef3b6b6c790f7e
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/68567
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Auto-Submit: David Benjamin <davidben@google.com>

2 files changed, 160 insertions, 16 deletions

diff --git a/crypto/x509/v3_utl.c b/crypto/x509/v3_utl.c
index c6144d1..4426be5 100644
--- a/crypto/x509/v3_utl.c
+++ b/crypto/x509/v3_utl.c
@@ -82,10 +82,10 @@ static void str_free(OPENSSL_STRING str);
 static int append_ia5(STACK_OF(OPENSSL_STRING) **sk,
                       const ASN1_IA5STRING *email);
 
-static int ipv4_from_asc(unsigned char v4[4], const char *in);
-static int ipv6_from_asc(unsigned char v6[16], const char *in);
+static int ipv4_from_asc(uint8_t v4[4], const char *in);
+static int ipv6_from_asc(uint8_t v6[16], const char *in);
 static int ipv6_cb(const char *elem, size_t len, void *usr);
-static int ipv6_hex(unsigned char *out, const char *in, size_t inlen);
+static int ipv6_hex(uint8_t *out, const char *in, size_t inlen);
 
 // Add a CONF_VALUE name value pair to stack
 
@@ -1154,7 +1154,7 @@ err:
   return NULL;
 }
 
-int x509v3_a2i_ipadd(unsigned char ipout[16], const char *ipasc) {
+int x509v3_a2i_ipadd(uint8_t ipout[16], const char *ipasc) {
   // If string contains a ':' assume IPv6
 
   if (strchr(ipasc, ':')) {
@@ -1170,25 +1170,58 @@ int x509v3_a2i_ipadd(unsigned char ipout[16], const char *ipasc) {
   }
 }
 
-static int ipv4_from_asc(unsigned char v4[4], const char *in) {
-  int a0, a1, a2, a3;
-  if (sscanf(in, "%d.%d.%d.%d", &a0, &a1, &a2, &a3) != 4) {
+// get_ipv4_component consumes one IPv4 component, terminated by either '.' or
+// the end of the string, from |*str|. On success, it returns one, sets |*out|
+// to the component, and advances |*str| to the first unconsumed character. On
+// invalid input, it returns zero.
+static int get_ipv4_component(uint8_t *out_byte, const char **str) {
+  // Store a slightly larger intermediary so the overflow check is easier.
+  uint32_t out = 0;
+  for (;;) {
+    if (!OPENSSL_isdigit(**str)) {
+      return 0;
+    }
+    out = (out * 10) + (**str - '0');
+    if (out > 255) {
+      // Components must be 8-bit.
+      return 0;
+    }
+    (*str)++;
+    if ((**str) == '.' || (**str) == '\0') {
+      *out_byte = (uint8_t)out;
+      return 1;
+    }
+    if (out == 0) {
+      // Reject extra leading zeros. Parsers sometimes treat them as octal, so
+      // accepting them would misinterpret input.
+      return 0;
+    }
+  }
+}
+
+// get_ipv4_dot consumes a '.' from |*str| and advances it. It returns one on
+// success and zero if |*str| does not point to a '.'.
+static int get_ipv4_dot(const char **str) {
+  if (**str != '.') {
     return 0;
   }
-  if ((a0 < 0) || (a0 > 255) || (a1 < 0) || (a1 > 255) || (a2 < 0) ||
-      (a2 > 255) || (a3 < 0) || (a3 > 255)) {
+  (*str)++;
+  return 1;
+}
+
+static int ipv4_from_asc(uint8_t v4[4], const char *in) {
+  if (!get_ipv4_component(&v4[0], &in) || !get_ipv4_dot(&in) ||
+      !get_ipv4_component(&v4[1], &in) || !get_ipv4_dot(&in) ||
+      !get_ipv4_component(&v4[2], &in) || !get_ipv4_dot(&in) ||
+      !get_ipv4_component(&v4[3], &in) || *in != '\0') {
     return 0;
   }
-  v4[0] = a0;
-  v4[1] = a1;
-  v4[2] = a2;
-  v4[3] = a3;
   return 1;
 }
 
 typedef struct {
   // Temporary store for IPV6 output
-  unsigned char tmp[16];
+  uint8_t tmp[16];
   // Total number of bytes in tmp
   int total;
   // The position of a zero (corresponding to '::')
@@ -1197,7 +1230,7 @@ typedef struct {
   int zero_cnt;
 } IPV6_STAT;
 
-static int ipv6_from_asc(unsigned char v6[16], const char *in) {
+static int ipv6_from_asc(uint8_t v6[16], const char *in) {
   IPV6_STAT v6stat;
   v6stat.total = 0;
   v6stat.zero_pos = -1;
@@ -1305,7 +1338,7 @@ static int ipv6_cb(const char *elem, size_t len, void *usr) {
 
 // Convert a string of up to 4 hex digits into the corresponding IPv6 form.
 
-static int ipv6_hex(unsigned char *out, const char *in, size_t inlen) {
+static int ipv6_hex(uint8_t *out, const char *in, size_t inlen) {
   if (inlen > 4) {
     return 0;
   }
diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc
index ab107c7..c22a918 100644
--- a/crypto/x509/x509_test.cc
+++ b/crypto/x509/x509_test.cc
@@ -8545,3 +8545,114 @@ TEST(X509Test, DuplicateName) {
     }
   }
 }
+
+TEST(X509Test, ParseIPAddress) {
+  const struct {
+    const char *inp;
+    // out is the expected output, or an empty vector if the parser is expected
+    // to fail.
+    std::vector<uint8_t> out;
+  } kIPTests[] = {
+      // Valid IPv4 addresses.
+      {"127.0.0.1", {127, 0, 0, 1}},
+      {"1.2.3.4", {1, 2, 3, 4}},
+      {"1.2.3.255", {1, 2, 3, 255}},
+      {"255.255.255.255", {255, 255, 255, 255}},
+
+      // Valid IPv6 addresses
+      {"::", {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}},
+      {"::1", {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}},
+      {"::01", {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}},
+      {"::001", {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}},
+      {"::0001", {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1}},
+      {"ffff::", {0xff, 0xff, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0}},
+      {"1::2", {0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2}},
+      {"1:1:1:1:1:1:1:1", {0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1, 0, 1}},
+      {"2001:db8::ff00:42:8329",
+       {0x20, 0x01, 0x0d, 0xb8, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0x00,
+        0x00, 0x42, 0x83, 0x29}},
+      {"1234::1.2.3.4", {0x12, 0x34, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 3, 4}},
+      {"::1.2.3.4", {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 2, 3, 4}},
+      {"ffff:ffff:ffff:ffff:ffff:ffff:1.2.3.4",
+       {0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+        1, 2, 3, 4}},
+
+      // Too few IPv4 components.
+      {"1", {}},
+      {"1.", {}},
+      {"1.2", {}},
+      {"1.2.", {}},
+      {"1.2.3", {}},
+      {"1.2.3.", {}},
+
+      // Invalid embedded IPv4 address.
+      {"::1.2.3", {}},
+
+      // Too many components.
+      {"1.2.3.4.5", {}},
+      {"1:2:3:4:5:6:7:8:9", {}},
+      {"1:2:3:4:5::6:7:8:9", {}},
+
+      // IPv4 literals take the place of two IPv6 components.
+      {"1:2:3:4:5:6:7:1.2.3.4", {}},
+
+      // '::' should have fewer than 16 components or it is redundant.
+      {"1:2:3:4:5:6:7::8", {}},
+
+      // Embedded IPv4 addresses must be at the end.
+      {"::1.2.3.4:1", {}},
+
+      // Stray whitespace or other invalid characters.
+      {"1.2.3.4 ", {}},
+      {"1.2.3 .4", {}},
+      {"1.2.3. 4", {}},
+      {" 1.2.3.4", {}},
+      {"1.2.3.4.", {}},
+      {"1.2.3.+4", {}},
+      {"1.2.3.-4", {}},
+      {"1.2.3.4.example.test", {}},
+      {"::1 ", {}},
+      {" ::1", {}},
+      {":: 1", {}},
+      {": :1", {}},
+      {"1.2.3.nope", {}},
+      {"::nope", {}},
+
+      // Components too large.
+      {"1.2.3.256", {}},  // Overflows when adding
+      {"1.2.3.260", {}},  // Overflows when multiplying by 10
+      {"1.2.3.999999999999999999999999999999999999999999", {}},
+      {"::fffff", {}},
+
+      // Although not an overflow, more than four hex digits is an error.
+      {"::00000", {}},
+
+      // Too many colons.
+      {":::", {}},
+      {"1:::", {}},
+      {":::2", {}},
+      {"1:::2", {}},
+
+      // Only one group of zeros may be elided.
+      {"1::2::3", {}},
+
+      // We only support decimal.
+      {"1.2.3.01", {}},
+      {"1.2.3.0x1", {}},
+
+      // Random garbage.
+      {"example.test", {}},
+      {"", {}},
+  };
+  for (const auto &t : kIPTests) {
+    SCOPED_TRACE(t.inp);
+    bssl::UniquePtr<ASN1_OCTET_STRING> oct(a2i_IPADDRESS(t.inp));
+    if (t.out.empty()) {
+      EXPECT_FALSE(oct);
+    } else {
+      ASSERT_TRUE(oct);
+      EXPECT_EQ(Bytes(t.out), Bytes(ASN1_STRING_get0_data(oct.get()),
+                                    ASN1_STRING_length(oct.get())));
+    }
+  }
+}