stricter parser for ipv4_from_asc

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.

Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/24438)

diff --git a/crypto/x509/v3_utl.c b/crypto/x509/v3_utl.c
index a036e9e9ad8226847047aff79fbb2ac47c41fd50..0d956eec3ced607726bec3cde4391c9a76761d1f 100644 (file)
--- a/crypto/x509/v3_utl.c
+++ b/crypto/x509/v3_utl.c
@@ -1151,23 +1151,60 @@ int ossl_a2i_ipadd(unsigned char *ipout, const char *ipasc)
     }
 }
 
-static int ipv4_from_asc(unsigned char *v4, const char *in)
-{
-    const char *p;
-    int a0, a1, a2, a3, n;
+/*
+ * 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;
 
-    if (sscanf(in, "%d.%d.%d.%d%n", &a0, &a1, &a2, &a3, &n) != 4)
-        return 0;
-    if ((a0 < 0) || (a0 > 255) || (a1 < 0) || (a1 > 255)
-        || (a2 < 0) || (a2 > 255) || (a3 < 0) || (a3 > 255))
-        return 0;
-    p = in + n;
-    if (!(*p == '\0' || ossl_isspace(*p)))
+    for (;;) {
+        if (!ossl_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;
-    v4[0] = a0;
-    v4[1] = a1;
-    v4[2] = a2;
-    v4[3] = a3;
+    }
+    (*str)++;
+    return 1;
+}
+
+static int ipv4_from_asc(unsigned char *v4, 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;
+    }
     return 1;
 }
 

diff --git a/test/x509_internal_test.c b/test/x509_internal_test.c
index be43537329bbc5fc81d8c896f4111071c7639cef..6c2e2c3b085700eb295bc34d6ee0c21b3a607689 100644 (file)
--- a/test/x509_internal_test.c
+++ b/test/x509_internal_test.c
@@ -58,22 +58,92 @@ static IP_TESTDATA a2i_ipaddress_tests[] = {
     {"127.0.0.1", "\x7f\x00\x00\x01", 4},
     {"1.2.3.4", "\x01\x02\x03\x04", 4},
     {"1.2.3.255", "\x01\x02\x03\xff", 4},
-    {"1.2.3", NULL, 0},
-    {"1.2.3 .4", NULL, 0},
+    {"255.255.255.255", "\xff\xff\xff\xff", 4},
 
+    {"::", "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16},
     {"::1", "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", 16},
+    {"::01", "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", 16},
+    {"::0001", "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", 16},
+    {"ffff::", "\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00", 16},
+    {"ffff::1", "\xff\xff\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01", 16},
+    {"1::2", "\x00\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02", 16},
     {"1:1:1:1:1:1:1:1", "\x00\x01\x00\x01\x00\x01\x00\x01\x00\x01\x00\x01\x00\x01\x00\x01", 16},
     {"2001:db8::ff00:42:8329", "\x20\x01\x0d\xb8\x00\x00\x00\x00\x00\x00\xff\x00\x00\x42\x83\x29", 16},
+    {"::1.2.3.4", "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01\x02\x03\x04", 16},
+    {"ffff:ffff:ffff:ffff:ffff:ffff:1.2.3.4", "\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\xff\x01\x02\x03\x04", 16},
+
     {"1:1:1:1:1:1:1:1.test", NULL, 0},
     {":::1", NULL, 0},
     {"2001::123g", NULL, 0},
 
-    {"example.test", NULL, 0},
+    /* Too few IPv4 components. */
+    {"1", NULL, 0 },
+    {"1.", NULL, 0 },
+    {"1.2", NULL, 0 },
+    {"1.2.", NULL, 0 },
+    {"1.2.3", NULL, 0 },
+    {"1.2.3.", NULL, 0 },
+
+    /* Invalid embedded IPv4 address. */
+    {"::1.2.3", NULL, 0 },
+
+    /* IPv4 literals take the place of two IPv6 components. */
+    {"1:2:3:4:5:6:7:1.2.3.4", NULL, 0 },
+
+    /* '::' should have fewer than 16 components or it is redundant. */
+    {"1:2:3:4:5:6:7::8", NULL, 0 },
+
+    /* Embedded IPv4 addresses must be at the end. */
+    {"::1.2.3.4:1", NULL, 0 },
+
+    /* Too many components. */
+    {"1.2.3.4.5", NULL, 0 },
+    {"1:2:3:4:5:6:7:8:9", NULL, 0 },
+    {"1:2:3:4:5::6:7:8:9", NULL, 0 },
+
+    /* Stray whitespace or other invalid characters. */
+    {"1.2.3.4 ", NULL, 0 },
+    {"1.2.3 .4", NULL, 0 },
+    {"1.2.3. 4", NULL, 0 },
+    {" 1.2.3.4", NULL, 0 },
+    {"1.2.3.4.", NULL, 0 },
+    {"1.2.3.+4", NULL, 0 },
+    {"1.2.3.-4", NULL, 0 },
+    {"1.2.3.4.example.test", NULL, 0 },
+    {"::1 ", NULL, 0 },
+    {" ::1", NULL, 0 },
+    {":: 1", NULL, 0 },
+    {": :1", NULL, 0 },
+    {"1.2.3.nope", NULL, 0 },
+    {"::nope", NULL, 0 },
+
+    /* Components too large. */
+    {"1.2.3.256", NULL, 0},  /* Overflows when adding */
+    {"1.2.3.260", NULL, 0},  /* Overflows when multiplying by 10 */
+    {"1.2.3.999999999999999999999999999999999999999999", NULL, 0 },
+    {"::fffff", NULL, 0 },
+
+    /* Although not an overflow, more than four hex digits is an error. */
+    {"::00000", NULL, 0 },
+
+    /* Too many colons. */
+    {":::", NULL, 0 },
+    {"1:::", NULL, 0 },
+    {":::2", NULL, 0 },
+    {"1:::2", NULL, 0 },
+
+    /* Only one group of zeros may be elided. */
+    {"1::2::3", NULL, 0 },
+
+    /* We only support decimal. */
+    {"1.2.3.01", NULL, 0 },
+    {"1.2.3.0x1", NULL, 0 },
+
+    /* Random garbage. */
+    {"example.test", NULL, 0 },
     {"", NULL, 0},
-
-    {"1.2.3.4 ", "\x01\x02\x03\x04", 4},
-    {" 1.2.3.4", "\x01\x02\x03\x04", 4},
-    {" 1.2.3.4 ", "\x01\x02\x03\x04", 4},
+    {" 1.2.3.4", NULL, 0},
+    {" 1.2.3.4 ", NULL, 0},
     {"1.2.3.4.example.test", NULL, 0},
 };