From 3e8eeb9e74d412bd87f1d03e0aaee49724eef2e3 Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Thu, 7 Feb 2019 15:47:42 -0600 Subject: [PATCH] Code review - Print relevant IP address on address errors - Add missing resource validations (swapped ranges, AS number out of bounds) - Remove validation of ROA's AS number. The RFCs never state that the number must be present in the EE certificate. --- src/address.c | 92 ++++++++++++++++++++++++++++++++++++---- src/main.c | 1 + src/object/certificate.c | 4 +- src/object/roa.c | 26 +++++------- src/resource.c | 68 +++++++++++++++++++---------- 5 files changed, 143 insertions(+), 48 deletions(-) diff --git a/src/address.c b/src/address.c index 41bdb4c7..0bcbe1ca 100644 --- a/src/address.c +++ b/src/address.c @@ -2,8 +2,21 @@ #include #include +#include /* inet_ntop */ #include "log.h" +static char const * +addr2str4(struct in_addr *addr, char *buffer) +{ + return inet_ntop(AF_INET, addr, buffer, INET_ADDRSTRLEN); +} + +static char const * +addr2str6(struct in6_addr *addr, char *buffer) +{ + return inet_ntop(AF_INET6, addr, buffer, INET6_ADDRSTRLEN); +} + /** * Returns a mask you can use to extract the suffix bits of an address whose * prefix lengths @prefix_len. @@ -40,6 +53,7 @@ int prefix4_decode(IPAddress2_t *str, struct ipv4_prefix *result) { int len; + char buffer[INET_ADDRSTRLEN]; if (str->size > 4) { return pr_err("IPv4 address has too many octets. (%u)", @@ -61,8 +75,10 @@ prefix4_decode(IPAddress2_t *str, struct ipv4_prefix *result) result->len = len; - if ((result->addr.s_addr & ipv4_suffix_mask(result->len)) != 0) - return pr_err("IPv4 prefix has enabled suffix bits."); + if ((result->addr.s_addr & ipv4_suffix_mask(result->len)) != 0) { + return pr_err("IPv4 prefix %s/%u has enabled suffix bits.", + addr2str4(&result->addr, buffer), result->len); + } return 0; } @@ -72,6 +88,7 @@ prefix6_decode(IPAddress2_t *str, struct ipv6_prefix *result) { struct in6_addr suffix; int len; + char buffer[INET6_ADDRSTRLEN]; if (str->size > 16) { return pr_err("IPv6 address has too many octets. (%u)", @@ -98,8 +115,25 @@ prefix6_decode(IPAddress2_t *str, struct ipv6_prefix *result) if ( (result->addr.s6_addr32[0] & suffix.s6_addr32[0]) || (result->addr.s6_addr32[1] & suffix.s6_addr32[1]) || (result->addr.s6_addr32[2] & suffix.s6_addr32[2]) - || (result->addr.s6_addr32[3] & suffix.s6_addr32[3])) - return pr_err("IPv6 prefix has enabled suffix bits."); + || (result->addr.s6_addr32[3] & suffix.s6_addr32[3])) { + return pr_err("IPv6 prefix %s/%u has enabled suffix bits.", + addr2str6(&result->addr, buffer), result->len); + } + + return 0; +} + +static int +check_order4(struct ipv4_range *result) +{ + char buffer_min[INET_ADDRSTRLEN]; + char buffer_max[INET_ADDRSTRLEN]; + + if (ntohl(result->min.s_addr) > ntohl(result->max.s_addr)) { + return pr_err("The IPv4 range %s-%s is inverted.", + addr2str4(&result->min, buffer_min), + addr2str4(&result->max, buffer_max)); + } return 0; } @@ -115,6 +149,8 @@ check_encoding4(struct ipv4_range *range) const uint32_t MIN = ntohl(range->min.s_addr); const uint32_t MAX = ntohl(range->max.s_addr); uint32_t mask; + char buffer_min[INET_ADDRSTRLEN]; + char buffer_max[INET_ADDRSTRLEN]; for (mask = 0x80000000u; mask != 0; mask >>= 1) if ((MIN & mask) != (MAX & mask)) @@ -124,7 +160,9 @@ check_encoding4(struct ipv4_range *range) if (((MIN & mask) != 0) || ((MAX & mask) == 0)) return 0; - return pr_err("IPv4 address is a range, but should have been encoded as a prefix."); + return pr_err("IPAddressRange %s-%s is a range, but should have been encoded as a prefix.", + addr2str4(&range->min, buffer_min), + addr2str4(&range->min, buffer_max)); } int @@ -143,13 +181,45 @@ range4_decode(IPAddressRange_t *input, struct ipv4_range *result) return error; result->max.s_addr = prefix.addr.s_addr | ipv4_suffix_mask(prefix.len); + error = check_order4(result); + if (error) + return error; + return check_encoding4(result); } static int -pr_bad_encoding(void) +check_order6(struct ipv6_range *result) +{ + uint32_t min; + uint32_t max; + unsigned int quadrant; + char buffer_min[INET6_ADDRSTRLEN]; + char buffer_max[INET6_ADDRSTRLEN]; + + for (quadrant = 0; quadrant < 4; quadrant++) { + min = ntohl(result->min.s6_addr32[quadrant]); + max = ntohl(result->max.s6_addr32[quadrant]); + if (min > max) { + return pr_err("The IPv6 range %s-%s is inverted.", + addr2str6(&result->min, buffer_min), + addr2str6(&result->max, buffer_max)); + } else if (min < max) { + return 0; /* result->min < result->max */ + } + } + + return 0; /* result->min == result->max */ +} + +static int +pr_bad_encoding(struct ipv6_range *range) { - return pr_err("IPv6 address is a range, but should have been encoded as a prefix."); + char buffer_min[INET6_ADDRSTRLEN]; + char buffer_max[INET6_ADDRSTRLEN]; + return pr_err("IPAddressRange %s-%s is a range, but should have been encoded as a prefix.", + addr2str6(&range->min, buffer_min), + addr2str6(&range->max, buffer_max)); } static int @@ -167,7 +237,7 @@ thingy(struct ipv6_range *range, unsigned int quadrant, uint32_t mask) mask = 0x80000000u; } - return pr_bad_encoding(); + return pr_bad_encoding(range); } static int @@ -186,7 +256,7 @@ check_encoding6(struct ipv6_range *range) return thingy(range, quadrant, mask); } - return pr_bad_encoding(); + return pr_bad_encoding(range); } int @@ -206,5 +276,9 @@ range6_decode(IPAddressRange_t *input, struct ipv6_range *result) result->max = prefix.addr; ipv6_suffix_mask(prefix.len, &result->max); + error = check_order6(result); + if (error) + return error; + return check_encoding6(result); } diff --git a/src/main.c b/src/main.c index d4417712..dc44fd62 100644 --- a/src/main.c +++ b/src/main.c @@ -41,6 +41,7 @@ add_rpki_oids(void) "Certificate Policy (CP) for the Resource PKI (RPKI)"); printf("certPolicyRpki registered. Its nid is %d.\n", NID_certPolicyRpki); + /* TODO implement RFC 8360 */ NID_certPolicyRpkiV2 = OBJ_create("1.3.6.1.5.5.7.14.3", "id-cp-ipAddr-asNumber-v2 (RFC 8360)", "Certificate Policy for Use with Validation Reconsidered in the RPKI"); diff --git a/src/object/certificate.c b/src/object/certificate.c index 50d0eafb..4361cc57 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -156,7 +156,7 @@ validate_name(X509_NAME *name, char *what) } #ifdef DEBUG - str = calloc(str_len + 1, 1); + str = calloc(str_len + 1, sizeof(char)); if (str == NULL) { pr_err("Out of memory."); return -ENOMEM; @@ -187,7 +187,7 @@ validate_subject(X509_NAME *name, char *what) return -EINVAL; sub_len = X509_NAME_get_text_by_NID(name, NID_commonName, NULL, 0); - subject = calloc(sub_len + 1, 1); + subject = calloc(sub_len + 1, sizeof(char)); if (subject == NULL) { pr_err("Out of memory."); return -ENOMEM; diff --git a/src/object/roa.c b/src/object/roa.c index d120cd1c..0248f574 100644 --- a/src/object/roa.c +++ b/src/object/roa.c @@ -114,9 +114,17 @@ print_addr6(struct resources *parent, unsigned long asn, } static int -print_addr(struct resources *parent, unsigned long asn, uint8_t family, +print_addr(struct resources *parent, ASID_t *as_id, uint8_t family, struct ROAIPAddress *roa_addr) { + unsigned long asn; + + if (asn_INTEGER2ulong(as_id, &asn) != 0) { + if (errno) + pr_errno(errno, "Error casting ROA's AS ID value"); + return pr_err("ROA's AS ID couldn't be parsed as unsigned long"); + } + switch (family) { case 1: /* IPv4 */ return print_addr4(parent, asn, roa_addr); @@ -131,7 +139,7 @@ static int __handle_roa(struct RouteOriginAttestation *roa, struct resources *parent) { struct ROAIPAddressFamily *block; - unsigned long as_id, version; + unsigned long version; int b; int a; int error; @@ -148,18 +156,6 @@ __handle_roa(struct RouteOriginAttestation *roa, struct resources *parent) return pr_err("ROA's version (%lu) is nonzero.", version); } - error = asn_INTEGER2ulong(&roa->asID, &as_id); - if (error) { - if (errno) - pr_errno(errno, "Error casting ROA's AS ID value"); - return pr_err("ROA's AS ID couldn't be parsed as unsigned long"); - } - /* rfc6482#section-3.2 (more or less.) */ - if (!resources_contains_asn(parent, as_id)) { - return pr_err("ROA is not allowed to attest for AS %lu", - as_id); - } - /* rfc6482#section-3.3 */ if (roa->ipAddrBlocks.list.array == NULL) @@ -181,7 +177,7 @@ __handle_roa(struct RouteOriginAttestation *roa, struct resources *parent) if (block->addresses.list.array == NULL) return pr_err("ROA's address list array is NULL."); for (a = 0; a < block->addresses.list.count; a++) { - error = print_addr(parent, as_id, + error = print_addr(parent, &roa->asID, block->addressFamily.buf[1], block->addresses.list.array[a]); if (error) diff --git a/src/resource.c b/src/resource.c index 7bf8555b..83ba33f0 100644 --- a/src/resource.c +++ b/src/resource.c @@ -2,6 +2,7 @@ #include #include +#include /* UINT32_MAX */ #include "address.h" #include "log.h" @@ -247,7 +248,7 @@ add_range4(struct resources *resources, IPAddressRange_t *input) parent = get_parent_resources(); - if ((parent != NULL) && (resources->ip4s == parent->ip4s)) + if (parent && (resources->ip4s == parent->ip4s)) return pr_err("Certificate defines IPv4 ranges while also inheriting his parent's."); error = range4_decode(input, &range); @@ -329,7 +330,7 @@ add_aors(struct resources *resources, int family, { struct IPAddressOrRange *aor; int i; - int error = 0; + int error; for (i = 0; i < aors->list.count; i++) { aor = aors->list.array[i]; @@ -401,25 +402,36 @@ inherit_asiors(struct resources *resources) } static int -add_asn(struct resources *resources, ASId_t min, ASId_t max, - struct resources *parent) +ASId2ulong(ASId_t *as_id, unsigned long *result) { - unsigned long asn_min, asn_max; + static const unsigned long ASN_MAX = UINT32_MAX; int error; - error = asn_INTEGER2ulong(&min, &asn_min); + error = asn_INTEGER2ulong(as_id, result); if (error) { if (errno) - pr_errno(errno, "Error converting ASN min value"); - return pr_err("Added ASN min value isn't a valid unsigned long"); + pr_errno(errno, "Error converting ASN value"); + return pr_err("ASN value is not a valid unsigned long"); } - error = asn_INTEGER2ulong(&max, &asn_max); - if (error) { - if (errno) - pr_errno(errno, "Error converting ASN max value"); - return pr_err("Added ASN max value isn't a valid unsigned long"); + + if ((*result) > ASN_MAX) { + return pr_err("ASN value '%lu' is out of bounds. (0-%lu)", + *result, ASN_MAX); } - if (parent && !rasn_contains(parent->asns, asn_min, asn_max)) + + return 0; +} + +static int +add_asn(struct resources *resources, unsigned long min, unsigned long max, + struct resources *parent) +{ + int error; + + if (min > max) + return pr_err("The ASN range %lu-%lu is inverted.", min, max); + + if (parent && !rasn_contains(parent->asns, min, max)) return pr_err("Parent certificate doesn't own child's ASN resource."); if (resources->asns == NULL) { @@ -428,17 +440,17 @@ add_asn(struct resources *resources, ASId_t min, ASId_t max, return pr_enomem(); } - error = rasn_add(resources->asns, asn_min, asn_max); + error = rasn_add(resources->asns, min, max); if (error){ pr_err("Error adding ASN range to certificate resources: %s", sarray_err2str(error)); return error; } - if (asn_min == asn_max) - pr_debug("ASN: %lu", asn_min); + if (min == max) + pr_debug("ASN: %lu", min); else - pr_debug("ASN: %lu-%lu", asn_min, asn_max); + pr_debug("ASN: %lu-%lu", min, max); return 0; } @@ -446,6 +458,9 @@ static int add_asior(struct resources *resources, struct ASIdOrRange *obj) { struct resources *parent; + unsigned long asn_min; + unsigned long asn_max; + int error; parent = get_parent_resources(); @@ -455,12 +470,21 @@ add_asior(struct resources *resources, struct ASIdOrRange *obj) switch (obj->present) { case ASIdOrRange_PR_NOTHING: break; + case ASIdOrRange_PR_id: - return add_asn(resources, obj->choice.id, obj->choice.id, - parent); + error = ASId2ulong(&obj->choice.id, &asn_min); + if (error) + return error; + return add_asn(resources, asn_min, asn_min, parent); + case ASIdOrRange_PR_range: - return add_asn(resources, obj->choice.range.min, - obj->choice.range.max, parent); + error = ASId2ulong(&obj->choice.range.min, &asn_min); + if (error) + return error; + error = ASId2ulong(&obj->choice.range.max, &asn_max); + if (error) + return error; + return add_asn(resources, asn_min, asn_max, parent); } return pr_err("Unknown ASIdOrRange type: %d", obj->present); -- 2.47.3