Add validation of IP vs Range selection

RFC 3779, section 2.2.3.7.
Also patch memory leaks during AIA handling and other small TODOs.

diff --git a/src/address.c b/src/address.c
index 6aae8f423d9166667fd8ae26075550f2f486ae87..41bdb4c7b5f6eec9c5449d53a5d31433c4ef6a0f 100644 (file)
--- a/src/address.c
+++ b/src/address.c
@@ -104,6 +104,29 @@ prefix6_decode(IPAddress2_t *str, struct ipv6_prefix *result)
        return 0;
 }
 
+/**
+ * If @range could have been encoded as a prefix, this function errors.
+ *
+ * rfc3779#section-2.2.3.7
+ */
+static int
+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;
+
+       for (mask = 0x80000000u; mask != 0; mask >>= 1)
+               if ((MIN & mask) != (MAX & mask))
+                       break;
+
+       for (; mask != 0; mask >>= 1)
+               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.");
+}
+
 int
 range4_decode(IPAddressRange_t *input, struct ipv4_range *result)
 {
@@ -120,7 +143,50 @@ range4_decode(IPAddressRange_t *input, struct ipv4_range *result)
                return error;
        result->max.s_addr = prefix.addr.s_addr | ipv4_suffix_mask(prefix.len);
 
-       return 0;
+       return check_encoding4(result);
+}
+
+static int
+pr_bad_encoding(void)
+{
+       return pr_err("IPv6 address is a range, but should have been encoded as a prefix.");
+}
+
+static int
+thingy(struct ipv6_range *range, unsigned int quadrant, uint32_t mask)
+{
+       uint32_t min;
+       uint32_t max;
+
+       for (; quadrant < 4; quadrant++) {
+               min = ntohl(range->min.s6_addr32[quadrant]);
+               max = ntohl(range->max.s6_addr32[quadrant]);
+               for (; mask != 0; mask >>= 1)
+                       if (((min & mask) != 0) || ((max & mask) == 0))
+                               return 0;
+               mask = 0x80000000u;
+       }
+
+       return pr_bad_encoding();
+}
+
+static int
+check_encoding6(struct ipv6_range *range)
+{
+       uint32_t min;
+       uint32_t max;
+       unsigned int quadrant;
+       uint32_t mask;
+
+       for (quadrant = 0; quadrant < 4; quadrant++) {
+               min = ntohl(range->min.s6_addr32[quadrant]);
+               max = ntohl(range->max.s6_addr32[quadrant]);
+               for (mask = 0x80000000u; mask != 0; mask >>= 1)
+                       if ((min & mask) != (max & mask))
+                               return thingy(range, quadrant, mask);
+       }
+
+       return pr_bad_encoding();
 }
 
 int
@@ -140,5 +206,5 @@ range6_decode(IPAddressRange_t *input, struct ipv6_range *result)
        result->max = prefix.addr;
        ipv6_suffix_mask(prefix.len, &result->max);
 
-       return 0;
+       return check_encoding6(result);
 }

diff --git a/src/address.h b/src/address.h
index b117daafa866c66461f799935bbd5e80e255024e..cb5803a187e08cc36070fdafefead2c2065ede47 100644 (file)
--- a/src/address.h
+++ b/src/address.h
@@ -34,4 +34,7 @@ int prefix6_decode(IPAddress2_t *, struct ipv6_prefix *);
 int range4_decode(IPAddressRange_t *, struct ipv4_range *);
 int range6_decode(IPAddressRange_t *, struct ipv6_range *);
 
+int range4_check_encoding(struct ipv4_range *);
+int range6_check_encoding(struct ipv6_range *);
+
 #endif /* SRC_ADDRESS_H_ */

diff --git a/src/asn1/oid.c b/src/asn1/oid.c
index 0366c408d706e9d3361e7a9f63369b9184d9dce4..91cbc334562f831aedd7f3f1f1f47d03325550d0 100644 (file)
--- a/src/asn1/oid.c
+++ b/src/asn1/oid.c
@@ -22,6 +22,7 @@ oid2arcs(OBJECT_IDENTIFIER_t *oid, struct oid_arcs *result)
        static const size_t MAX_ARCS = 9;
        ssize_t count;
        ssize_t count2;
+       asn_oid_arc_t *tmp;
 
        result->arcs = malloc(MAX_ARCS * sizeof(asn_oid_arc_t));
        if (result->arcs == NULL)
