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.

diff --git a/src/address.c b/src/address.c
index 41bdb4c7b5f6eec9c5449d53a5d31433c4ef6a0f..0bcbe1ca6781dbe463998138bb9991e340217d1d 100644 (file)
--- a/src/address.c
+++ b/src/address.c
@@ -2,8 +2,21 @@
 
 #include <string.h>
 #include <errno.h>
+#include <arpa/inet.h> /* 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 d4417712f56c56f164b103c081585ffdfe05cbf3..dc44fd62200de8f54b1855313d71193ebec21864 100644 (file)
--- 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 50d0eafb36e42812d751ce096e38e3a0b925035f..4361cc57f7fe0b7a18e540c61e5cc94b8dda2277 100644 (file)
--- 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 d120cd1ce4da45912c6f221193378bdd36d8afb6..0248f5749672187053e09c1439071ac62fdeb6cd 100644 (file)
--- 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 7bf8555b2f4bcedb9871fb31e8467f6441a88fa6..83ba33f0c544784f8d5de42a5760c0888a60182f 100644 (file)
--- a/src/resource.c
+++ b/src/resource.c
@@ -2,6 +2,7 @@
 
 #include <errno.h>
 #include <arpa/inet.h>
+#include <stdint.h> /* 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);