From: Alberto Leiva Popper Date: Wed, 24 Jul 2019 19:31:51 +0000 (-0500) Subject: Reimplement the fix to bug #11 X-Git-Tag: v1.0.0^2~16 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=76135f0ac7cc7a281e0ae48408c9fcd75aef5932;p=thirdparty%2FFORT-validator.git Reimplement the fix to bug #11 I was uncomfortable with the previous solution for two reasons: - It wasn't deferring certificate revocation validation to libcrypto. I am not sure if our implementation of it was sufficient, but regardless, this operation should not be performed by Fort itself. - It induced redundant CRL loading, which was a little unnecessarily slow. The root of the problem was that Fort was (originally) trying to validate manifests' certificates using their grandparents' CRL (rather than the parents'), which was incorrect and now fixed. --- diff --git a/src/asn1/signed_data.c b/src/asn1/signed_data.c index d4a1a7ef..cf5a90ee 100644 --- a/src/asn1/signed_data.c +++ b/src/asn1/signed_data.c @@ -90,6 +90,8 @@ handle_sdata_certificate(ANY_t *cert_encoded, struct signed_object_args *args, goto end1; } + x509_name_pr_debug("Issuer", X509_get_issuer_name(cert)); + error = certificate_validate_chain(cert, args->crls); if (error) goto end2; @@ -443,37 +445,38 @@ release_sdata_pkcs7: } int -signed_data_decode(ANY_t *coded, struct signed_object_args *args, - struct SignedData **result) +signed_data_decode(struct signed_data *sdata, ANY_t *coded) { - struct SignedData *sdata; int error; + sdata->encoded = coded; + /* 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, - false); + error = asn1_decode_any(coded, &asn_DEF_SignedData, + (void **) &sdata->decoded, false); if (error) { /* Try to decode as PKCS content (RFC 5652 section 5.2.1) */ - error = signed_data_decode_pkcs7(coded, &sdata); - if (error) - return (error); + error = signed_data_decode_pkcs7(coded, &sdata->decoded); } - error = validate(sdata, coded, args); - if (error) { - signed_data_free(sdata); - return error; - } + return error; +} - *result = sdata; - return 0; +int +signed_data_validate(struct signed_data *sdata, struct signed_object_args *args) +{ + /* + * TODO (fine) maybe collapse this wrapper, + * since there's no point to it anymore. + */ + return validate(sdata->decoded, sdata->encoded, args); } void -signed_data_free(struct SignedData *sdata) +signed_data_cleanup(struct signed_data *sdata) { - ASN_STRUCT_FREE(asn_DEF_SignedData, sdata); + ASN_STRUCT_FREE(asn_DEF_SignedData, sdata->decoded); } /* Caller must free *@result. */ diff --git a/src/asn1/signed_data.h b/src/asn1/signed_data.h index ed0e5dbd..d627def8 100644 --- a/src/asn1/signed_data.h +++ b/src/asn1/signed_data.h @@ -10,6 +10,8 @@ /* * This only exists to reduce argument lists. + * TODO (fine) rename to signed_data_args, since it has nothing to do with + * signed objects anymore. */ struct signed_object_args { /** Location of the signed object. */ @@ -29,9 +31,14 @@ int signed_object_args_init(struct signed_object_args *, struct rpki_uri *, STACK_OF(X509_CRL) *, bool); 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 *); +struct signed_data { + ANY_t *encoded; + struct SignedData *decoded; +}; + +int signed_data_decode(struct signed_data *, ANY_t *); +int signed_data_validate(struct signed_data *, struct signed_object_args *); +void signed_data_cleanup(struct signed_data *); int get_content_type_attr(struct SignedData *, OBJECT_IDENTIFIER_t **); diff --git a/src/object/certificate.c b/src/object/certificate.c index 37bc077c..4e983296 100644 --- a/src/object/certificate.c +++ b/src/object/certificate.c @@ -87,11 +87,15 @@ validate_issuer(X509 *cert, bool is_ta) if (!is_ta) return validate_issuer_name("Certificate", issuer); + /* TODO wait. Shouldn't we check subject == issuer? */ + error = x509_name_decode(issuer, "issuer", &name); - if (!error) - x509_name_put(name); + if (error) + return error; + pr_debug("Issuer: %s", x509_name_commonName(name)); - return error; + x509_name_put(name); + return 0; } static int @@ -108,6 +112,7 @@ validate_subject(X509 *cert) error = x509_name_decode(X509_get_subject_name(cert), "subject", &name); if (error) return error; + pr_debug("Subject: %s", x509_name_commonName(name)); error = x509stack_store_subject(validation_certstack(state), name); @@ -1485,7 +1490,7 @@ certificate_traverse(struct rpp *rpp_parent, struct rpki_uri *cert_uri) */ mft_retry = true; do { - error = handle_manifest(mft, rpp_parent_crl, &pp); + error = handle_manifest(mft, &pp); if (!mft_retry) uri_refput(mft); if (!error || !mft_retry) diff --git a/src/object/ghostbusters.c b/src/object/ghostbusters.c index 42f82ca0..5dabb963 100644 --- a/src/object/ghostbusters.c +++ b/src/object/ghostbusters.c @@ -7,9 +7,11 @@ #include "vcard.h" static int -handle_vcard(OCTET_STRING_t *vcard, void *arg) +handle_vcard(struct signed_object *sobj) { - return handle_ghostbusters_vcard(vcard); + return handle_ghostbusters_vcard( + sobj->sdata.decoded->encapContentInfo.eContent + ); } int @@ -17,30 +19,42 @@ ghostbusters_traverse(struct rpki_uri *uri, struct rpp *pp) { static OID oid = OID_GHOSTBUSTERS; struct oid_arcs arcs = OID2ARCS("ghostbusters", oid); + struct signed_object sobj; struct signed_object_args sobj_args; STACK_OF(X509_CRL) *crl; int error; + /* Prepare */ pr_debug_add("Ghostbusters '%s' {", uri_get_printable(uri)); fnstack_push_uri(uri); - error = rpp_crl(pp, &crl); + /* Decode */ + error = signed_object_decode(&sobj, uri); if (error) - goto end1; + goto revert_log; + /* Prepare validation arguments */ + error = rpp_crl(pp, &crl); + if (error) + goto revert_sobj; error = signed_object_args_init(&sobj_args, uri, crl, true); if (error) - goto end1; + goto revert_sobj; - error = signed_object_decode(&sobj_args, &arcs, handle_vcard, NULL); + /* Validate everything */ + error = signed_object_validate(&sobj, &arcs, &sobj_args); if (error) - goto end2; - + goto revert_args; + error = handle_vcard(&sobj); + if (error) + goto revert_args; error = refs_validate_ee(&sobj_args.refs, pp, sobj_args.uri); -end2: +revert_args: signed_object_args_cleanup(&sobj_args); -end1: +revert_sobj: + signed_object_cleanup(&sobj); +revert_log: pr_debug_rm("}"); fnstack_pop(); return error; diff --git a/src/object/manifest.c b/src/object/manifest.c index 9515ce7f..78069256 100644 --- a/src/object/manifest.c +++ b/src/object/manifest.c @@ -16,9 +16,14 @@ #include "object/signed_object.h" static int -manifest_decode(OCTET_STRING_t *string, void *arg) +decode_manifest(struct signed_object *sobj, struct Manifest **result) { - return asn1_decode_octet_string(string, &asn_DEF_Manifest, arg, true); + return asn1_decode_octet_string( + sobj->sdata.decoded->encapContentInfo.eContent, + &asn_DEF_Manifest, + (void **) result, + true + ); } static int @@ -141,8 +146,7 @@ validate_manifest(struct Manifest *manifest) } static int -__handle_manifest(struct Manifest *mft, struct rpki_uri *mft_uri, - struct rpp **pp) +build_rpp(struct Manifest *mft, struct rpki_uri *mft_uri, struct rpp **pp) { int i; struct FileAndHash *fah; @@ -206,41 +210,65 @@ fail: * @pp. */ int -handle_manifest(struct rpki_uri *uri, STACK_OF(X509_CRL) *crls, struct rpp **pp) +handle_manifest(struct rpki_uri *uri, struct rpp **pp) { static OID oid = OID_MANIFEST; struct oid_arcs arcs = OID2ARCS("manifest", oid); + struct signed_object sobj; struct signed_object_args sobj_args; struct Manifest *mft; + STACK_OF(X509_CRL) *crl; int error; + /* Prepare */ pr_debug_add("Manifest '%s' {", uri_get_printable(uri)); fnstack_push_uri(uri); - error = signed_object_args_init(&sobj_args, uri, crls, false); + /* Decode */ + error = signed_object_decode(&sobj, uri); if (error) - goto end1; + goto revert_log; + error = decode_manifest(&sobj, &mft); + if (error) + goto revert_sobj; - error = signed_object_decode(&sobj_args, &arcs, manifest_decode, &mft); + /* Initialize out parameter (@pp) */ + error = build_rpp(mft, uri, pp); if (error) - goto end2; + goto revert_manifest; - error = validate_manifest(mft); + /* Prepare validation arguments */ + error = rpp_crl(*pp, &crl); if (error) - goto end3; - error = __handle_manifest(mft, uri, pp); + goto revert_rpp; + error = signed_object_args_init(&sobj_args, uri, crl, false); if (error) - goto end3; + goto revert_rpp; + /* Validate everything */ + error = signed_object_validate(&sobj, &arcs, &sobj_args); + if (error) + goto revert_args; + error = validate_manifest(mft); + if (error) + goto revert_args; error = refs_validate_ee(&sobj_args.refs, *pp, uri); if (error) - rpp_refput(*pp); + goto revert_args; -end3: - ASN_STRUCT_FREE(asn_DEF_Manifest, mft); -end2: + /* Success */ + signed_object_args_cleanup(&sobj_args); + goto revert_manifest; + +revert_args: signed_object_args_cleanup(&sobj_args); -end1: +revert_rpp: + rpp_refput(*pp); +revert_manifest: + ASN_STRUCT_FREE(asn_DEF_Manifest, mft); +revert_sobj: + signed_object_cleanup(&sobj); +revert_log: pr_debug_rm("}"); fnstack_pop(); return error; diff --git a/src/object/manifest.h b/src/object/manifest.h index ded148c0..a9cfcb99 100644 --- a/src/object/manifest.h +++ b/src/object/manifest.h @@ -1,10 +1,9 @@ #ifndef SRC_OBJECT_MANIFEST_H_ #define SRC_OBJECT_MANIFEST_H_ -#include #include "uri.h" #include "rpp.h" -int handle_manifest(struct rpki_uri *, STACK_OF(X509_CRL) *, struct rpp **); +int handle_manifest(struct rpki_uri *, struct rpp **); #endif /* SRC_OBJECT_MANIFEST_H_ */ diff --git a/src/object/name.c b/src/object/name.c index d800fde2..a5dc7f02 100644 --- a/src/object/name.c +++ b/src/object/name.c @@ -174,6 +174,7 @@ validate_issuer_name(char const *container, X509_NAME *issuer) error = x509_name_decode(issuer, "issuer", &child_issuer); if (error) goto end; + pr_debug("Issuer: %s", child_issuer->commonName); if (!x509_name_equals(parent_subject, child_issuer)) { char const *parent_serial; @@ -196,3 +197,24 @@ validate_issuer_name(char const *container, X509_NAME *issuer) end: x509_name_put(parent_subject); return error; } + +#ifdef DEBUG + +void +x509_name_pr_debug(const char *prefix, X509_NAME *name) +{ + struct rfc5280_name *printable; + + if (name == NULL) { + pr_debug("%s: (null)", prefix); + return; + } + + if (x509_name_decode(name, prefix, &printable) != 0) + return; /* Error message already printed */ + + pr_debug("%s: %s", prefix, printable->commonName); + x509_name_put(printable); +} + +#endif diff --git a/src/object/name.h b/src/object/name.h index 85d487e0..a1a070d5 100644 --- a/src/object/name.h +++ b/src/object/name.h @@ -18,6 +18,14 @@ char const *x509_name_serialNumber(struct rfc5280_name *); bool x509_name_equals(struct rfc5280_name *, struct rfc5280_name *); + +/* X509_NAME utils */ int validate_issuer_name(char const *, X509_NAME *); +#ifdef DEBUG +void x509_name_pr_debug(char const *, X509_NAME *); +#else +#define x509_name_pr_debug(a, b) /* Nothing */ +#endif + #endif /* SRC_OBJECT_NAME_H_ */ diff --git a/src/object/roa.c b/src/object/roa.c index 30be931c..71024026 100644 --- a/src/object/roa.c +++ b/src/object/roa.c @@ -13,10 +13,14 @@ #include "object/signed_object.h" static int -roa_decode(OCTET_STRING_t *string, void *arg) +decode_roa(struct signed_object *sobj, struct RouteOriginAttestation **result) { - return asn1_decode_octet_string(string, &asn_DEF_RouteOriginAttestation, - arg, true); + return asn1_decode_octet_string( + sobj->sdata.decoded->encapContentInfo.eContent, + &asn_DEF_RouteOriginAttestation, + (void **) result, + true + ); } static int @@ -151,7 +155,6 @@ __handle_roa(struct RouteOriginAttestation *roa, struct resources *parent) int a; int error; - pr_debug_add("eContent {"); if (roa->version != NULL) { error = asn_INTEGER2ulong(roa->version, &version); @@ -244,37 +247,48 @@ roa_traverse(struct rpki_uri *uri, struct rpp *pp) { static OID oid = OID_ROA; struct oid_arcs arcs = OID2ARCS("roa", oid); + struct signed_object sobj; struct signed_object_args sobj_args; struct RouteOriginAttestation *roa; STACK_OF(X509_CRL) *crl; int error; + /* Prepare */ pr_debug_add("ROA '%s' {", uri_get_printable(uri)); fnstack_push_uri(uri); - error = rpp_crl(pp, &crl); - if (error) - goto revert_fnstack; - - error = signed_object_args_init(&sobj_args, uri, crl, false); + /* Decode */ + error = signed_object_decode(&sobj, uri); if (error) - goto revert_fnstack; - - error = signed_object_decode(&sobj_args, &arcs, roa_decode, &roa); + goto revert_log; + error = decode_roa(&sobj, &roa); if (error) goto revert_sobj; - error = refs_validate_ee(&sobj_args.refs, pp, sobj_args.uri); + /* Prepare validation arguments */ + error = rpp_crl(pp, &crl); + if (error) + goto revert_roa; + error = signed_object_args_init(&sobj_args, uri, crl, false); if (error) goto revert_roa; + /* Validate and handle everything */ + error = signed_object_validate(&sobj, &arcs, &sobj_args); + if (error) + goto revert_args; error = __handle_roa(roa, sobj_args.res); + if (error) + goto revert_args; + error = refs_validate_ee(&sobj_args.refs, pp, sobj_args.uri); +revert_args: + signed_object_args_cleanup(&sobj_args); revert_roa: ASN_STRUCT_FREE(asn_DEF_RouteOriginAttestation, roa); revert_sobj: - signed_object_args_cleanup(&sobj_args); -revert_fnstack: + signed_object_cleanup(&sobj); +revert_log: fnstack_pop(); pr_debug_rm("}"); return error; diff --git a/src/object/signed_object.c b/src/object/signed_object.c index e7949d7a..25d17bd2 100644 --- a/src/object/signed_object.c +++ b/src/object/signed_object.c @@ -4,6 +4,24 @@ #include "log.h" #include "asn1/content_info.h" +int +signed_object_decode(struct signed_object *sobj, struct rpki_uri *uri) +{ + int error; + + error = content_info_load(uri, &sobj->cinfo); + if (error) + return error; + + error = signed_data_decode(&sobj->sdata, &sobj->cinfo->content); + if (error) { + content_info_free(sobj->cinfo); + return error; + } + + return 0; +} + static int validate_eContentType(struct SignedData *sdata, struct oid_arcs const *oid) { @@ -50,39 +68,30 @@ validate_content_type(struct SignedData *sdata, struct oid_arcs const *oid) } int -signed_object_decode(struct signed_object_args *args, - struct oid_arcs const *oid, - signed_object_cb cb, - void *cb_arg) +signed_object_validate(struct signed_object *sobj, struct oid_arcs const *oid, + struct signed_object_args *args) { - struct ContentInfo *cinfo; - struct SignedData *sdata; int error; - error = content_info_load(args->uri, &cinfo); - if (error) - goto end1; - - error = signed_data_decode(&cinfo->content, args, &sdata); - if (error) - goto end2; - /* rfc6482#section-2 */ /* rfc6486#section-4.1 */ /* rfc6486#section-4.4.1 */ - error = validate_eContentType(sdata, oid); + error = validate_eContentType(sobj->sdata.decoded, oid); if (error) - goto end3; + return error; /* rfc6482#section-2 */ /* rfc6486#section-4.3 */ - error = validate_content_type(sdata, oid); + error = validate_content_type(sobj->sdata.decoded, oid); if (error) - goto end3; + return error; - error = cb(sdata->encapContentInfo.eContent, cb_arg); + return signed_data_validate(&sobj->sdata, args); +} -end3: signed_data_free(sdata); -end2: content_info_free(cinfo); -end1: return error; +void +signed_object_cleanup(struct signed_object *sobj) +{ + content_info_free(sobj->cinfo); + signed_data_cleanup(&sobj->sdata); } diff --git a/src/object/signed_object.h b/src/object/signed_object.h index 41dc5586..41404d74 100644 --- a/src/object/signed_object.h +++ b/src/object/signed_object.h @@ -1,13 +1,17 @@ #ifndef SRC_OBJECT_SIGNED_OBJECT_H_ #define SRC_OBJECT_SIGNED_OBJECT_H_ -#include #include "asn1/oid.h" #include "asn1/signed_data.h" -typedef int (*signed_object_cb)(OCTET_STRING_t *, void *); +struct signed_object { + struct ContentInfo *cinfo; + struct signed_data sdata; +}; -int signed_object_decode(struct signed_object_args *, struct oid_arcs const *, - signed_object_cb, void *); +int signed_object_decode(struct signed_object *, struct rpki_uri *); +int signed_object_validate(struct signed_object *, struct oid_arcs const *, + struct signed_object_args *); +void signed_object_cleanup(struct signed_object *); #endif /* SRC_OBJECT_SIGNED_OBJECT_H_ */ diff --git a/src/state.c b/src/state.c index b136bdc2..4fb3e77d 100644 --- a/src/state.c +++ b/src/state.c @@ -79,6 +79,7 @@ validation_prepare(struct validation **out, struct tal *tal, struct validation_handler *validation_handler) { struct validation *result; + X509_VERIFY_PARAM *params; int error; result = malloc(sizeof(struct validation)); @@ -97,6 +98,14 @@ validation_prepare(struct validation **out, struct tal *tal, goto abort1; } + params = X509_VERIFY_PARAM_new(); + if (params == NULL) { + error = pr_enomem(); + goto abort2; + } + + X509_VERIFY_PARAM_set_flags(params, X509_V_FLAG_CRL_CHECK); + X509_STORE_set1_param(result->store, params); X509_STORE_set_verify_cb(result->store, cb); error = certstack_create(&result->certstack);