memory resource leaks in yubihsm-shell CLI tool

[invd]

Part no. 1

During recent fuzzing of yubihsm-shell, I've noticed that the following memory allocated in the main() is not properly free'd in the cleanup phase:

yubihsm-shell/src/main.c

Line 1791 in 130a1cf

ctx.connector_list = calloc(1, sizeof(char *));

yubihsm-shell/src/main.c

Line 1798 in 130a1cf

ctx.connector_list[0] = strdup(LOCAL_CONNECTOR_URL);

The main_exit section doesn't touch it:

yubihsm-shell/src/main.c

Lines 2718 to 2733 in 130a1cf

	main_exit:
	cmdline_parser_free(&args_info);
	if (ctx.out != stdout && ctx.out != NULL) {
	fclose(ctx.out);
	}
	if (ctx.cacert) {
	free(ctx.cacert);
	}
	if (ctx.proxy) {
	free(ctx.proxy);
	}
	yh_exit();

Part no. 2

While looking at the pcc_failure cleanup section of the related parse_configured_connectors() function, I've noticed the following line, which I think is flawed and also leads to some resource leakage:

yubihsm-shell/src/main.c

Line 1713 in 130a1cf

ctx->connector_list = NULL;

It doesn't make sense to overwrite ctx->connector_list multiple times with NULL and then try to free() it.
The line should probably have been ctx->connector_list[i] = NULL;

Comment

Given the context, I do not consider either issue security related. The functional impact of the leaks is likely also low for regular CLI operation.

[nevun]

Thank you for the report. You are correct that this is a memory leak and no 2 is for sure a mistake.

Would you agree the following patch fixes it?

diff --git a/src/main.c b/src/main.c
index 7e488ad..1431ea7 100644
--- a/src/main.c
+++ b/src/main.c
@@ -1710,7 +1710,7 @@ static int parse_configured_connectors(yubihsm_context *ctx, char **connectors,
 pcc_failure:
   for (int i = 0; i < ctx->n_connectors; i++) {
     free(ctx->connector_list[i]);
-    ctx->connector_list = NULL;
+    ctx->connector_list[i] = NULL;
   }
 
   free(ctx->connector_list);
@@ -2737,5 +2737,14 @@ main_exit:
   ctx.state = NULL;
 #endif
 
+  if (ctx.connector_list != NULL) {
+    for (int i = 0; i < ctx.n_connectors; i++) {
+      free(ctx.connector_list[i]);
+      ctx.connector_list[i] = NULL;
+    }
+    free(ctx.connector_list);
+    ctx.connector_list = NULL;
+  }
+
   return rc;
 }

[invd]

@nevun:
I think ctx.n_connectors = 0; can be added to the second part of the patch, otherwise things look good.
I'll test the exact code in the next days.