From: Alberto Leiva Popper <ydahhrk@gmail.com>
Date: Thu, 27 Jun 2019 17:45:29 +0000 (-0500)
Subject: Patch bad initialization of CRL stack
X-Git-Tag: v1.0.0~3
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=897e06df1a79b4f9a408875706ae62b5e33a9bed;p=thirdparty%2FFORT-validator.git

Patch bad initialization of CRL stack
---

diff --git a/docs/doc/installation.md b/docs/doc/installation.md
index 06be6785..6dd3ee74 100644
--- a/docs/doc/installation.md
+++ b/docs/doc/installation.md
@@ -6,8 +6,6 @@ title: Compilation and Installation
 
 # {{ page.title }}
 
-> TODO update with proper .tar.gz releases, once they are created
-
 ## Index
 
 1. [Dependencies](#dependencies)
diff --git a/src/object/certificate.c b/src/object/certificate.c
index bbffe29a..e3600c69 100644
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -1386,6 +1386,7 @@ certificate_traverse(struct rpp *rpp_parent, struct rpki_uri *cert_uri)
 
 	struct validation *state;
 	int total_parents;
+	STACK_OF(X509_CRL) *rpp_parent_crl;
 	X509 *cert;
 	struct rpki_uri *mft;
 	struct certificate_refs refs;
@@ -1405,11 +1406,15 @@ certificate_traverse(struct rpp *rpp_parent, struct rpki_uri *cert_uri)
 	fnstack_push_uri(cert_uri);
 	memset(&refs, 0, sizeof(refs));
 
+	error = rpp_crl(rpp_parent, &rpp_parent_crl);
+	if (error)
+		goto revert_fnstack_and_debug;
+
 	/* -- Validate the certificate (@cert) -- */
 	error = certificate_load(cert_uri, &cert);
 	if (error)
 		goto revert_fnstack_and_debug;
-	error = certificate_validate_chain(cert, rpp_crl(rpp_parent));
+	error = certificate_validate_chain(cert, rpp_parent_crl);
 	if (error)
 		goto revert_cert;
 	error = certificate_validate_rfc6487(cert, IS_TA);
@@ -1432,7 +1437,7 @@ certificate_traverse(struct rpp *rpp_parent, struct rpki_uri *cert_uri)
 		goto revert_uri_and_refs;
 	cert = NULL; /* Ownership stolen */
 
-	error = handle_manifest(mft, rpp_crl(rpp_parent), &pp);
+	error = handle_manifest(mft, rpp_parent_crl, &pp);
 	if (error) {
 		x509stack_cancel(validation_certstack(state));
 		goto revert_uri_and_refs;
diff --git a/src/object/ghostbusters.c b/src/object/ghostbusters.c
index 9ace5baa..42f82ca0 100644
--- a/src/object/ghostbusters.c
+++ b/src/object/ghostbusters.c
@@ -18,12 +18,17 @@ ghostbusters_traverse(struct rpki_uri *uri, struct rpp *pp)
 	static OID oid = OID_GHOSTBUSTERS;
 	struct oid_arcs arcs = OID2ARCS("ghostbusters", oid);
 	struct signed_object_args sobj_args;
+	STACK_OF(X509_CRL) *crl;
 	int error;
 
 	pr_debug_add("Ghostbusters '%s' {", uri_get_printable(uri));
 	fnstack_push_uri(uri);
 
-	error = signed_object_args_init(&sobj_args, uri, rpp_crl(pp), true);
+	error = rpp_crl(pp, &crl);
+	if (error)
+		goto end1;
+
+	error = signed_object_args_init(&sobj_args, uri, crl, true);
 	if (error)
 		goto end1;
 
diff --git a/src/object/roa.c b/src/object/roa.c
index 10919eb1..bc386265 100644
--- a/src/object/roa.c
+++ b/src/object/roa.c
@@ -191,12 +191,17 @@ roa_traverse(struct rpki_uri *uri, struct rpp *pp)
 	struct oid_arcs arcs = OID2ARCS("roa", oid);
 	struct signed_object_args sobj_args;
 	struct RouteOriginAttestation *roa;
+	STACK_OF(X509_CRL) *crl;
 	int error;
 
 	pr_debug_add("ROA '%s' {", uri_get_printable(uri));
 	fnstack_push_uri(uri);
 
-	error = signed_object_args_init(&sobj_args, uri, rpp_crl(pp), false);
+	error = rpp_crl(pp, &crl);
+	if (error)
+		goto revert_fnstack;
+
+	error = signed_object_args_init(&sobj_args, uri, crl, false);
 	if (error)
 		goto revert_fnstack;
 
diff --git a/src/rpp.c b/src/rpp.c
index 50160bbe..4f1b5a27 100644
--- a/src/rpp.c
+++ b/src/rpp.c
@@ -145,25 +145,54 @@ end:
 	return error;
 }
 
-STACK_OF(X509_CRL) *
-rpp_crl(struct rpp *pp)
+/**
+ * Returns the pp's CRL in stack form (which is how libcrypto functions want
+ * it).
+ * The stack belongs to @pp and should not be released. Can be NULL, in which
+ * case you're currently validating the TA (since it lacks governing CRL).
+ */
+int
+rpp_crl(struct rpp *pp, STACK_OF(X509_CRL) **result)
 {
-	if (pp == NULL)
-		return NULL;
-	if (pp->crl == NULL)
-		return NULL;
-	if (pp->crl_stack != NULL)
-		return pp->crl_stack;
+	STACK_OF(X509_CRL) *stack;
+	int error;
 
-	pp->crl_stack = sk_X509_CRL_new_null();
-	if (pp->crl_stack == NULL)
-		return NULL;
-	if (add_crl_to_stack(pp, pp->crl_stack) != 0) {
-		sk_X509_CRL_pop_free(pp->crl_stack, X509_CRL_free);
-		return NULL;
+	if (pp == NULL) {
+		/* No pp = currently validating TA. There's no CRL. */
+		*result = NULL;
+		return 0;
+	}
+	if (pp->crl == NULL) {
+		/* rpp_crl() assumes the rpp has been populated already. */
+		pr_crit("RPP lacks a CRL.");
+	}
+	if (pp->crl_stack != NULL) {
+		/* Result already cached. */
+		*result = pp->crl_stack;
+		return 0;
+	}
+
+	/*
+	 * TODO (performance) ghostbusters_traverse() and roa_traverse() can
+	 * call rpp_crl() repeatedly, which means that, if it errors here,
+	 * it will keep trying to recreate the stack and log the same error
+	 * over and over.
+	 * Consider creating a flag which will keep track of the CRL cache
+	 * status, and prevent this code if it knows it's going to fail.
+	 * Or maybe some other solution.
+	 */
+	stack = sk_X509_CRL_new_null();
+	if (stack == NULL)
+		return pr_enomem();
+	error = add_crl_to_stack(pp, stack);
+	if (error) {
+		sk_X509_CRL_pop_free(stack, X509_CRL_free);
+		return error;
 	}
 
-	return pp->crl_stack;
+	pp->crl_stack = stack; /* Cache result */
+	*result = stack;
+	return 0;
 }
 
 static int
diff --git a/src/rpp.h b/src/rpp.h
index 3b310c24..546ad684 100644
--- a/src/rpp.h
+++ b/src/rpp.h
@@ -15,7 +15,7 @@ int rpp_add_roa(struct rpp *, struct rpki_uri *);
 int rpp_add_ghostbusters(struct rpp *, struct rpki_uri *);
 
 struct rpki_uri *rpp_get_crl(struct rpp const *);
-STACK_OF(X509_CRL) *rpp_crl(struct rpp *);
+int rpp_crl(struct rpp *, STACK_OF(X509_CRL) **);
 
 void rpp_traverse(struct rpp *);