@@ -38,10 +39,12 @@ oid2arcs(OBJECT_IDENTIFIER_t *oid, struct oid_arcs *result)
 
        /* If necessary, reallocate arcs array and try again. */
        if (count > MAX_ARCS) {
-               /* TODO realloc tmp */
-               result->arcs = realloc(result->arcs, count * sizeof(asn_oid_arc_t));
-               if (!result->arcs)
+               tmp = realloc(result->arcs, count * sizeof(asn_oid_arc_t));
+               if (tmp == NULL) {
+                       free(result->arcs);
                        return pr_enomem();
+               }
+               result->arcs = tmp;
 
                count2 = OBJECT_IDENTIFIER_get_arcs(oid, result->arcs, count);
                if (count != count2) {

diff --git a/src/log.c b/src/log.c
index 165d13cdca46a6874218d39ab0c7378cc14f24e4..727a3e56a69accefa24bcdc9df472a4f28bef9c0 100644 (file)
--- a/src/log.c
+++ b/src/log.c
@@ -27,8 +27,10 @@ pr_indent(FILE *stream)
 static void
 pr_file_name(FILE *stream)
 {
+#ifndef UNIT_TESTING
        char const *file = fnstack_peek();
        fprintf(stream, "%s: ", (file != NULL) ? file : "(Unknown file)");
+#endif
 }
 
 #ifdef DEBUG

diff --git a/src/main.c b/src/main.c
index 4951ff9883e4f38efc66ef7d3551b9b202c65bdf..e31ea9ba8c5f099dddb68a229776b0caf253019b 100644 (file)
--- a/src/main.c
+++ b/src/main.c
@@ -30,8 +30,8 @@ add_rpki_oids(void)
        printf("signedObject registered. Its nid is %d.\n", NID_signedObject);
 
        NID_rpkiNotify = OBJ_create("1.3.6.1.5.5.7.48.13",
-                   "id-ad-rpkiNotify (RFC 8182)",
-                   /* TODO */ "Blah blah");
+           "id-ad-rpkiNotify (RFC 8182)",
+           /* TODO */ "Blah blah");
        printf("rpkiNotify registered. Its nid is %d.\n", NID_rpkiNotify);
 }
 

diff --git a/src/object/certificate.c b/src/object/certificate.c
index f0bee2eb7a330f48c3e7c95ef4d80edb8e518c58..b1d64c3572c42749c5b2157f9e73d6f39655c897 100644 (file)
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -590,6 +590,12 @@ is_rsync(ASN1_IA5STRING *uri)
            : false;
 }
 
