From 2d160d935af1168ac6deaade64c4a81771d9bc21 Mon Sep 17 00:00:00 2001 From: Alberto Leiva Popper Date: Thu, 14 Feb 2019 17:03:22 -0600 Subject: [PATCH] Address several TODOs. --- src/address.h | 3 --- src/algorithm.c | 4 ++-- src/asn1/decode.c | 10 ++++++++-- src/asn1/signed_data.c | 37 ++++++++++++++++++++++++++++--------- src/asn1/signed_data.h | 14 ++++++++++++-- src/main.c | 8 ++++++-- src/object/certificate.c | 7 +++---- src/object/certificate.h | 6 +++++- src/object/manifest.c | 22 +++++++++++----------- src/object/roa.c | 15 +++++---------- src/object/tal.c | 14 +++++++++++++- src/object/tal.h | 3 ++- src/resource.c | 19 ++++++++----------- src/rsync/rsync.c | 6 ++++-- src/state.c | 3 ++- src/thread_var.h | 1 - 16 files changed, 109 insertions(+), 63 deletions(-) diff --git a/src/address.h b/src/address.h index 43e8c79e..62859b53 100644 --- a/src/address.h +++ b/src/address.h @@ -35,7 +35,4 @@ int prefix6_decode(IPAddress_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/algorithm.c b/src/algorithm.c index 4df2d1f7..84a10d5d 100644 --- a/src/algorithm.c +++ b/src/algorithm.c @@ -12,8 +12,8 @@ int rpki_public_key_algorithm(void) { /* - * TODO Everyone uses this algorithm, but at a quick glance, it doesn't - * seem to match RFC 7935's public key algorithm. Wtf? + * TODO Everyone uses this algorithm, but the RFC says that it should + * be NID_sha256WithRSAEncryption. Wtf? */ return NID_rsaEncryption; } diff --git a/src/asn1/decode.c b/src/asn1/decode.c index 2800c52d..4e4f2647 100644 --- a/src/asn1/decode.c +++ b/src/asn1/decode.c @@ -34,8 +34,9 @@ asn1_decode(const void *buffer, size_t buffer_size, if (rval.code != RC_OK) { /* Must free partial object according to API contracts. */ ASN_STRUCT_FREE(*descriptor, *result); - /* TODO if rval.code == RC_WMORE (1), more work is needed */ - return pr_err("Error decoding ASN.1 object: %u", rval.code); + /* We expect the data to be complete; RC_WMORE is an error. */ + return pr_err("Error '%u' decoding ASN.1 object around byte %zu", + rval.code, rval.consumed); } error = validate(descriptor, *result); @@ -61,6 +62,11 @@ asn1_decode_octet_string(OCTET_STRING_t *string, return asn1_decode(string->buf, string->size, descriptor, result); } +/* + * TODO (next iteration) There's no need to load the entire file into memory. + * ber_decode() can take an incomplete buffer, in which case it returns + * RC_WMORE. + */ int asn1_decode_fc(struct file_contents *fc, asn_TYPE_descriptor_t const *descriptor, void **result) diff --git a/src/asn1/signed_data.c b/src/asn1/signed_data.c index 4d0b7f06..39b47f28 100644 --- a/src/asn1/signed_data.c +++ b/src/asn1/signed_data.c @@ -17,6 +17,27 @@ static const OID oid_mda = OID_MESSAGE_DIGEST_ATTR; static const OID oid_sta = OID_SIGNING_TIME_ATTR; static const OID oid_bsta = OID_BINARY_SIGNING_TIME_ATTR; +int +signed_object_args_init(struct signed_object_args *args, + struct rpki_uri const *uri, STACK_OF(X509_CRL) *crls) +{ + args->res = resources_create(); + if (args->res == NULL) + return pr_enomem(); + + args->uri = uri; + args->crls = crls; + memset(&args->refs, 0, sizeof(args->refs)); + return 0; +} + +void +signed_object_args_cleanup(struct signed_object_args *args) +{ + resources_destroy(args->res); + refs_cleanup(&args->refs); +} + static int is_digest_algorithm(AlgorithmIdentifier_t *id, char const *what) { @@ -95,13 +116,10 @@ handle_sdata_certificate(ANY_t *any, struct signed_object_args *args, if (error) goto end2; - if (args->res != NULL) { - /* TODO validate resources even if the SO lacks them */ - resources_set_policy(args->res, policy); - error = certificate_get_resources(cert, args->res); - if (error) - goto end2; - } + resources_set_policy(args->res, policy); + error = certificate_get_resources(cert, args->res); + if (error) + goto end2; end2: X509_free(cert); @@ -392,7 +410,7 @@ validate(struct SignedData *sdata, struct signed_object_args *args) /* rfc6488#section-3.2 */ /* rfc6488#section-3.3 */ - /* TODO */ + /* TODO (field) */ return 0; } @@ -404,7 +422,8 @@ signed_data_decode(ANY_t *coded, struct signed_object_args *args, struct SignedData *sdata; int error; - /* rfc6488#section-3.1.l TODO this is BER, not guaranteed to be DER. */ + /* rfc6488#section-3.1.l */ + /* TODO (next iteration) this is BER, not guaranteed to be DER. */ error = asn1_decode_any(coded, &asn_DEF_SignedData, (void **) &sdata); if (error) return error; diff --git a/src/asn1/signed_data.h b/src/asn1/signed_data.h index 81643cf9..bf31fb83 100644 --- a/src/asn1/signed_data.h +++ b/src/asn1/signed_data.h @@ -10,15 +10,25 @@ /* * This only exists to reduce argument lists. - * TODO document fields. */ struct signed_object_args { + /** Location of the signed object. */ + struct rpki_uri const *uri; + /** CRL that might or might not revoke the embedded certificate. */ STACK_OF(X509_CRL) *crls; + /** A copy of the resources carried by the embedded certificate. */ struct resources *res; - struct rpki_uri const *uri; + /** + * A bunch of URLs found in the embedded certificate's extensions, + * recorded for future validation. + */ struct certificate_refs refs; }; +int signed_object_args_init(struct signed_object_args *, + struct rpki_uri const *, STACK_OF(X509_CRL) *); +void signed_object_args_cleanup(struct signed_object_args *); + int signed_data_decode(ANY_t *, struct signed_object_args *args, struct SignedData **); void signed_data_free(struct SignedData *); diff --git a/src/main.c b/src/main.c index c729b43e..65b90ea4 100644 --- a/src/main.c +++ b/src/main.c @@ -197,9 +197,13 @@ main(int argc, char **argv) if (!error) { if (config_get_shuffle_uris()) tal_shuffle_uris(tal); + error = foreach_uri(tal, handle_tal_uri); - /* TODO error on no uris were valid. */ - error = (error >= 0) ? 0 : error; + if (error > 0) + error = 0; + else if (error == 0) + error = pr_err("None of the URIs of the TAL yielded a successful traversal."); + tal_destroy(tal); } diff --git a/src/object/certificate.c b/src/object/certificate.c index f7b6b6da..dd53abe8 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -155,13 +155,12 @@ validate_spki(const unsigned char *cert_spk, int cert_spk_len) * trouble. We'll have to decode the TAL's SPKI though. */ - /* - * TODO we're decoding the TAL's public key, but the stacked file name - * is the certificate's. It looks weird when it errors. - */ + fnstack_push(tal_get_file_name(tal)); tal_get_spki(tal, &_tal_spki, &_tal_spki_len); error = asn1_decode(_tal_spki, _tal_spki_len, &asn_DEF_SubjectPublicKeyInfo, (void **) &tal_spki); + fnstack_pop(); + if (error) goto fail1; diff --git a/src/object/certificate.h b/src/object/certificate.h index 5b87e3ea..22d0a681 100644 --- a/src/object/certificate.h +++ b/src/object/certificate.h @@ -24,7 +24,11 @@ int certificate_validate_rfc6487(X509 *, bool); /** * Returns the IP and AS resources declared in the respective extensions. * - * TODO why is this not part of the certificate_validate*s, again? + * Note: One reason why this is separate from the validate_extensions functions + * is because it needs to be handled after the policy has been extracted from + * the certificate policies extension, and handle_extensions() currently does + * not care about order. I don't know if you'll find other reasons if you choose + * to migrate it. */ int certificate_get_resources(X509 *, struct resources *); diff --git a/src/object/manifest.c b/src/object/manifest.c index 2f851aea..94cacb86 100644 --- a/src/object/manifest.c +++ b/src/object/manifest.c @@ -179,31 +179,31 @@ handle_manifest(struct rpki_uri const *uri, STACK_OF(X509_CRL) *crls, pr_debug_add("Manifest %s {", uri->global); fnstack_push(uri->global); - sobj_args.uri = uri; - sobj_args.crls = crls; - sobj_args.res = NULL; - memset(&sobj_args.refs, 0, sizeof(sobj_args.refs)); + error = signed_object_args_init(&sobj_args, uri, crls); + if (error) + goto end1; mft.file_path = uri->global; error = signed_object_decode(&sobj_args, &asn_DEF_Manifest, &arcs, - (void **) &mft.obj); /* mft.obj and sobj_args.refs in the heap */ + (void **) &mft.obj); if (error) - goto end1; + goto end2; error = validate_manifest(mft.obj); if (error) - goto end2; - error = __handle_manifest(&mft, pp); /* pp in the heap */ + goto end3; + error = __handle_manifest(&mft, pp); if (error) - goto end2; + goto end3; error = refs_validate_ee(&sobj_args.refs, *pp, uri); if (error) rpp_destroy(*pp); -end2: +end3: ASN_STRUCT_FREE(asn_DEF_Manifest, mft.obj); - refs_cleanup(&sobj_args.refs); +end2: + signed_object_args_cleanup(&sobj_args); end1: pr_debug_rm("}"); fnstack_pop(); diff --git a/src/object/roa.c b/src/object/roa.c index e31a4844..9948f5a2 100644 --- a/src/object/roa.c +++ b/src/object/roa.c @@ -191,7 +191,8 @@ family_error: return pr_err("ROA's IP family is not v4 or v6."); } -int handle_roa(struct rpki_uri const *uri, struct rpp *pp, +int +handle_roa(struct rpki_uri const *uri, struct rpp *pp, STACK_OF(X509_CRL) *crls) { static OID oid = OID_ROA; @@ -203,14 +204,9 @@ int handle_roa(struct rpki_uri const *uri, struct rpp *pp, pr_debug_add("ROA %s {", uri->global); fnstack_push(uri->global); - sobj_args.uri = uri; - sobj_args.crls = crls; - sobj_args.res = resources_create(); - if (sobj_args.res == NULL) { - error = pr_enomem(); + error = signed_object_args_init(&sobj_args, uri, crls); + if (error) goto end1; - } - memset(&sobj_args.refs, 0, sizeof(sobj_args.refs)); error = signed_object_decode(&sobj_args, &asn_DEF_RouteOriginAttestation, &arcs, (void **) &roa); @@ -225,9 +221,8 @@ int handle_roa(struct rpki_uri const *uri, struct rpp *pp, end3: ASN_STRUCT_FREE(asn_DEF_RouteOriginAttestation, roa); - refs_cleanup(&sobj_args.refs); end2: - resources_destroy(sobj_args.res); + signed_object_args_cleanup(&sobj_args); end1: pr_debug_rm("}"); fnstack_pop(); diff --git a/src/object/tal.c b/src/object/tal.c index 42911347..3b83de77 100644 --- a/src/object/tal.c +++ b/src/object/tal.c @@ -22,6 +22,7 @@ struct uris { }; struct tal { + char const *file_name; struct uris uris; unsigned char *spki; /* Decoded; not base64. */ size_t spki_len; @@ -143,8 +144,11 @@ read_spki(struct line_file *lfile, struct tal *tal) return error; } +/** + * @file_name is expected to outlive @result. + */ int -tal_load(const char *file_name, struct tal **result) +tal_load(char const *file_name, struct tal **result) { struct line_file *lfile; struct tal *tal; @@ -160,6 +164,8 @@ tal_load(const char *file_name, struct tal **result) goto fail3; } + tal->file_name = file_name; + error = uris_init(&tal->uris); if (error) goto fail2; @@ -241,6 +247,12 @@ tal_shuffle_uris(struct tal *tal) } } +char const * +tal_get_file_name(struct tal *tal) +{ + return tal->file_name; +} + void tal_get_spki(struct tal *tal, unsigned char const **buffer, size_t *len) { diff --git a/src/object/tal.h b/src/object/tal.h index 87a3e579..e092989f 100644 --- a/src/object/tal.h +++ b/src/object/tal.h @@ -8,13 +8,14 @@ struct tal; -int tal_load(const char *, struct tal **); +int tal_load(char const *, struct tal **); void tal_destroy(struct tal *); typedef int (*foreach_uri_cb)(struct tal *, struct rpki_uri const *); int foreach_uri(struct tal *, foreach_uri_cb); void tal_shuffle_uris(struct tal *); +char const *tal_get_file_name(struct tal *); void tal_get_spki(struct tal *, unsigned char const **, size_t *); #endif /* TAL_OBJECT_H_ */ diff --git a/src/resource.c b/src/resource.c index b637e41b..5366f6ed 100644 --- a/src/resource.c +++ b/src/resource.c @@ -86,26 +86,24 @@ inherit_aors(struct resources *resources, int family) parent = get_parent_resources(); if (parent == NULL) - return pr_err("Certificate inherits IP resources, but parent does not define any resources."); + return pr_crit("Parent has no resources."); switch (family) { case AF_INET: if (resources->ip4s != NULL) return pr_err("Certificate inherits IPv4 resources while also defining others of its own."); - if (parent->ip4s == NULL) - return pr_err("Certificate inherits IPv4 resources from parent, but parent lacks IPv4 resources."); resources->ip4s = parent->ip4s; - res4_get(resources->ip4s); + if (resources->ip4s != NULL) + res4_get(resources->ip4s); pr_debug(""); return 0; case AF_INET6: if (resources->ip6s != NULL) return pr_err("Certificate inherits IPv6 resources while also defining others of its own."); - if (parent->ip6s == NULL) - return pr_err("Certificate inherits IPv6 resources from parent, but parent lacks IPv6 resources."); resources->ip6s = parent->ip6s; - res6_get(resources->ip6s); + if (resources->ip6s != NULL) + res6_get(resources->ip6s); pr_debug(""); return 0; } @@ -388,15 +386,14 @@ inherit_asiors(struct resources *resources) parent = get_parent_resources(); if (parent == NULL) - return pr_err("Certificate inherits ASN resources, but parent does not define any resources."); + return pr_crit("Parent has no resources."); if (resources->asns != NULL) return pr_err("Certificate inherits ASN resources while also defining others of its own."); - if (parent->asns == NULL) - return pr_err("Certificate inherits ASN resources from parent, but parent lacks ASN resources."); resources->asns = parent->asns; - rasn_get(resources->asns); + if (resources->asns != NULL) + rasn_get(resources->asns); pr_debug(""); return 0; } diff --git a/src/rsync/rsync.c b/src/rsync/rsync.c index a0770ba1..38583265 100644 --- a/src/rsync/rsync.c +++ b/src/rsync/rsync.c @@ -86,8 +86,10 @@ do_rsync(char const *rsync_uri, char const *localuri) pr_debug("(%s) command = %s", __func__, command); /* - * TODO Improve the execution of the command, maybe using another - * function instead of system(). + * TODO (next iteration) system(3): "Do not use system() from a + * privileged program" + * I don't think there's a reason to run this program with privileges, + * but consider using exec(3) instead. */ error = system(command); if (error) { diff --git a/src/state.c b/src/state.c index a8b94650..7ee41837 100644 --- a/src/state.c +++ b/src/state.c @@ -375,7 +375,8 @@ validation_store_subject(struct validation *state, char *subject) ARRAYLIST_FOREACH(&cert->subjects, cursor) if (strcmp(*cursor, subject) == 0) - return pr_err("Subject name is not unique."); + return pr_err("Subject name '%s' is not unique.", + subject); duplicate = strdup(subject); if (duplicate == NULL) diff --git a/src/thread_var.h b/src/thread_var.h index 868cc6ea..074148a7 100644 --- a/src/thread_var.h +++ b/src/thread_var.h @@ -13,7 +13,6 @@ void fnstack_push(char const *); char const *fnstack_peek(void); void fnstack_pop(void); -/* TODO use these more, to print better error messages. */ char const *v4addr2str(struct in_addr *addr); char const *v4addr2str2(struct in_addr *addr); char const *v6addr2str(struct in6_addr *addr); -- 2.47.3