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 d4a1a7ef53fbf243b3d722ec16a04eb4000ebe32..cf5a90ee5afa4eee4e0fd4ecdcf24e1d4970c62a 100644 (file)
--- 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 ed0e5dbd30241cd175cc95223269d9263e34b8ea..d627def8ed60e7c52d347c8dc58b1d2cfbb014cf 100644 (file)
--- 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 37bc077c3a7dcf1ba404f06e11923c29db7b8c1c..4e983296652f3ab0e5b0ee3b2a436d6682e99bce 100644 (file)
--- 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 42f82ca0fffdd9fdddb815588f3d69e9ab82db61..5dabb9633ad42f0b9b3bff671ad1ca5d5576b61b 100644 (file)
--- 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 9515ce7f462e5ac722100f2de287e3388b76432b..7806925629c825588aabe5405fc95afb821e9678 100644 (file)
--- 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 ded148c04866700aaeef5c98db7eb392628f6120..a9cfcb992c972271be1678bca57d365ce18c7216 100644 (file)
--- 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 <openssl/x509.h>
 #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 d800fde2951802e6f0ccaac0dc5ad755ae5978ea..a5dc7f0205b39c48eee510d7a768d1ed8c2d6e41 100644 (file)
--- 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 85d487e073742c2789f7bbc40e66fde011cc67be..a1a070d59cf07e6aaf0f640900176cb8d99eaef2 100644 (file)
--- 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 30be931c80f9c3a96b7770feb289928bb6bbcf69..71024026844eb3cdae668430d22b0d2c85434b98 100644 (file)
--- 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 e7949d7aae422339dda37d0ee5e486a69bf2686e..25d17bd2fe147325d42db42eb7ed3f6ba54a3077 100644 (file)
--- 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 41dc55867396765dfd27d536b5d228edbfdc5109..41404d74e30cee402dd0b792f348385d1236a742 100644 (file)
--- 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 <openssl/x509.h>
 #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 b136bdc2f52ffeb3c19ed17eb4ab3245a4f8121b..4fb3e77d8e554bc7cf31ca78e3baf38d8dc96214 100644 (file)
--- 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);