Fix range_should_be_prefix() to actually return the correct result

range_should_be_prefix() was misidentifying whether an IP address range
should in fact be represented as a prefix. This was due to a bug introduced
in commit 42d7d7dd which made this incorrect change:

-    OPENSSL_assert(memcmp(min, max, length) <= 0);
+    if (memcmp(min, max, length) <= 0)
+        return -1;

This error leads to incorrect DER being encoded/accepted.

Reported by Theo Buehler (@botovq)

Reviewed-by: Paul Dale <pauli@openssl.org>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/18524)

(cherry picked from commit 30532e59f475e0066c030693e4d614311a9e0cae)

2 files changed, 123 insertions, 2 deletions

diff --git a/crypto/x509/v3_addr.c b/crypto/x509/v3_addr.c
index 83752cd4b9..4205e7d7af 100644
--- a/crypto/x509/v3_addr.c
+++ b/crypto/x509/v3_addr.c
@@ -13,6 +13,8 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <assert.h>
+#include <string.h>
 
 #include "internal/cryptlib.h"
 #include <openssl/conf.h>
@@ -343,8 +345,13 @@ static int range_should_be_prefix(const unsigned char *min,
     unsigned char mask;
     int i, j;
 
-    if (memcmp(min, max, length) <= 0)
-        return -1;
+    /*
+     * It is the responsibility of the caller to confirm min <= max. We don't
+     * use ossl_assert() here since we have no way of signalling an error from
+     * this function - so we just use a plain assert instead.
+     */
+    assert(memcmp(min, max, length) <= 0);
+
     for (i = 0; i < length && min[i] == max[i]; i++) ;
     for (j = length - 1; j >= 0 && min[j] == 0x00 && max[j] == 0xFF; j--) ;
     if (i < j)
@@ -427,6 +434,9 @@ static int make_addressRange(IPAddressOrRange **result,
     IPAddressOrRange *aor;
     int i, prefixlen;
 
+    if (memcmp(min, max, length) > 0)
+        return 0;
+
     if ((prefixlen = range_should_be_prefix(min, max, length)) >= 0)
         return make_addressPrefix(result, min, prefixlen);
 
diff --git a/test/v3ext.c b/test/v3ext.c
index a2adb1a9f0..98bd060f67 100644
--- a/test/v3ext.c
+++ b/test/v3ext.c
@@ -12,6 +12,7 @@
 #include <openssl/x509v3.h>
 #include <openssl/pem.h>
 #include <openssl/err.h>
+#include "internal/nelem.h"
 
 #include "testutil.h"
 
@@ -114,6 +115,115 @@ static int test_asid(void)
     ASIdentifiers_free(asid4);
     return testresult;
 }
+
+static struct ip_ranges_st {
+    const unsigned int afi;
+    const char *ip1;
+    const char *ip2;
+    int rorp;
+} ranges[] = {
+    { IANA_AFI_IPV4, "192.168.0.0", "192.168.0.1", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV4, "192.168.0.0", "192.168.0.2", IPAddressOrRange_addressRange},
+    { IANA_AFI_IPV4, "192.168.0.0", "192.168.0.3", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV4, "192.168.0.0", "192.168.0.254", IPAddressOrRange_addressRange},
+    { IANA_AFI_IPV4, "192.168.0.0", "192.168.0.255", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV4, "192.168.0.1", "192.168.0.255", IPAddressOrRange_addressRange},
+    { IANA_AFI_IPV4, "192.168.0.1", "192.168.0.1", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV4, "192.168.0.0", "192.168.255.255", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV4, "192.168.1.0", "192.168.255.255", IPAddressOrRange_addressRange},
+    { IANA_AFI_IPV6, "2001:0db8::0", "2001:0db8::1", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV6, "2001:0db8::0", "2001:0db8::2", IPAddressOrRange_addressRange},
+    { IANA_AFI_IPV6, "2001:0db8::0", "2001:0db8::3", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV6, "2001:0db8::0", "2001:0db8::fffe", IPAddressOrRange_addressRange},
+    { IANA_AFI_IPV6, "2001:0db8::0", "2001:0db8::ffff", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV6, "2001:0db8::1", "2001:0db8::ffff", IPAddressOrRange_addressRange},
+    { IANA_AFI_IPV6, "2001:0db8::1", "2001:0db8::1", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV6, "2001:0db8::0:0", "2001:0db8::ffff:ffff", IPAddressOrRange_addressPrefix},
+    { IANA_AFI_IPV6, "2001:0db8::1:0", "2001:0db8::ffff:ffff", IPAddressOrRange_addressRange}
+};
+
+static int check_addr(IPAddrBlocks *addr, int type)
+{
+    IPAddressFamily *fam;
+    IPAddressOrRange *aorr;
+
+    if (!TEST_int_eq(sk_IPAddressFamily_num(addr), 1))
+        return 0;
+
+    fam = sk_IPAddressFamily_value(addr, 0);
+    if (!TEST_ptr(fam))
+        return 0;
+
+    if (!TEST_int_eq(fam->ipAddressChoice->type, IPAddressChoice_addressesOrRanges))
+        return 0;
+
+    if (!TEST_int_eq(sk_IPAddressOrRange_num(fam->ipAddressChoice->u.addressesOrRanges), 1))
+        return 0;
+
+    aorr = sk_IPAddressOrRange_value(fam->ipAddressChoice->u.addressesOrRanges, 0);
+    if (!TEST_ptr(aorr))
+        return 0;
+
+    if (!TEST_int_eq(aorr->type, type))
+        return 0;
+
+    return 1;
+}
+
+static int test_addr_ranges(void)
+{
+    IPAddrBlocks *addr = NULL;
+    ASN1_OCTET_STRING *ip1 = NULL, *ip2 = NULL;
+    size_t i;
+    int testresult = 0;
+
+    for (i = 0; i < OSSL_NELEM(ranges); i++) {
+        addr = sk_IPAddressFamily_new_null();
+        if (!TEST_ptr(addr))
+            goto end;
+        /*
+         * Has the side effect of installing the comparison function onto the
+         * stack.
+         */
+        if (!TEST_true(X509v3_addr_canonize(addr)))
+            goto end;
+
+        ip1 = a2i_IPADDRESS(ranges[i].ip1);
+        if (!TEST_ptr(ip1))
+            goto end;
+        if (!TEST_true(ip1->length == 4 || ip1->length == 16))
+            goto end;
+        ip2 = a2i_IPADDRESS(ranges[i].ip2);
+        if (!TEST_ptr(ip2))
+            goto end;
+        if (!TEST_int_eq(ip2->length, ip1->length))
+            goto end;
+        if (!TEST_true(memcmp(ip1->data, ip2->data, ip1->length) <= 0))
+            goto end;
+
+        if (!TEST_true(X509v3_addr_add_range(addr, ranges[i].afi, NULL, ip1->data, ip2->data)))
+            goto end;
+
+        if (!TEST_true(X509v3_addr_is_canonical(addr)))
+            goto end;
+
+        if (!check_addr(addr, ranges[i].rorp))
+            goto end;
+
+        sk_IPAddressFamily_pop_free(addr, IPAddressFamily_free);
+        addr = NULL;
+        ASN1_OCTET_STRING_free(ip1);
+        ASN1_OCTET_STRING_free(ip2);
+        ip1 = ip2 = NULL;
+    }
+
+    testresult = 1;
+ end:
+    sk_IPAddressFamily_pop_free(addr, IPAddressFamily_free);
+    ASN1_OCTET_STRING_free(ip1);
+    ASN1_OCTET_STRING_free(ip2);
+    return testresult;
+}
 #endif /* OPENSSL_NO_RFC3779 */
 
 OPT_TEST_DECLARE_USAGE("cert.pem\n")
@@ -131,6 +241,7 @@ int setup_tests(void)
     ADD_TEST(test_pathlen);
 #ifndef OPENSSL_NO_RFC3779
     ADD_TEST(test_asid);
+    ADD_TEST(test_addr_ranges);
 #endif /* OPENSSL_NO_RFC3779 */
     return 1;
 }