From: Remi Tricot-Le Breton <rlebreton@haproxy.com>
Date: Wed, 20 Mar 2024 13:13:35 +0000 (+0100)
Subject: BUG/MEDIUM: ssl: Fix crash in ocsp-update log function
X-Git-Tag: v3.0-dev6~75
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=d4e3be18df73a4f15b7b609d00f1d036f7ab7f81;p=thirdparty%2Fhaproxy.git

BUG/MEDIUM: ssl: Fix crash in ocsp-update log function

The ocsp-update logging mechanism was built around the 'sess_log'
function which required to keep a pointer to the said session until the
logging function could be called. This was made by keeping a pointer to
the appctx returned by the 'httpclient_start' function. But this appctx
lives its life on its own and might be destroyed before
'ssl_ocsp_send_log' is called, which could result in a crash (UAF).
Fixing this crash requires to stop using the 'sess_log' function to emit
the ocsp-update logs. The log line will then need to be built by hand
out of the information actually available when 'ssl_ocsp_send_log' is
called. Since we don't use the "regular" logging functions anymore, we
don't need to use the error_logformat anymore. In order to keep a
consistent behavior than before, we will keep the same format for the
logs but replace the fields that required a 'sess' pointer by fake
values (the %ci:%cp for instance, which was never filled anyway).

This crash was raised in GitHub issue #2442.
It should be backported up to branch 2.8.
---

diff --git a/src/ssl_ocsp.c b/src/ssl_ocsp.c
index 95e113e8e0..b78cb3c28f 100644
--- a/src/ssl_ocsp.c
+++ b/src/ssl_ocsp.c
@@ -878,7 +878,6 @@ static struct proxy *httpclient_ocsp_update_px;
 static struct ssl_ocsp_task_ctx {
 	struct certificate_ocsp *cur_ocsp;
 	struct httpclient *hc;
-	struct appctx *appctx;
 	int flags;
 	int update_status;
 } ssl_ocsp_task_ctx;
@@ -1111,18 +1110,41 @@ void ocsp_update_response_end_cb(struct httpclient *hc)
 
 
 /*
- * Send a log line that will use the dedicated proxy's error_logformat string.
- * It uses the sess_log function instead of app_log for instance in order to
- * benefit from the "generic" items that can be added to a log format line such
- * as the date and frontend name that can be found at the beginning of the
- * ocspupdate_log_format line.
+ * Send a log line that will mimic this previously used logformat :
+ * char ocspupdate_log_format[] = "%ci:%cp [%tr] %ft %[ssl_ocsp_certname] \
+ * %[ssl_ocsp_status] %{+Q}[ssl_ocsp_status_str] %[ssl_ocsp_fail_cnt] \
+ * %[ssl_ocsp_success_cnt]";
+ * We can't use the regular sess_log function because we don't have any control
+ * over the stream and session used by the httpclient which might not exist
+ * anymore by the time we call this function.
  */
 static void ssl_ocsp_send_log()
 {
-	if (!ssl_ocsp_task_ctx.appctx)
+	int status_str_len = 0;
+	char *status_str = NULL;
+	struct certificate_ocsp *ocsp = ssl_ocsp_task_ctx.cur_ocsp;
+	struct tm tm;
+	char timebuf[25];
+
+	if (!httpclient_ocsp_update_px)
 		return;
 
-	sess_log(ssl_ocsp_task_ctx.appctx->sess);
+	if (ocsp && ssl_ocsp_task_ctx.update_status < OCSP_UPDT_ERR_LAST) {
+		status_str_len = istlen(ocsp_update_errors[ssl_ocsp_task_ctx.update_status]);
+		status_str = istptr(ocsp_update_errors[ssl_ocsp_task_ctx.update_status]);
+	}
+
+	get_localtime(date.tv_sec, &tm);
+	date2str_log(timebuf, &tm, &date, 25);
+
+	send_log(httpclient_ocsp_update_px, LOG_INFO, "-:- [%s] %s %s %u \"%.*s\" %u %u",
+	         timebuf,
+	         httpclient_ocsp_update_px->id,
+	         ocsp->path,
+	         ssl_ocsp_task_ctx.update_status,
+	         status_str_len, status_str,
+	         ocsp ? ocsp->num_failure : 0,
+	         ocsp ? ocsp->num_success : 0);
 }
 
 /*
@@ -1316,7 +1338,7 @@ static struct task *ssl_ocsp_update_responses(struct task *task, void *context,
 		hc->ops.res_payload = ocsp_update_response_body_cb;
 		hc->ops.res_end = ocsp_update_response_end_cb;
 
-		if (!(ctx->appctx = httpclient_start(hc))) {
+		if (!httpclient_start(hc)) {
 			goto leave;
 		}
 
@@ -1380,7 +1402,6 @@ http_error:
 	return task;
 }
 
-char ocspupdate_log_format[] = "%ci:%cp [%tr] %ft %[ssl_ocsp_certname] %[ssl_ocsp_status] %{+Q}[ssl_ocsp_status_str] %[ssl_ocsp_fail_cnt] %[ssl_ocsp_success_cnt]";
 
 /*
  * Initialize the proxy for the OCSP update HTTP client with 2 servers, one for
@@ -1392,7 +1413,6 @@ static int ssl_ocsp_update_precheck()
 	httpclient_ocsp_update_px = httpclient_create_proxy("<OCSP-UPDATE>");
 	if (!httpclient_ocsp_update_px)
 		return ERR_RETRYABLE;
-	httpclient_ocsp_update_px->conf.error_logformat_string = strdup(ocspupdate_log_format);
 	httpclient_ocsp_update_px->conf.logformat_string = httpclient_log_format;
 	httpclient_ocsp_update_px->options2 |= PR_O2_NOLOGNORM;