CVE-2021-43114: Other (NVD-CWE-Other)
value: FORT Validator versions prior to 1.5.2 will crash if an RPKI CA publishes an X.509 EE certificate. This will lead to RTR clients such as BGP routers to lose access to the RPKI VRP data set, effectively disabling Route Origin Validation.
CWE description: NVD is only using a subset of CWE for mapping instead of the entire CWE, and the weakness type is not covered by that subset.
CVSS v3 base score: 7.5
Published: 2021-11-09T13:15Z
Repository: https://github.com/NICMx/FORT-validator
Language: C
Files changed: src/cert_stack.c, src/object/certificate.c
Commit date: 2021-09-08 12:43:45 -0500
Commit message: Certificates: Fuse meta and level stacks

These stacks always had the same size, and their corresponding elements
always referred to the same certificate.

This was pending work from #55, which I think is now properly solved.

Also refactors x509stack_push(); was messy. Patched an unlikely memory
leak in the chaos.

Diff: diff --git a/src/cert_stack.c b/src/cert_stack.c
index 69ec10a..4478958 100644
--- a/src/cert_stack.c
+++ b/src/cert_stack.c
@@ -54,28 +54,23 @@ struct metadata_node {
 	 * don't have many children, and I'm running out of time.
 	 */
 	struct serial_numbers serials;
 	struct subjects subjects;
 
+	/*
+	 * Certificate repository "level". This aims to identify if the
+	 * certificate is located at a distinct server than its father (common
+	 * case when the RIRs delegate RPKI repositories).
+	 */
+	unsigned int level;
+
 	/** Used by certstack. Points to the next stacked certificate. */
 	SLIST_ENTRY(metadata_node) next;
 };
 
 SLIST_HEAD(metadata_stack, metadata_node);
 
-/**
- * Certificate repository "level". This aims to identify if the
- * certificate is located at a distinct server than its father (common
- * case when the RIRs delegate RPKI repositories).
- */
-struct repo_level_node {
-	unsigned int level;
-	SLIST_ENTRY(repo_level_node) next;
-};
-
-SLIST_HEAD(repo_level_stack, repo_level_node);
-
 /**
  * This is the foundation through which we pull off our iterative traversal,
  * as opposed to a stack-threatening recursive one.
  *
  * It is a bunch of data that replaces the one that would normally be allocated
@@ -107,16 +102,10 @@ struct cert_stack {
 	 * (This is a SLIST and not a STACK_OF because the OpenSSL stack
 	 * implementation is different than the LibreSSL one, and the latter is
 	 * seemingly not intended to be used outside of its library.)
 	 */
 	struct metadata_stack metas;
-
-	/**
-	 * Stacked data to store the repository "levels" (each level is a
-	 * delegation of an RPKI server).
-	 */
-	struct repo_level_stack levels;
 };
 
 int
 certstack_create(struct cert_stack **result)
 {
@@ -132,11 +121,10 @@ certstack_create(struct cert_stack **result)
 		return val_crypto_err("sk_X509_new_null() returned NULL");
 	}
 
 	SLIST_INIT(&stack->defers);
 	SLIST_INIT(&stack->metas);
-	SLIST_INIT(&stack->levels);
 
 	*result = stack;
 	return 0;
 }
 
@@ -183,11 +171,10 @@ void
 certstack_destroy(struct cert_stack *stack)
 {
 	unsigned int stack_size;
 	struct metadata_node *meta;
 	struct defer_node *post;
-	struct repo_level_node *level;
 
 	stack_size = 0;
 	while (!SLIST_EMPTY(&stack->defers)) {
 		post = SLIST_FIRST(&stack->defers);
 		SLIST_REMOVE_HEAD(&stack->defers, next);
@@ -206,19 +193,10 @@ certstack_destroy(struct cert_stack *stack)
 		meta_destroy(meta);
 		stack_size++;
 	}
 	pr_val_debug("Deleted %u metadatas.", stack_size);
 
-	stack_size = 0;
-	while (!SLIST_EMPTY(&stack->levels)) {
-		level = SLIST_FIRST(&stack->levels);
-		SLIST_REMOVE_HEAD(&stack->levels, next);
-		free(level);
-		stack_size++;
-	}
-	pr_val_debug("Deleted %u stacked levels.", stack_size);
-
 	free(stack);
 }
 
 int
 deferstack_push(struct cert_stack *stack, struct deferred_cert *deferred)
