From c2d858c793f1c66f3c541ebc3f6d0bf4deb3b7e7 Mon Sep 17 00:00:00 2001
From: tb <>
Date: Fri, 9 Oct 2020 17:19:35 +0000
Subject: Fix leak or double free with OCSP_request_add0_id()
On success, OCSP_request_add0_id() transfers ownership of cid to
either 'one' or 'req' depending on whether the latter is NULL or
not. On failure, the caller can't tell whether OCSP_ONEREQ_new()
failed (in which case cid needs to be freed) or whether it was a
failure to allocate memory in sk_insert() (in which case cid must
not be freed).
The caller is thus faced with the choice of leaving either a leak
or a potential double free. Fix this by transferring ownership
only at the end of the function.
Found while reviewing an upcoming diff by beck.
ok jsing
---
src/lib/libcrypto/ocsp/ocsp_cl.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
(limited to 'src')
diff --git a/src/lib/libcrypto/ocsp/ocsp_cl.c b/src/lib/libcrypto/ocsp/ocsp_cl.c
index 0ed816cdc3..cb5a2f3d18 100644
--- a/src/lib/libcrypto/ocsp/ocsp_cl.c
+++ b/src/lib/libcrypto/ocsp/ocsp_cl.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ocsp_cl.c,v 1.16 2018/11/25 19:48:43 jmc Exp $ */
+/* $OpenBSD: ocsp_cl.c,v 1.17 2020/10/09 17:19:35 tb Exp $ */
/* Written by Tom Titchener <Tom_Titchener@groove.net> for the OpenSSL
* project. */
@@ -81,18 +81,19 @@
OCSP_ONEREQ *
OCSP_request_add0_id(OCSP_REQUEST *req, OCSP_CERTID *cid)
{
- OCSP_ONEREQ *one = NULL;
+ OCSP_ONEREQ *one;
- if (!(one = OCSP_ONEREQ_new()))
+ if ((one = OCSP_ONEREQ_new()) == NULL)
goto err;
- if (one->reqCert)
- OCSP_CERTID_free(one->reqCert);
+ if (req != NULL) {
+ if (!sk_OCSP_ONEREQ_push(req->tbsRequest->requestList, one))
+ goto err;
+ }
+ OCSP_CERTID_free(one->reqCert);
one->reqCert = cid;
- if (req && !sk_OCSP_ONEREQ_push(req->tbsRequest->requestList, one))
- goto err;
return one;
-err:
+ err:
OCSP_ONEREQ_free(one);
return NULL;
}
--
cgit v1.2.3-55-g6feb