From 30532e59f475e0066c030693e4d614311a9e0cae Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Fri, 10 Jun 2022 15:58:58 +0100 Subject: [PATCH] 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 Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/18524) --- crypto/x509/v3_addr.c | 14 +++++- test/v3ext.c | 111 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 2 deletions(-) diff --git a/crypto/x509/v3_addr.c b/crypto/x509/v3_addr.c index feefb9c3df..c3ccecb2eb 100644 --- a/crypto/x509/v3_addr.c +++ b/crypto/x509/v3_addr.c @@ -13,6 +13,8 @@ #include #include +#include +#include #include "internal/cryptlib.h" #include @@ -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 29e33d16a5..926f3884b1 100644 --- a/test/v3ext.c +++ b/test/v3ext.c @@ -12,6 +12,7 @@ #include #include #include +#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; }