@@ -240,11 +218,10 @@ deferstack_push(struct cert_stack *stack, struct deferred_cert *deferred)
 static void
 x509stack_pop(struct cert_stack *stack)
 {
 	X509 *cert;
 	struct metadata_node *meta;
-	struct repo_level_node *repo;
 
 	cert = sk_X509_pop(stack->x509s);
 	if (cert == NULL)
 		pr_crit("Attempted to pop empty X509 stack");
 	X509_free(cert);
@@ -252,16 +229,10 @@ x509stack_pop(struct cert_stack *stack)
 	meta = SLIST_FIRST(&stack->metas);
 	if (meta == NULL)
 		pr_crit("Attempted to pop empty metadata stack");
 	SLIST_REMOVE_HEAD(&stack->metas, next);
 	meta_destroy(meta);
-
-	repo = SLIST_FIRST(&stack->levels);
-	if (repo == NULL)
-		pr_crit("Attempted to pop empty repo level stack");
-	SLIST_REMOVE_HEAD(&stack->levels, next);
-	free(repo);
 }
 
 /**
  * Contract: Returns either 0 or -ENOENT. No other outcomes.
  */
@@ -295,93 +266,135 @@ bool
 deferstack_is_empty(struct cert_stack *stack)
 {
 	return SLIST_EMPTY(&stack->defers);
 }
 
