From: pcarana Date: Tue, 6 Aug 2019 16:28:24 +0000 (-0500) Subject: Assign ID to incidence, validate RTR port, fix slurm bug. X-Git-Tag: v1.0.0^2~13 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=16d41cd4cc2ed4305ff59d39310cf213ce11c45f;p=thirdparty%2FFORT-validator.git Assign ID to incidence, validate RTR port, fix slurm bug. -Use an ID for the registered incidence. -When loading multiple SLURM files, the validation from RFC 8416 section 4.2 wasn't considered. A context is used to perform such validation, since every prefix or asn (for bgpsec) must be validated according to its own context/file. -Remove dead code (structs) from SLURM. -Validate RTR port range since this isn't validated by getaddrinfo. --- diff --git a/docs/doc/incidence.md b/docs/doc/incidence.md index 91fa9c98..4fe4504b 100644 --- a/docs/doc/incidence.md +++ b/docs/doc/incidence.md @@ -17,7 +17,7 @@ title: Incidence The RPKI RFCs define fairly strict profiles for RPKI objects, and are unequivocal in stating that incorrectly-formed objects are supposed to be rejected by Relying Party validation. In practice, however, this does not prevent a significant amount of Certificate Authorities from issuing incorrect objects. -By default, Fort is as pedantic as it can possibly be. The `incidence` section of its configuration file is a means to modify its behavior upon encountering profile violations that, from experience, are often overlooked. +By default, Fort is lax with some of this bad practices. The `incidence` section of its configuration file is a means to modify its behavior upon encountering profile violations that, from experience, are often overlooked. ## `incidences` definition @@ -26,13 +26,13 @@ By default, Fort is as pedantic as it can possibly be. The `incidence` section o ``` "incidences": [ { - "name": "Signed Object's hash algorithm has NULL object as parameters", + "name": "incid-hashalg-has-params", "action": "warn" } ] ``` -`name` is the identifier of an incidence. It is case-sensitive and developer-defined. It states the particular error condition that will be handled by the remaining field. +`name` is the identifier of an incidence. It is case-sensitive and developer-defined. It states an ID of the particular error condition that will be handled by the remaining field. `action` is an enumeration that states the outcome of a violation of the corresponding incidence. It can take one of three values: @@ -40,7 +40,7 @@ By default, Fort is as pedantic as it can possibly be. The `incidence` section o 2. `warn`: Print error message in `warning` log level, continue validation as if nothing happened. 3. `ignore`: Do not print error message, continue validation as if nothing happened. -By Fort's pedantic nature, most incidences have an `action` of `error` by default. +Since most of the incidences are result of a bad practice at the global RPKI, they have an `action` of `ignore` by default. If a strict behavior is desired, then the corresponding incidences should be configured with an `action` of `error`. ## Incidence types @@ -48,6 +48,9 @@ Presently, there is only one incidence type defined. This list is expected to gr ### Signed Object's hash algorithm has NULL object as parameters +- **Name:** `incid-hashalg-has-params` +- **Default action:** `ignore` + [RFC 6488](https://tools.ietf.org/html/rfc6488) (RPKI Signed Objects) defers digest algorithm specification to RFC 6485: ``` diff --git a/docs/doc/usage.md b/docs/doc/usage.md index 03171a26..4664bd4e 100644 --- a/docs/doc/usage.md +++ b/docs/doc/usage.md @@ -411,7 +411,7 @@ The configuration options are mostly the same as the ones from the `argv` interf "incidences": [ { - "name": "Signed Object's hash algorithm has NULL object as parameters", + "name": "incid-hashalg-has-params", "action": "ignore" } ], diff --git a/man/fort.8 b/man/fort.8 index e363d30f..0f6abccf 100644 --- a/man/fort.8 +++ b/man/fort.8 @@ -118,7 +118,7 @@ of objects, each with two members "name" and "action", eg: .RS 2 { .RS 2 -"name": "Signed Object's hash algorithm has NULL object as parameters", +"name": "incid-hashalg-has-params", .br "action": "warn" .RE @@ -144,9 +144,10 @@ nothing happened. happened. .RE .P -By default, all the incidences have an action of \fIerror\fR. Currently there's +By default, all the incidences have an action of \fIignore\fR. Currently there's only one registered incidence: -\fISigned Object's hash algorithm has NULL object as parameters\fR. +\fIincid-hashalg-has-params\fR (Signed Object's hash algorithm has NULL object +as parameters). .P More information about incidences can be consulted at FORT's web docs. .RE @@ -174,8 +175,8 @@ and/or read. Right now, FORT accesses RPKI repositories by way of \fIrsync\fR. During each validation cycle, FORT will literally invoke an rsync command (see \fBrsync.program\fR and \fBrsync.arguments-recursive\fR), which will download -the files into \fB--local-repository\fR. FORT’s validation operates on the resulting -copy. +the files into \fB--local-repository\fR. FORT’s validation operates on the +resulting copy. .P Because rsync uses delta encoding, you’re advised to keep this cache around. It significantly speeds up subsequent validation cycles. @@ -501,7 +502,7 @@ to a specific value: }, "incidences": [ { - "name": "Signed Object's hash algorithm has NULL object as parameters", + "name": "incid-hashalg-has-params", "action": "ignore" } ], diff --git a/src/incidence/incidence.c b/src/incidence/incidence.c index 5cd7fb34..3c28cf08 100644 --- a/src/incidence/incidence.c +++ b/src/incidence/incidence.c @@ -11,6 +11,7 @@ struct incidence { const enum incidence_id id; char const *const name; + char const *const description; const enum incidence_action default_action; enum incidence_action action; }; @@ -18,8 +19,9 @@ struct incidence { static struct incidence incidences[__INID_MAX] = { { INID_HASHALG_HAS_PARAMS, + "incid-hashalg-has-params", "Signed Object's hash algorithm has NULL object as parameters", - INAC_ERROR, + INAC_IGNORE, }, }; @@ -81,10 +83,6 @@ init_action(json_t *json) else return pr_err("Unknown incidence action: '%s'", action_str); - if (action > incidences[id].action) - return pr_err("The '%s' incidence cannot have a more severe action than '%s'.", - name, action2str(incidences[id].action)); - incidences[id].action = action; return 0; } @@ -143,7 +141,8 @@ incidence_print(void) for (i = 0; i < __INID_MAX; i++) { if (incidences[i].action != incidences[i].default_action) { - pr_info("%s: %s", incidences[i].name, + pr_info("%s (%s): %s", incidences[i].name, + incidences[i].description, action2str(incidences[i].action)); printed = true; } diff --git a/src/rtr/rtr.c b/src/rtr/rtr.c index f72fdfaa..88ac2ca5 100644 --- a/src/rtr/rtr.c +++ b/src/rtr/rtr.c @@ -32,7 +32,9 @@ init_addrinfo(struct addrinfo **result) { char const *hostname; char const *service; + char *tmp; struct addrinfo hints; + unsigned long parsed, port; int error; memset(&hints, 0 , sizeof(hints)); @@ -52,6 +54,26 @@ init_addrinfo(struct addrinfo **result) return error; } + errno = 0; + parsed = strtoul(service, &tmp, 10); + if (errno || *tmp != '\0') + return 0; /* Ok, not a number */ + + /* + * 'getaddrinfo' isn't very strict validating the service when a port + * number is indicated. If a port larger than the max (65535) is + * received, the 16 rightmost bits are utilized as the port and set at + * the addrinfo returned. + * + * So, a manual validation is implemented. Port is actually a uint16_t, + * so read what's necessary and compare using the same data type. + */ + port = (unsigned char)((*result)->ai_addr->sa_data[0]) << 8; + port += (unsigned char)((*result)->ai_addr->sa_data[1]); + if (parsed != port) + return pr_err("Service port %s is out of range (max value is %d)", + service, USHRT_MAX); + return 0; } diff --git a/src/slurm/slurm_db.c b/src/slurm/slurm_db.c index 7109705d..fa0dfd95 100644 --- a/src/slurm/slurm_db.c +++ b/src/slurm/slurm_db.c @@ -6,10 +6,20 @@ #include "data_structure/array_list.h" -ARRAY_LIST(al_filter_prefix, struct slurm_prefix) -ARRAY_LIST(al_assertion_prefix, struct slurm_prefix) -ARRAY_LIST(al_filter_bgpsec, struct slurm_bgpsec) -ARRAY_LIST(al_assertion_bgpsec, struct slurm_bgpsec) +struct slurm_prefix_ctx { + struct slurm_prefix element; + int ctx; +}; + +struct slurm_bgpsec_ctx { + struct slurm_bgpsec element; + int ctx; +}; + +ARRAY_LIST(al_filter_prefix, struct slurm_prefix_ctx) +ARRAY_LIST(al_assertion_prefix, struct slurm_prefix_ctx) +ARRAY_LIST(al_filter_bgpsec, struct slurm_bgpsec_ctx) +ARRAY_LIST(al_assertion_bgpsec, struct slurm_bgpsec_ctx) struct arraylist_db { struct al_filter_prefix filter_pfx_al; @@ -49,15 +59,49 @@ prefix_filtered_by(struct slurm_prefix *filter, struct slurm_prefix *prefix) } static bool -prefix_equal(struct slurm_prefix *left, struct slurm_prefix *right, - bool filter) +prefix_contained(struct slurm_prefix_ctx *left_ctx, struct slurm_prefix *right, + int ctx) { + struct slurm_prefix *left; + struct vrp *left_vrp, *right_vrp; + + /* + * rfc8416#section-4.2: + * 1. There may be conflicting changes to ROA Prefix Assertions if an + * IP address X and distinct SLURM files Y and Z exist such that X + * is contained by any prefix in any "prefixAssertions" or + * "prefixFilters" in file Y and X is contained by any prefix in any + * "prefixAssertions" or "prefixFilters" in file Z. + * + * A negative @ctx or an equal context will avoid this check. + */ + if (ctx < 0 || ctx == left_ctx->ctx) + return false; + + left = &left_ctx->element; + left_vrp = &left->vrp; + right_vrp = &right->vrp; + + return (left->data_flag & SLURM_PFX_FLAG_PREFIX) > 0 && + (right->data_flag & SLURM_PFX_FLAG_PREFIX) > 0 && + VRP_PREFIX_EQ(left_vrp, right_vrp); +} + +static bool +prefix_equal(struct slurm_prefix_ctx *left_ctx, struct slurm_prefix *right, + int ctx, bool filter) +{ + struct slurm_prefix *left; struct vrp *left_vrp, *right_vrp; bool equal; + left = &left_ctx->element; left_vrp = &left->vrp; right_vrp = &right->vrp; + if (prefix_contained(left_ctx, right, ctx)) + return true; + /* Ignore the comments */ if ((left->data_flag & ~SLURM_COM_FLAG_COMMENT) != (right->data_flag & ~SLURM_COM_FLAG_COMMENT)) @@ -97,11 +141,42 @@ bgpsec_filtered_by(struct slurm_bgpsec *bgpsec, struct slurm_bgpsec *filter) } static bool -bgpsec_equal(struct slurm_bgpsec *left, struct slurm_bgpsec *right, - bool filter) +bgpsec_contained(struct slurm_bgpsec_ctx *left_ctx, struct slurm_bgpsec *right, + int ctx) { + struct slurm_bgpsec *left; + + /* + * rfc8416#section-4.2: + * 2. There may be conflicting changes to BGPsec Assertions if an ASN X + * and distinct SLURM files Y and Z exist such that X is used in any + * "bgpsecAssertions" or "bgpsecFilters" in file Y and X is used in + * any "bgpsecAssertions" or "bgpsecFilters" in file Z. + * + * A negative @ctx or an equal context will avoid this check. + */ + if (ctx < 0 || ctx == left_ctx->ctx) + return false; + + left = &left_ctx->element; + + return (left->data_flag & SLURM_COM_FLAG_ASN) > 0 && + (right->data_flag & SLURM_COM_FLAG_ASN) > 0 && + left->asn == right->asn; +} + +static bool +bgpsec_equal(struct slurm_bgpsec_ctx *left_ctx, struct slurm_bgpsec *right, + int ctx, bool filter) +{ + struct slurm_bgpsec *left; bool equal; + left = &left_ctx->element; + + if (bgpsec_contained(left_ctx, right, ctx)) + return true; + /* Ignore the comments */ if ((left->data_flag & ~SLURM_COM_FLAG_COMMENT) != (right->data_flag & ~SLURM_COM_FLAG_COMMENT)) @@ -126,41 +201,53 @@ bgpsec_equal(struct slurm_bgpsec *left, struct slurm_bgpsec *right, return equal; } -#define ADD_FUNCS(name, type, list_name, db_list, equal_cb, filter) \ +#define ADD_FUNCS(name, type, list_name, db_list, db_alt_list, equal_cb,\ + cont_cb, filter) \ static type * \ - name##_locate(type *obj) \ + name##_locate(type *obj, int ctx) \ { \ - type *cursor; \ + type##_ctx *cursor; \ array_index i; \ \ ARRAYLIST_FOREACH(db_list, cursor, i) \ - if (equal_cb(cursor, obj, filter)) \ - return cursor; \ + if (equal_cb(cursor, obj, ctx, filter)) \ + return &cursor->element; \ + \ + ARRAYLIST_FOREACH(db_alt_list, cursor, i) \ + if (cont_cb(cursor, obj, ctx)) \ + return &cursor->element; \ \ return NULL; \ } \ \ static bool \ - name##_exists(type *obj) \ + name##_exists(type *obj, int ctx) \ { \ - return name##_locate(obj) != NULL; \ + return name##_locate(obj, ctx) != NULL; \ } \ \ int \ - slurm_db_add_##name(type *elem) { \ - if (name##_exists(elem)) \ + slurm_db_add_##name(type *elem, int ctx) { \ + type##_ctx new_elem; \ + if (name##_exists(elem, ctx)) \ return -EEXIST; \ - return list_name##_add(db_list, elem); \ + new_elem.element = *elem; \ + new_elem.ctx = ctx; \ + return list_name##_add(db_list, &new_elem); \ } ADD_FUNCS(prefix_filter, struct slurm_prefix, al_filter_prefix, - &array_lists_db.filter_pfx_al, prefix_equal, true) + &array_lists_db.filter_pfx_al, &array_lists_db.assertion_pfx_al, + prefix_equal, prefix_contained, true) ADD_FUNCS(bgpsec_filter, struct slurm_bgpsec, al_filter_bgpsec, - &array_lists_db.filter_bgps_al, bgpsec_equal, true) + &array_lists_db.filter_bgps_al, &array_lists_db.assertion_bgps_al, + bgpsec_equal, bgpsec_contained, true) ADD_FUNCS(prefix_assertion, struct slurm_prefix, al_assertion_prefix, - &array_lists_db.assertion_pfx_al, prefix_equal, false) + &array_lists_db.assertion_pfx_al, &array_lists_db.filter_pfx_al, + prefix_equal, prefix_contained, false) ADD_FUNCS(bgpsec_assertion, struct slurm_bgpsec, al_assertion_bgpsec, - &array_lists_db.assertion_bgps_al, bgpsec_equal, false) + &array_lists_db.assertion_bgps_al, &array_lists_db.filter_bgps_al, + bgpsec_equal, bgpsec_contained, false) bool slurm_db_vrp_is_filtered(struct vrp const *vrp) @@ -172,18 +259,18 @@ slurm_db_vrp_is_filtered(struct vrp const *vrp) slurm_prefix.vrp = *vrp; slurm_prefix.comment = NULL; - return prefix_filter_exists(&slurm_prefix); + return prefix_filter_exists(&slurm_prefix, -1); } int slurm_db_foreach_assertion_prefix(assertion_pfx_foreach_cb cb, void *arg) { - struct slurm_prefix *cursor; + struct slurm_prefix_ctx *cursor; array_index i; int error; ARRAYLIST_FOREACH(&array_lists_db.assertion_pfx_al, cursor, i) { - error = cb(cursor, arg); + error = cb(&cursor->element, arg); if (error) return error; } @@ -192,21 +279,21 @@ slurm_db_foreach_assertion_prefix(assertion_pfx_foreach_cb cb, void *arg) } static void -clean_slurm_prefix(struct slurm_prefix *prefix) +clean_slurm_prefix(struct slurm_prefix_ctx *prefix) { - if ((prefix->data_flag & SLURM_COM_FLAG_COMMENT) > 0) - free(prefix->comment); + if ((prefix->element.data_flag & SLURM_COM_FLAG_COMMENT) > 0) + free(prefix->element.comment); } static void -clean_slurm_bgpsec(struct slurm_bgpsec *bgpsec) +clean_slurm_bgpsec(struct slurm_bgpsec_ctx *bgpsec) { - if ((bgpsec->data_flag & SLURM_BGPS_FLAG_SKI) > 0) - free(bgpsec->ski); - if ((bgpsec->data_flag & SLURM_BGPS_FLAG_ROUTER_KEY) > 0) - free(bgpsec->router_public_key); - if ((bgpsec->data_flag & SLURM_COM_FLAG_COMMENT) > 0) - free(bgpsec->comment); + if ((bgpsec->element.data_flag & SLURM_BGPS_FLAG_SKI) > 0) + free(bgpsec->element.ski); + if ((bgpsec->element.data_flag & SLURM_BGPS_FLAG_ROUTER_KEY) > 0) + free(bgpsec->element.router_public_key); + if ((bgpsec->element.data_flag & SLURM_COM_FLAG_COMMENT) > 0) + free(bgpsec->element.comment); } void diff --git a/src/slurm/slurm_db.h b/src/slurm/slurm_db.h index 52d19747..f664f2f7 100644 --- a/src/slurm/slurm_db.h +++ b/src/slurm/slurm_db.h @@ -4,31 +4,14 @@ #include #include "slurm/slurm_parser.h" -struct slurm_prefix_list { - struct slurm_prefix *list; - unsigned int len; -}; - -struct slurm_bgpsec_list { - struct slurm_bgpsec *list; - unsigned int len; -}; - -struct slurm_db { - struct slurm_prefix_list prefix_filters; - struct slurm_bgpsec_list bgpsec_filters; - struct slurm_prefix_list prefix_assertions; - struct slurm_bgpsec_list bgpsec_assertions; -}; - typedef int (*assertion_pfx_foreach_cb)(struct slurm_prefix *, void *); void slurm_db_init(void); -int slurm_db_add_prefix_filter(struct slurm_prefix *); -int slurm_db_add_prefix_assertion(struct slurm_prefix *); -int slurm_db_add_bgpsec_filter(struct slurm_bgpsec *); -int slurm_db_add_bgpsec_assertion(struct slurm_bgpsec *); +int slurm_db_add_prefix_filter(struct slurm_prefix *, int); +int slurm_db_add_prefix_assertion(struct slurm_prefix *, int); +int slurm_db_add_bgpsec_filter(struct slurm_bgpsec *, int); +int slurm_db_add_bgpsec_assertion(struct slurm_bgpsec *, int); bool slurm_db_vrp_is_filtered(struct vrp const *vrp); int slurm_db_foreach_assertion_prefix(assertion_pfx_foreach_cb, void *); diff --git a/src/slurm/slurm_loader.c b/src/slurm/slurm_loader.c index 8b089b4e..3c8fa6e2 100644 --- a/src/slurm/slurm_loader.c +++ b/src/slurm/slurm_loader.c @@ -16,6 +16,7 @@ static int slurm_load(bool *loaded) { + int ctx = 0; /* Context (file number) */ /* Optional configuration */ *loaded = false; if (config_get_slurm() == NULL) @@ -24,8 +25,8 @@ slurm_load(bool *loaded) *loaded = true; slurm_db_init(); - return process_file_or_dir(config_get_slurm(), - SLURM_FILE_EXTENSION, slurm_parse, NULL); + return process_file_or_dir(config_get_slurm(), SLURM_FILE_EXTENSION, + slurm_parse, &ctx); } static void diff --git a/src/slurm/slurm_parser.c b/src/slurm/slurm_parser.c index 2cc52e90..eebc1544 100644 --- a/src/slurm/slurm_parser.c +++ b/src/slurm/slurm_parser.c @@ -34,7 +34,10 @@ if (element == NULL) \ return pr_err("SLURM member '%s' is required", name); -static int handle_json(json_t *); +/* Context value, local to avoid forwarding the parameter */ +int cur_ctx; + +static int handle_json(json_t *, int *); int slurm_parse(char const *location, void *arg) @@ -51,7 +54,7 @@ slurm_parse(char const *location, void *arg) return -ENOENT; } - error = handle_json(json_root); + error = handle_json(json_root, arg); json_decref(json_root); return error; @@ -382,7 +385,7 @@ load_single_prefix(json_t *object, bool is_assertion) goto release_comment; } - error = slurm_db_add_prefix_filter(&result); + error = slurm_db_add_prefix_filter(&result, cur_ctx); if (error) goto release_comment; @@ -409,7 +412,7 @@ load_single_prefix(json_t *object, bool is_assertion) goto release_comment; } - error = slurm_db_add_prefix_assertion(&result); + error = slurm_db_add_prefix_assertion(&result, cur_ctx); if (error) goto release_comment; @@ -428,21 +431,24 @@ load_prefix_array(json_t *array, bool is_assertion) json_array_foreach(array, index, element) { error = load_single_prefix(element, is_assertion); - if (error) { - if (error == -EEXIST) - pr_err( - "The prefix %s element \"%s\", is duplicated or covered by another %s; SLURM loading will be stopped", - (is_assertion ? "assertion" : "filter"), - json_dumps(element, 0), - (is_assertion ? "assertion" : "filter")); - else - pr_err( - "Error at prefix %s, element \"%s\", SLURM loading will be stopped", - (is_assertion ? "assertions" : "filters"), - json_dumps(element, 0)); + if (!error) + continue; + if (error == -EEXIST) + pr_err( + "The prefix %s element \"%s\", is duplicated or covered by another %s; SLURM loading will be stopped.%s", + (is_assertion ? "assertion" : "filter"), + json_dumps(element, 0), + (is_assertion ? "assertion" : "filter"), + (cur_ctx > 0 + ? " TIP: More than 1 SLURM files were found, check if the prefix is contained in multiple files (see RFC 8416 section 4.2)." + : "")); + else + pr_err( + "Error at prefix %s, element \"%s\", SLURM loading will be stopped", + (is_assertion ? "assertions" : "filters"), + json_dumps(element, 0)); - return error; - } + return error; } return 0; @@ -515,7 +521,7 @@ load_single_bgpsec(json_t *object, bool is_assertion) goto release_comment; } - error = slurm_db_add_bgpsec_filter(&result); + error = slurm_db_add_bgpsec_filter(&result, cur_ctx); if (error) goto release_comment; @@ -529,7 +535,7 @@ load_single_bgpsec(json_t *object, bool is_assertion) goto release_comment; } - error = slurm_db_add_bgpsec_assertion(&result); + error = slurm_db_add_bgpsec_assertion(&result, cur_ctx); if (error) goto release_comment; @@ -552,21 +558,24 @@ load_bgpsec_array(json_t *array, bool is_assertion) json_array_foreach(array, index, element) { error = load_single_bgpsec(element, is_assertion); - if (error) { - if (error == -EEXIST) - pr_err( - "The bgpsec %s element \"%s\", is duplicated or covered by another %s; SLURM loading will be stopped", - (is_assertion ? "assertion" : "filter"), - json_dumps(element, 0), - (is_assertion ? "assertion" : "filter")); - else - pr_err( - "Error at bgpsec %s, element \"%s\", SLURM loading will be stopped", - (is_assertion ? "assertions" : "filters"), - json_dumps(element, 0)); + if (!error) + continue; + if (error == -EEXIST) + pr_err( + "The bgpsec %s element \"%s\", is duplicated or covered by another %s; SLURM loading will be stopped.%s", + (is_assertion ? "assertion" : "filter"), + json_dumps(element, 0), + (is_assertion ? "assertion" : "filter"), + (cur_ctx > 0 + ? " TIP: More than 1 SLURM files were found, check if the ASN is contained in multiple files (see RFC 8416 section 4.2)." + : "")); + else + pr_err( + "Error at bgpsec %s, element \"%s\", SLURM loading will be stopped", + (is_assertion ? "assertions" : "filters"), + json_dumps(element, 0)); - return error; - } + return error; } return 0; @@ -660,7 +669,7 @@ load_assertions(json_t *root) } static int -handle_json(json_t *root) +handle_json(json_t *root, int *ctx) { size_t expected_members; int error; @@ -668,6 +677,8 @@ handle_json(json_t *root) if (!json_is_object(root)) return pr_err("The root of the SLURM is not a JSON object."); + cur_ctx = *ctx; + error = load_version(root); if (error) return error; @@ -686,5 +697,7 @@ handle_json(json_t *root) "SLURM root must have only %lu members (RFC 8416 section 3.2)", expected_members); + (*ctx)++; /* Next time will be another context */ + return 0; }