OCSP: fix memory leak in OCSP_url_svcloc_new method.

Add a few coverage test case.

Fixes #8949

[extended tests]

Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from https://github.com/openssl/openssl/pull/8959)

2 files changed, 83 insertions, 3 deletions

diff --git a/crypto/ocsp/ocsp_ext.c b/crypto/ocsp/ocsp_ext.c
index 8ebfd6284e..c5cf279170 100644
--- a/crypto/ocsp/ocsp_ext.c
+++ b/crypto/ocsp/ocsp_ext.c
@@ -439,6 +439,7 @@ X509_EXTENSION *OCSP_url_svcloc_new(X509_NAME *issuer, const char **urls)
 
     if ((sloc = OCSP_SERVICELOC_new()) == NULL)
         goto err;
+    X509_NAME_free(sloc->issuer);
     if ((sloc->issuer = X509_NAME_dup(issuer)) == NULL)
         goto err;
     if (urls && *urls
@@ -449,12 +450,11 @@ X509_EXTENSION *OCSP_url_svcloc_new(X509_NAME *issuer, const char **urls)
             goto err;
         if ((ad->method = OBJ_nid2obj(NID_ad_OCSP)) == NULL)
             goto err;
-        if ((ad->location = GENERAL_NAME_new()) == NULL)
-            goto err;
         if ((ia5 = ASN1_IA5STRING_new()) == NULL)
             goto err;
         if (!ASN1_STRING_set((ASN1_STRING *)ia5, *urls, -1))
             goto err;
+        /* ad->location is allocated inside ACCESS_DESCRIPTION_new */
         ad->location->type = GEN_URI;
         ad->location->d.ia5 = ia5;
         ia5 = NULL;
diff --git a/test/ocspapitest.c b/test/ocspapitest.c
index 03b88e064b..355bd448ae 100644
--- a/test/ocspapitest.c
+++ b/test/ocspapitest.c
@@ -47,6 +47,24 @@ static int get_cert_and_key(X509 **cert_out, EVP_PKEY **key_out)
     return 0;
 }
 
+static int get_cert(X509 **cert_out)
+{
+    BIO *certbio;
+    X509 *cert = NULL;
+
+    if (!TEST_ptr(certbio = BIO_new_file(certstr, "r")))
+        return 0;
+    cert = PEM_read_bio_X509(certbio, NULL, NULL, NULL);
+    BIO_free(certbio);
+    if (!TEST_ptr(cert))
+        goto end;
+    *cert_out = cert;
+    return 1;
+ end:
+    X509_free(cert);
+    return 0;
+}
+
 static OCSP_BASICRESP *make_dummy_resp(void)
 {
     const unsigned char namestr[] = "openssl.example.com";
@@ -131,7 +149,67 @@ static int test_resp_signer(void)
     EVP_PKEY_free(key);
     return ret;
 }
-#endif
+
+static int test_access_description(int testcase)
+{
+    ACCESS_DESCRIPTION *ad = ACCESS_DESCRIPTION_new();
+    int ret = 0;
+
+    if (!TEST_ptr(ad))
+        goto err;
+
+    switch (testcase) {
+    case 0:     /* no change */
+        break;
+    case 1:     /* check and release current location */
+        if (!TEST_ptr(ad->location))
+            goto err;
+        GENERAL_NAME_free(ad->location);
+        ad->location = NULL;
+        break;
+    case 2:     /* replace current location */
+        GENERAL_NAME_free(ad->location);
+        ad->location = GENERAL_NAME_new();
+        if (!TEST_ptr(ad->location))
+            goto err;
+        break;
+    }
+    ACCESS_DESCRIPTION_free(ad);
+    ret = 1;
+err:
+    return ret;
+}
+
+static int test_ocsp_url_svcloc_new(void)
+{
+    static const char *  urls[] = {
+        "www.openssl.org",
+        "www.openssl.net",
+        NULL
+    };
+
+    X509 *issuer = NULL;
+    X509_EXTENSION * ext = NULL;
+    int ret = 0;
+
+    if (!TEST_true(get_cert(&issuer)))
+        goto err;
+
+    /*
+     * Test calling this ocsp method to catch any memory leak
+     */
+    ext = OCSP_url_svcloc_new(X509_get_issuer_name(issuer), urls);
+    if (!TEST_ptr(ext))
+        goto err;
+
+    X509_EXTENSION_free(ext);
+    ret = 1;
+err:
+    X509_free(issuer);
+    return ret;
+}
+
+#endif /* OPENSSL_NO_OCSP */
 
 OPT_TEST_DECLARE_USAGE("certfile privkeyfile\n")
 
@@ -142,6 +220,8 @@ int setup_tests(void)
         return 0;
 #ifndef OPENSSL_NO_OCSP
     ADD_TEST(test_resp_signer);
+    ADD_ALL_TESTS(test_access_description, 3);
+    ADD_TEST(test_ocsp_url_svcloc_new);
 #endif
     return 1;
 }