From: Alberto Leiva Popper Date: Tue, 29 Jan 2019 20:36:09 +0000 (-0600) Subject: Add validation of IP vs Range selection X-Git-Tag: v0.0.2~105^2~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4656e70cf36318411762c2f905eda49d784ac33b;p=thirdparty%2FFORT-validator.git 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 6aae8f42..41bdb4c7 100644 --- 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 b117daaf..cb5803a1 100644 --- 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 0366c408..91cbc334 100644 --- 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 165d13cd..727a3e56 100644 --- 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 4951ff98..e31ea9ba 100644 --- 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 f0bee2eb..b1d64c35 100644 --- 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 cd39399f..d3199efd 100644 --- 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=`. +# 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 index 00000000..616b68cf --- /dev/null +++ b/test/address_test.c @@ -0,0 +1,187 @@ +#include "address.c" + +#include +#include +#include + +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 4f6b9623..299256f9 100644 --- a/test/tal_test.c +++ b/test/tal_test.c @@ -3,11 +3,11 @@ #include #include #include +#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++)