+static bool
+is_rsync_uri(GENERAL_NAME *name)
+{
+       return name->type == GEN_URI && is_rsync(name->d.GN_URI);
+}
+
 static int
 handle_rpkiManifest(ACCESS_DESCRIPTION *ad, struct rpki_uri *mft)
 {
@@ -971,7 +977,7 @@ handle_cdp(X509_EXTENSION *ext, void *arg)
        names = dp->distpoint->name.fullname;
        for (i = 0; i < sk_GENERAL_NAME_num(names); i++) {
                name = sk_GENERAL_NAME_value(names, i);
-               if (name->type == GEN_URI && is_rsync(name->d.GN_URI)) {
+               if (is_rsync_uri(name)) {
                        /*
                         * Since we're parsing and validating the manifest's CRL
                         * at some point, I think that all we need to do now is
@@ -987,7 +993,6 @@ handle_cdp(X509_EXTENSION *ext, void *arg)
                         * later.
                         */
                        error = ia5s2string(name->d.GN_URI, &refs->crldp);
-                       pr_debug("Certificate CRL: %s", refs->crldp);
                        goto end;
                }
        }
@@ -1010,32 +1015,50 @@ handle_aia(X509_EXTENSION *ext, void *arg)
        AUTHORITY_INFO_ACCESS *aia;
        ACCESS_DESCRIPTION *ad;
        int i;
+       int error;
 
        aia = X509V3_EXT_d2i(ext);
        if (aia == NULL)
                return cannot_decode(&AIA);
 
+       /*
+        * RFC 6487:
+        *     an rsync URI [RFC5781] MUST be specified with an accessMethod
+        *     value of id-ad-caIssuers. (...) Other accessMethod URIs
+        *     referencing the same object MAY also be included in the value
+        *     sequence of this extension.
+        *
+        * It doesn't technically say that id-ad-caIssuers is reserved for RSYNC
+        * URIs, but it sure feels like it's the actual intention.
+        *
+        * But let's commit to the literal wording, which I think equals "there
+        * must be a caIssuers RSYNC URI, which must be correct (ie. equal the
+        * manifest's), and there can also be anything else."
+        */
        for (i = 0; i < sk_ACCESS_DESCRIPTION_num(aia); i++) {
                ad = sk_ACCESS_DESCRIPTION_value(aia, i);
-               if (OBJ_obj2nid(ad->method) == NID_ad_ca_issuers) {
-                       if (ad->location->type != GEN_URI) {
-                               return pr_err("The AIA caIssuers is not an URI. (type: %d)",
-                                   ad->location->type);
-                       }
-
+               /*
+                * TODO open the call hierarchy of location.
+                * All GEN_URIs should probably be handled the same.
+                */
+               if (OBJ_obj2nid(ad->method) == NID_ad_ca_issuers
+                   && is_rsync_uri(ad->location)) {
                        /*
                         * Bringing the parent certificate's URI all the way
                         * over here is too much trouble, so do the handle_cdp()
                         * hack.
                         */
-
-                       return ia5s2string(ad->location->d.GN_URI,
+                       error = ia5s2string(ad->location->d.GN_URI,
                            &refs->caIssuers);
+                       goto end;
                }
        }
 
+       error = pr_err("The certificate lacks a caIssuers RSYNC URI.");
+
+end:
        AUTHORITY_INFO_ACCESS_free(aia);
-       return 0;
+       return error;
 }
 
 static int

diff --git a/test/Makefile.am b/test/Makefile.am
index cd39399facf043760894c0e6a1bc7312c2a7aff7..d3199efd573fa4f3a028b5fceab235810f50bb08 100644 (file)
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -1,26 +1,38 @@
-AM_CFLAGS = -pedantic -Wall -std=gnu11 -I../src ${CHECK_CFLAGS}
+# Reminder: `make check`
+
+# If you want to only run one test, do `make check TESTS=<test>`.
+# Example: `make check TESTS=address`
+
+# Once compiled, you can also just run the test directly: `./address.test`
+# You can easily see standard output and error this way.
+
+AM_CFLAGS = -pedantic -Wall -std=gnu11 -I../src -DUNIT_TESTING ${CHECK_CFLAGS}
 MY_LDADD = ${CHECK_LIBS}
 
-check_PROGRAMS = line_file.test rsync.test
+check_PROGRAMS = address.test line_file.test rsync.test tal.test
 TESTS = ${check_PROGRAMS}
 
+address_test_SOURCES = ../src/address.h
+address_test_SOURCES += ../src/log.c ../src/log.h
+address_test_SOURCES += address_test.c
+address_test_LDADD = ${MY_LDADD}
+
 line_file_test_SOURCES = ../src/file.c ../src/file.h ../src/log.c ../src/log.h
-line_file_test_SOURCES += ../src/thread_var.c ../src/thread_var.h
 line_file_test_SOURCES += ../src/common.c ../src/common.h
 line_file_test_SOURCES += line_file_test.c ../src/line_file.c ../src/line_file.h
 line_file_test_LDADD = ${MY_LDADD}
 
 tal_test_SOURCES = ../src/file.c ../src/file.h ../src/log.c ../src/log.h
-tal_test_SOURCES += ../src/thread_var.c ../src/thread_var.h
 tal_test_SOURCES += ../src/common.c ../src/common.h
 tal_test_SOURCES += ../src/crypto/base64.c ../src/crypto/base64.h
+tal_test_SOURCES += ../src/random.c ../src/random.h
+tal_test_SOURCES += ../src/uri.c ../src/uri.h
 tal_test_SOURCES += tal_test.c ../src/line_file.c ../src/line_file.h
 tal_test_CFLAGS = ${AM_CFLAGS} ${GLIB_CFLAGS}
 tal_test_LDADD = ${MY_LDADD} ${GLIB_LIBS}
 
 rsync_test_SOURCES = ../src/file.c ../src/file.h ../src/log.c ../src/log.h
 rsync_test_SOURCES += ../src/uri.c ../src/uri.h
-rsync_test_SOURCES += ../src/thread_var.c ../src/thread_var.h
 rsync_test_SOURCES += ../src/common.c ../src/common.h
 rsync_test_SOURCES += rsync_test.c
-rsync_test_LDADD = ${MY_LDADD}
\ No newline at end of file
+rsync_test_LDADD = ${MY_LDADD}

diff --git a/test/address_test.c b/test/address_test.c
new file mode 100644 (file)
index 0000000..616b68c
--- /dev/null
+++ b/test/address_test.c
@@ -0,0 +1,187 @@
+#include "address.c"
+
+#include <check.h>
+#include <errno.h>
+#include <stdlib.h>
+
+static void
+test_range4(uint32_t min, uint32_t max, bool valid)
+{
+       struct ipv4_range range = {
+           .min.s_addr = htonl(min),
+           .max.s_addr = htonl(max),
+       };
+       ck_assert_int_eq(valid ? 0 : -EINVAL, check_encoding4(&range));
+}
+
+START_TEST(check_encoding4_test)
+{
+       test_range4(0x00000000, 0x00000000, false);
+       test_range4(0x12345678, 0x12345678, false);
+       test_range4(0xFFFFFFFF, 0xFFFFFFFF, false);
+       test_range4(0x00000000, 0xFFFFFFFF, false);
+       test_range4(0x00000000, 0x00000001, false);
+       test_range4(0x11000000, 0x11001000, true);
+       test_range4(0x11000000, 0x1100FFFF, false);
+       test_range4(0x11000000, 0x1100F1FF, true);
+
+}
+END_TEST
+
+static void
+test_range6(uint32_t a1a, uint32_t a1b, uint32_t a1c, uint32_t a1d,
+    uint32_t a2a, uint32_t a2b, uint32_t a2c, uint32_t a2d,
+    bool valid)
+{
+       struct ipv6_range range;
+
+       range.min.s6_addr32[0] = htonl(a1a);
+       range.min.s6_addr32[1] = htonl(a1b);
+       range.min.s6_addr32[2] = htonl(a1c);
+       range.min.s6_addr32[3] = htonl(a1d);
+       range.max.s6_addr32[0] = htonl(a2a);
+       range.max.s6_addr32[1] = htonl(a2b);
+       range.max.s6_addr32[2] = htonl(a2c);
+       range.max.s6_addr32[3] = htonl(a2d);
+
+       ck_assert_int_eq(valid ? 0 : -EINVAL, check_encoding6(&range));
+}
+
+START_TEST(check_encoding6_test)
+{
+       test_range6(0x00000000, 0x00000000, 0x00000000, 0x00000000,
+                   0x00000000, 0x00000000, 0x00000000, 0x00000000,
+                   false);
+       test_range6(0x12345678, 0x12345678, 0x12345678, 0x12345678,
+                   0x12345678, 0x12345678, 0x12345678, 0x12345678,
+                   false);
+       test_range6(0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   false);
+
+       test_range6(0x00000000, 0x00000000, 0x00000000, 0x00000000,
+                   0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   false);
+       test_range6(0x00000000, 0x00000000, 0x00000000, 0x00000000,
+                   0x00000000, 0x00000000, 0x00000000, 0x00000001,
+                   false);
+
+       /* Matching most significant bits stop on the first quadrant */
+       test_range6(0x00001000, 0x00000000, 0x00000000, 0x00000000,
+                   0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   false);
+
+       test_range6(0x00001010, 0x00000000, 0x00000000, 0x00000000,
+                   0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00001000, 0x00000000, 0x00000000,
+                   0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00000000, 0x00001000, 0x00000000,
+                   0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00000000, 0x00000000, 0x00001000,
+                   0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+
+       test_range6(0x00001000, 0x00000000, 0x00000000, 0x00000000,
+                   0x00001F0F, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00000000, 0x00000000, 0x00000000,
+                   0x00001FFF, 0xFF0FFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00000000, 0x00000000, 0x00000000,
+                   0x00001FFF, 0xFFFFFFFF, 0xFFFFF0FF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00000000, 0x00000000, 0x00000000,
+                   0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFF0FF,
+                   true);
+
+       /* Matching most significant bits stop on the second quadrant */
+       test_range6(0x00001000, 0x00001000, 0x00000000, 0x00000000,
+                   0x00001000, 0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   false);
+
+       test_range6(0x00001000, 0x00001010, 0x00000000, 0x00000000,
+                   0x00001000, 0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00000000,
+                   0x00001000, 0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00001000, 0x00000000, 0x00001000,
+                   0x00001000, 0x00001FFF, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+
+       test_range6(0x00001000, 0x00001000, 0x00000000, 0x00000000,
+                   0x00001000, 0x00001F0F, 0xFFFFFFFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00001000, 0x00000000, 0x00000000,
+                   0x00001000, 0x00001FFF, 0xFFFFF0FF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00001000, 0x00000000, 0x00000000,
+                   0x00001000, 0x00001FFF, 0xFFFFFFFF, 0xFFFFF0FF,
+                   true);
+
+       /* Matching most significant bits stop on the third quadrant */
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00000000,
+                   0x00001000, 0x00001000, 0x00001FFF, 0xFFFFFFFF,
+                   false);
+
+       test_range6(0x00001000, 0x00001000, 0x00001010, 0x00000000,
+                   0x00001000, 0x00001000, 0x00001FFF, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00001000,
+                   0x00001000, 0x00001000, 0x00001FFF, 0xFFFFFFFF,
+                   true);
+
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00000000,
+                   0x00001000, 0x00001000, 0x00001F0F, 0xFFFFFFFF,
+                   true);
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00000000,
+                   0x00001000, 0x00001000, 0x00001FFF, 0xFFFFF0FF,
+                   true);
+
+       /* Matching most significant bits stop on the fourth quadrant */
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00001000,
+                   0x00001000, 0x00001000, 0x00001000, 0x00001FFF,
+                   false);
+
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00001010,
+                   0x00001000, 0x00001000, 0x00001000, 0x00001FFF,
+                   true);
+
+       test_range6(0x00001000, 0x00001000, 0x00001000, 0x00001000,
+                   0x00001000, 0x00001000, 0x00001000, 0x00001F0F,
+                   true);
+}
+END_TEST
+
+Suite *address_load_suite(void)
+{
+       Suite *suite;
+       TCase *core;
+
+       core = tcase_create("Core");
+       tcase_add_test(core, check_encoding4_test);
+       tcase_add_test(core, check_encoding6_test);
+
+       suite = suite_create("Encoding checking");
+       suite_add_tcase(suite, core);
+       return suite;
+}
+
+int main(void)
+{
+       Suite *suite;
+       SRunner *runner;
+       int tests_failed;
+
+       suite = address_load_suite();
+
+       runner = srunner_create(suite);
+       srunner_run_all(runner, CK_NORMAL);
+       tests_failed = srunner_ntests_failed(runner);
+       srunner_free(runner);
+
+       return (tests_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
+}

diff --git a/test/tal_test.c b/test/tal_test.c
index 4f6b9623c6c52326a2718a65d17fc9e88859fe5a..299256f9eda270479ddb6509197560255847c5f2 100644 (file)
--- a/test/tal_test.c
+++ b/test/tal_test.c
@@ -3,11 +3,11 @@
 #include <check.h>
 #include <errno.h>
 #include <stdlib.h>
+#include "common.h"
 
 START_TEST(tal_load_normal)
 {
        struct tal *tal;
-       struct uri *uri;
        unsigned int i;
        /* Got this by feeding the subjectPublicKeyInfo to `base64 -d`. */
        unsigned char decoded[] = {
@@ -42,14 +42,11 @@ START_TEST(tal_load_normal)
 
        ck_assert_int_eq(tal_load("tal/lacnic.tal", &tal), 0);
 
-       uri = SLIST_FIRST(&tal->uris);
-       ck_assert_str_eq(uri->string, "rsync://repository.lacnic.net/rpki/lacnic/rta-lacnic-rpki.cer");
-       uri = SLIST_NEXT(uri, next);
-       ck_assert_str_eq(uri->string, "http://potato");
-       uri = SLIST_NEXT(uri, next);
-       ck_assert_str_eq(uri->string, "rsync://potato");
-       uri = SLIST_NEXT(uri, next);
-       ck_assert(uri == NULL);
+       ck_assert_uint_eq(tal->uris.count, 3);
+       ck_assert_str_eq(tal->uris.array[0],
+           "rsync://repository.lacnic.net/rpki/lacnic/rta-lacnic-rpki.cer");
+       ck_assert_str_eq(tal->uris.array[1], "http://potato");
+       ck_assert_str_eq(tal->uris.array[2], "rsync://potato");
 
        ck_assert_uint_eq(ARRAY_LEN(decoded), tal->spki_len);
        for (i = 0; i < ARRAY_LEN(decoded); i++)