From: William Lallemand <wlallemand@haproxy.org>
Date: Tue, 26 Apr 2022 16:17:15 +0000 (+0200)
Subject: BUG/MEDIUM: ssl/cli: fix yielding in show_cafile_detail
X-Git-Tag: v2.6-dev8~64
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=03a32e5dd2fa103fa259db343d97bd093544ef25;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: ssl/cli: fix yielding in show_cafile_detail

HAProxy crashes when "show ssl ca-file" is being called on a ca-file
which contains a lot of certificates. (127 in our test with
/etc/ssl/certs/ca-certificates.crt).

The root cause is the fonction does not yield when there is no available
space when writing the details, and we could write a lot.

To fix the issue, we try to put the data with ci_putchk() after every
show_cert_detail() and we yield if the ci_putchk() fails.

This patch also cleans up a little bit the code:

- the end label is now a real end with a return 1;
- i0 is used instead of (long)p1
- the ID is stored upon yield
---

diff --git a/src/ssl_ckch.c b/src/ssl_ckch.c
index 447fdbb568..aff35ebcd7 100644
--- a/src/ssl_ckch.c
+++ b/src/ssl_ckch.c
@@ -2928,11 +2928,12 @@ static int cli_io_handler_show_cafile_detail(struct appctx *appctx)
 	struct conn_stream *cs = appctx->owner;
 	struct cafile_entry *cafile_entry = appctx->ctx.cli.p0;
 	struct buffer *out = alloc_trash_chunk();
-	int i;
+	int i = 0;
 	X509 *cert;
 	STACK_OF(X509_OBJECT) *objs;
 	int retval = 0;
-	long ca_index = (long)appctx->ctx.cli.p1;
+	int ca_index = appctx->ctx.cli.i0;
+	int show_all = appctx->ctx.cli.i1;
 
 	if (!out)
 		goto end_no_putchk;
@@ -2954,33 +2955,39 @@ static int cli_io_handler_show_cafile_detail(struct appctx *appctx)
 		goto end;
 
 	objs = X509_STORE_get0_objects(cafile_entry->ca_store);
-	for (i = 0; i < sk_X509_OBJECT_num(objs); i++) {
+	for (i = ca_index; i < sk_X509_OBJECT_num(objs); i++) {
+
 		cert = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(objs, i));
 		if (!cert)
 			continue;
 
-		/* Certificate indexes start at 1 on the CLI output. */
-		if (ca_index && ca_index-1 != i)
-			continue;
-
+		/* file starts at line 1 */
 		chunk_appendf(out, " \nCertificate #%d:\n", i+1);
 		retval = show_cert_detail(cert, NULL, out);
 		if (retval < 0)
 			goto end_no_putchk;
-		else if (retval || ca_index)
+		else if (retval)
+			goto yield;
+
+		if (ci_putchk(cs_ic(cs), out) == -1) {
+			cs_rx_room_blk(cs);
+			goto yield;
+		}
+
+		if (!show_all)   /* only need to dump one certificate */
 			goto end;
 	}
 
 end:
-	if (ci_putchk(cs_ic(cs), out) == -1) {
-		cs_rx_room_blk(cs);
-		goto yield;
-	}
+	free_trash_chunk(out);
+	return 1; /* end, don't come back */
 
 end_no_putchk:
 	free_trash_chunk(out);
 	return 1;
 yield:
+	/* save the current state */
+	appctx->ctx.cli.i0 = i;
 	free_trash_chunk(out);
 	return 0; /* should come back */
 }
@@ -2990,7 +2997,7 @@ yield:
 static int cli_parse_show_cafile(char **args, char *payload, struct appctx *appctx, void *private)
 {
 	struct cafile_entry *cafile_entry;
-	long ca_index = 0;
+	int ca_index = 0;
 	char *colons;
 	char *err = NULL;
 
@@ -3002,6 +3009,8 @@ static int cli_parse_show_cafile(char **args, char *payload, struct appctx *appc
 	if (HA_SPIN_TRYLOCK(CKCH_LOCK, &ckch_lock))
 		return cli_err(appctx, "Can't show!\nOperations on certificates are currently locked!\n");
 
+	appctx->ctx.cli.i1 = 1; /* show all certificates */
+	appctx->ctx.cli.i0 = 0;
 	/* check if there is a certificate to lookup */
 	if (*args[3]) {
 
@@ -3017,6 +3026,8 @@ static int cli_parse_show_cafile(char **args, char *payload, struct appctx *appc
 				goto error;
 			}
 			*colons = '\0';
+			appctx->ctx.cli.i0 = ca_index - 1; /* we start counting at 0 in the ca_store, but at 1 on the CLI */
+			appctx->ctx.cli.i1 = 0; /* show only one certificate */
 		}
 
 		if (*args[3] == '*') {
@@ -3036,7 +3047,6 @@ static int cli_parse_show_cafile(char **args, char *payload, struct appctx *appc
 		}
 
 		appctx->ctx.cli.p0 = cafile_entry;
-		appctx->ctx.cli.p1 = (void*)ca_index;
 		/* use the IO handler that shows details */
 		appctx->io_handler = cli_io_handler_show_cafile_detail;
 	}