+static int
+init_resources(X509 *x509, enum rpki_policy policy, enum cert_type type,
+    struct resources **_result)
+{
+	struct resources *result;
+	int error;
+
+	result = resources_create(false);
+	if (result == NULL)
+		return pr_enomem();
+
+	resources_set_policy(result, policy);
+	error = certificate_get_resources(x509, result, type);
+	if (error)
+		goto fail;
+
+	/*
+	 * rfc8630#section-2.3
+	 * "The INR extension(s) of this TA MUST contain a non-empty set of
+	 * number resources."
+	 * The "It MUST NOT use the "inherit" form of the INR extension(s)"
+	 * part is already handled in certificate_get_resources().
+	 */
+	if (type == TA && resources_empty(result)) {
+		error = pr_val_err("Trust Anchor certificate does not define any number resources.");
+		goto fail;
+	}
+
+	*_result = result;
+	return 0;
+
+fail:
+	resources_destroy(result);
+	return error;
+}
+
+static int
+init_level(struct cert_stack *stack, unsigned int *_result)
+{
+	struct metadata_node *head_meta;
+	unsigned int work_repo_level;
+	unsigned int result;
+
+	/*
+	 * Bruh, I don't understand the point of this block.
+	 * Why can't it just be `result = working_repo_peek_level();`?
+	 */
+
+	result = 0;
+	work_repo_level = working_repo_peek_level();
+	head_meta = SLIST_FIRST(&stack->metas);
+	if (head_meta != NULL && work_repo_level > head_meta->level)
+		result = work_repo_level;
+
+	*_result = result;
+	return 0;
+}
+
+static struct defer_node *
+create_separator(void)
+{
+	struct defer_node *result;
+
+	result = malloc(sizeof(struct defer_node));
+	if (result == NULL)
+		return NULL;
+
+	result->type = DNT_SEPARATOR;
+	return result;
+}
+
 /** Steals ownership of @x509 on success. */
 int
 x509stack_push(struct cert_stack *stack, struct rpki_uri *uri, X509 *x509,
     enum rpki_policy policy, enum cert_type type)
 {
 	struct metadata_node *meta;
-	struct repo_level_node *repo, *head_repo;
 	struct defer_node *defer_separator;
-	unsigned int work_repo_level;
 	int ok;
 	int error;
 
-	repo = malloc(sizeof(struct repo_level_node));
-	if (repo == NULL)
-		return pr_enomem();
-
-	repo->level = 0;
-	work_repo_level = working_repo_peek_level();
-	head_repo = SLIST_FIRST(&stack->levels);
-	if (head_repo != NULL && work_repo_level > head_repo->level)
-		repo->level = work_repo_level;
-
-	SLIST_INSERT_HEAD(&stack->levels, repo, next);
-
 	meta = malloc(sizeof(struct metadata_node));
-	if (meta == NULL) {
-		error = pr_enomem();
-		goto end3;
-	}
+	if (meta == NULL)
+		return pr_enomem();
 
 	meta->uri = uri;
 	uri_refget(uri);
 	serial_numbers_init(&meta->serials);
 	subjects_init(&meta->subjects);
 
-	meta->resources = resources_create(false);
-	if (meta->resources == NULL) {
-		error = pr_enomem();
-		goto end4;
-	}
-	resources_set_policy(meta->resources, policy);
-	error = certificate_get_resources(x509, meta->resources, type);
+	error = init_resources(x509, policy, type, &meta->resources);
 	if (error)
-		goto end5;
+		goto cleanup_subjects;
 
-	/*
-	 * rfc8630#section-2.3
-	 * "The INR extension(s) of this TA MUST contain a non-empty set of
-	 * number resources."
-	 * The "It MUST NOT use the "inherit" form of the INR extension(s)"
-	 * part is already handled in certificate_get_resources().
-	 */
-	if (type == TA && resources_empty(meta->resources)) {
-		error = pr_val_err("Trust Anchor certificate does not define any number resources.");
-		goto end5;
-	}
+	error = init_level(stack, &meta->level); /* Does not need a revert */
+	if (error)
+		goto destroy_resources;
 
-	defer_separator = malloc(sizeof(struct defer_node));
+	defer_separator = create_separator();
 	if (defer_separator == NULL) {
 		error = pr_enomem();
-		goto end5;
+		goto destroy_resources;
 	}
-	defer_separator->type = DNT_SEPARATOR;
 
 	ok = sk_X509_push(stack->x509s, x509);
 	if (ok <= 0) {
 		error = val_crypto_err(
 		    "Could not add certificate to trusted stack: %d", ok);
-		goto end5;
+		goto destroy_separator;
 	}
 
 	SLIST_INSERT_HEAD(&stack->defers, defer_separator, next);
 	SLIST_INSERT_HEAD(&stack->metas, meta, next);
 
 	return 0;
 
-end5:	resources_destroy(meta->resources);
-end4:	subjects_cleanup(&meta->subjects, subject_cleanup);
+destroy_separator:
+	free(defer_separator);
+destroy_resources:
+	resources_destroy(meta->resources);
+cleanup_subjects:
+	subjects_cleanup(&meta->subjects, subject_cleanup);
 	serial_numbers_cleanup(&meta->serials, serial_cleanup);
 	uri_refput(meta->uri);
 	free(meta);
-end3:	SLIST_REMOVE_HEAD(&stack->levels, next);
-	free(repo);
 	return error;
 }
 
 /**
  * This one is intended to revert a recent x509 push.
@@ -426,12 +439,12 @@ x509stack_peek_resources(struct cert_stack *stack)
 }
 
 unsigned int
 x509stack_peek_level(struct cert_stack *stack)
 {
-	struct repo_level_node *repo = SLIST_FIRST(&stack->levels);
-	return (repo != NULL) ? repo->level : 0;
+	struct metadata_node *meta = SLIST_FIRST(&stack->metas);
+	return (meta != NULL) ? meta->level : 0;
 }
 
 static int
 get_current_file_name(char **_result)
 {
diff --git a/src/object/certificate.c b/src/object/certificate.c
index 9f5fb0d..bd856b1 100644
--- a/src/object/certificate.c
+++ b/src/object/certificate.c
@@ -1150,10 +1150,13 @@ __certificate_get_resources(X509 *cert, struct resources *resources,
 		return pr_val_err("Certificate lacks both IP and AS extension.");
 
 	return 0;
 }
 
+/**
+ * Copies the resources from @cert to @resources.
+ */
 int
 certificate_get_resources(X509 *cert, struct resources *resources,
     enum cert_type type)
 {
 	enum rpki_policy policy;

Vulnerable code: /**
 * Certificate repository "level". This aims to identify if the
 * certificate is located at a distinct server than its father (common
 * case when the RIRs delegate RPKI repositories).
 */
struct repo_level_node {
	unsigned int level;
	SLIST_ENTRY(repo_level_node) next;
};

SLIST_HEAD(repo_level_stack, repo_level_node);


	/**
	 * Stacked data to store the repository "levels" (each level is a
	 * delegation of an RPKI server).
	 */
	struct repo_level_stack levels;
	SLIST_INIT(&stack->levels);
	struct repo_level_node *level;
	stack_size = 0;
	while (!SLIST_EMPTY(&stack->levels)) {
		level = SLIST_FIRST(&stack->levels);
		SLIST_REMOVE_HEAD(&stack->levels, next);
		free(level);
		stack_size++;
	}
	pr_val_debug("Deleted %u stacked levels.", stack_size);

	struct repo_level_node *repo;

	repo = SLIST_FIRST(&stack->levels);
	if (repo == NULL)
		pr_crit("Attempted to pop empty repo level stack");
	SLIST_REMOVE_HEAD(&stack->levels, next);
	free(repo);
	struct repo_level_node *repo, *head_repo;
	unsigned int work_repo_level;
	repo = malloc(sizeof(struct repo_level_node));
	if (repo == NULL)
		return pr_enomem();

	repo->level = 0;
	work_repo_level = working_repo_peek_level();
	head_repo = SLIST_FIRST(&stack->levels);
	if (head_repo != NULL && work_repo_level > head_repo->level)
		repo->level = work_repo_level;

	SLIST_INSERT_HEAD(&stack->levels, repo, next);

	if (meta == NULL) {
		error = pr_enomem();
		goto end3;
	}
	meta->resources = resources_create(false);
	if (meta->resources == NULL) {
		error = pr_enomem();
		goto end4;
	}
	resources_set_policy(meta->resources, policy);
	error = certificate_get_resources(x509, meta->resources, type);
		goto end5;
	/*
	 * rfc8630#section-2.3
	 * "The INR extension(s) of this TA MUST contain a non-empty set of
	 * number resources."
	 * The "It MUST NOT use the "inherit" form of the INR extension(s)"
	 * part is already handled in certificate_get_resources().
	 */
	if (type == TA && resources_empty(meta->resources)) {
		error = pr_val_err("Trust Anchor certificate does not define any number resources.");
		goto end5;
	}
	defer_separator = malloc(sizeof(struct defer_node));
		goto end5;
	defer_separator->type = DNT_SEPARATOR;
		goto end5;
end5:	resources_destroy(meta->resources);
end4:	subjects_cleanup(&meta->subjects, subject_cleanup);
end3:	SLIST_REMOVE_HEAD(&stack->levels, next);
	free(repo);
	struct repo_level_node *repo = SLIST_FIRST(&stack->levels);
	return (repo != NULL) ? repo->level : 0;
Fixed code: 	/*
	 * Certificate repository "level". This aims to identify if the
	 * certificate is located at a distinct server than its father (common
	 * case when the RIRs delegate RPKI repositories).
	 */
	unsigned int level;

static int
init_resources(X509 *x509, enum rpki_policy policy, enum cert_type type,
    struct resources **_result)
{
	struct resources *result;
	int error;

	result = resources_create(false);
	if (result == NULL)
		return pr_enomem();

	resources_set_policy(result, policy);
	error = certificate_get_resources(x509, result, type);
	if (error)
		goto fail;

	/*
	 * rfc8630#section-2.3
	 * "The INR extension(s) of this TA MUST contain a non-empty set of
	 * number resources."
	 * The "It MUST NOT use the "inherit" form of the INR extension(s)"
	 * part is already handled in certificate_get_resources().
	 */
	if (type == TA && resources_empty(result)) {
		error = pr_val_err("Trust Anchor certificate does not define any number resources.");
		goto fail;
	}

	*_result = result;
	return 0;

fail:
	resources_destroy(result);
	return error;
}

static int
init_level(struct cert_stack *stack, unsigned int *_result)
{
	struct metadata_node *head_meta;
	unsigned int work_repo_level;
	unsigned int result;

	/*
	 * Bruh, I don't understand the point of this block.
	 * Why can't it just be `result = working_repo_peek_level();`?
	 */

	result = 0;
	work_repo_level = working_repo_peek_level();
	head_meta = SLIST_FIRST(&stack->metas);
	if (head_meta != NULL && work_repo_level > head_meta->level)
		result = work_repo_level;

	*_result = result;
	return 0;
}

static struct defer_node *
create_separator(void)
{
	struct defer_node *result;

	result = malloc(sizeof(struct defer_node));
	if (result == NULL)
		return NULL;

	result->type = DNT_SEPARATOR;
	return result;
}

	if (meta == NULL)
		return pr_enomem();
	error = init_resources(x509, policy, type, &meta->resources);
		goto cleanup_subjects;
	error = init_level(stack, &meta->level); /* Does not need a revert */
	if (error)
		goto destroy_resources;
	defer_separator = create_separator();
		goto destroy_resources;
		goto destroy_separator;
destroy_separator:
	free(defer_separator);
destroy_resources:
	resources_destroy(meta->resources);
cleanup_subjects:
	subjects_cleanup(&meta->subjects, subject_cleanup);
	struct metadata_node *meta = SLIST_FIRST(&stack->metas);
	return (meta != NULL) ? meta->level : 0;
/**
 * Copies the resources from @cert to @resources.
 */