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

1 files changed, 9 insertions, 8 deletions

diff --git a/lib/libcrypto/ocsp/ocsp_cl.c b/lib/libcrypto/ocsp/ocsp_cl.c
index 0ed816cdc31..cb5a2f3d188 100644
--- a/lib/libcrypto/ocsp/ocsp_cl.c
+++ b/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;
 }