Blob Blame Raw
From 664776a2f8b4574ab8c80e7bc6986ef62ef24b77 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Thu, 3 Jul 2014 23:53:44 +0200
Subject: [PATCH 1/5] nss: let nss_{cache,load}_crl return CURLcode

Upstream-commit: 2968f957aa025003d15a4fa42c3138e99c6d2e3f
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/nss.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/lib/nss.c b/lib/nss.c
index 86775b4..a82fc64 100644
--- a/lib/nss.c
+++ b/lib/nss.c
@@ -471,7 +471,7 @@ static SECStatus nss_cache_crl(SECItem *crlDER)
     /* CRL already cached */
     SEC_DestroyCrl(crl);
     SECITEM_FreeItem(crlDER, PR_FALSE);
-    return SECSuccess;
+    return CURLE_SSL_CRL_BADFILE;
   }
 
   /* acquire lock before call of CERT_CacheCRL() */
@@ -480,16 +480,16 @@ static SECStatus nss_cache_crl(SECItem *crlDER)
     /* unable to cache CRL */
     PR_Unlock(nss_crllock);
     SECITEM_FreeItem(crlDER, PR_FALSE);
-    return SECFailure;
+    return CURLE_SSL_CRL_BADFILE;
   }
 
   /* we need to clear session cache, so that the CRL could take effect */
   SSL_ClearSessionCache();
   PR_Unlock(nss_crllock);
-  return SECSuccess;
+  return CURLE_OK;
 }
 
-static SECStatus nss_load_crl(const char* crlfilename)
+static CURLcode nss_load_crl(const char* crlfilename)
 {
   PRFileDesc *infile;
   PRFileInfo  info;
@@ -499,7 +499,7 @@ static SECStatus nss_load_crl(const char* crlfilename)
 
   infile = PR_Open(crlfilename, PR_RDONLY, 0);
   if(!infile)
-    return SECFailure;
+    return CURLE_SSL_CRL_BADFILE;
 
   if(PR_SUCCESS != PR_GetOpenFileInfo(infile, &info))
     goto fail;
@@ -545,7 +545,7 @@ static SECStatus nss_load_crl(const char* crlfilename)
 fail:
   PR_Close(infile);
   SECITEM_FreeItem(&filedata, PR_FALSE);
-  return SECFailure;
+  return CURLE_SSL_CRL_BADFILE;
 }
 
 static CURLcode nss_load_key(struct connectdata *conn, int sockindex,
@@ -1463,13 +1463,12 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
   }
 
   if(data->set.ssl.CRLfile) {
-    if(SECSuccess != nss_load_crl(data->set.ssl.CRLfile)) {
-      curlerr = CURLE_SSL_CRL_BADFILE;
+    const CURLcode rv = nss_load_crl(data->set.ssl.CRLfile);
+    if(CURLE_OK != rv) {
+      curlerr = rv;
       goto error;
     }
-    infof(data,
-          "  CRLfile: %s\n",
-          data->set.ssl.CRLfile ? data->set.ssl.CRLfile : "none");
+    infof(data, "  CRLfile: %s\n", data->set.ssl.CRLfile);
   }
 
   if(data->set.str[STRING_CERT]) {
-- 
2.13.5


From 9efc8373f8190581b5463ebcb38f52ddaa89db51 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Fri, 4 Jul 2014 00:36:21 +0200
Subject: [PATCH 2/5] nss: make crl_der allocated on heap

... and spell it as crl_der instead of crlDER

Upstream-commit: caa4db8a51e2b02e43ee85e63bc3fec232986699
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/nss.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/lib/nss.c b/lib/nss.c
index a82fc64..4e210bb 100644
--- a/lib/nss.c
+++ b/lib/nss.c
@@ -463,23 +463,23 @@ static CURLcode nss_load_cert(struct ssl_connect_data *ssl,
 }
 
 /* add given CRL to cache if it is not already there */
-static SECStatus nss_cache_crl(SECItem *crlDER)
+static CURLcode nss_cache_crl(SECItem *crl_der)
 {
   CERTCertDBHandle *db = CERT_GetDefaultCertDB();
-  CERTSignedCrl *crl = SEC_FindCrlByDERCert(db, crlDER, 0);
+  CERTSignedCrl *crl = SEC_FindCrlByDERCert(db, crl_der, 0);
   if(crl) {
     /* CRL already cached */
     SEC_DestroyCrl(crl);
-    SECITEM_FreeItem(crlDER, PR_FALSE);
+    SECITEM_FreeItem(crl_der, PR_TRUE);
     return CURLE_SSL_CRL_BADFILE;
   }
 
   /* acquire lock before call of CERT_CacheCRL() */
   PR_Lock(nss_crllock);
-  if(SECSuccess != CERT_CacheCRL(db, crlDER)) {
+  if(SECSuccess != CERT_CacheCRL(db, crl_der)) {
     /* unable to cache CRL */
     PR_Unlock(nss_crllock);
-    SECITEM_FreeItem(crlDER, PR_FALSE);
+    SECITEM_FreeItem(crl_der, PR_TRUE);
     return CURLE_SSL_CRL_BADFILE;
   }
 
@@ -494,7 +494,7 @@ static CURLcode nss_load_crl(const char* crlfilename)
   PRFileDesc *infile;
   PRFileInfo  info;
   SECItem filedata = { 0, NULL, 0 };
-  SECItem crlDER = { 0, NULL, 0 };
+  SECItem *crl_der = NULL;
   char *body;
 
   infile = PR_Open(crlfilename, PR_RDONLY, 0);
@@ -510,6 +510,10 @@ static CURLcode nss_load_crl(const char* crlfilename)
   if(info.size != PR_Read(infile, filedata.data, info.size))
     goto fail;
 
+  crl_der = SECITEM_AllocItem(NULL, NULL, 0U);
+  if(!crl_der)
+    goto fail;
+
   /* place a trailing zero right after the visible data */
   body = (char*)filedata.data;
   body[--filedata.len] = '\0';
@@ -530,20 +534,21 @@ static CURLcode nss_load_crl(const char* crlfilename)
 
     /* retrieve DER from ASCII */
     *trailer = '\0';
-    if(ATOB_ConvertAsciiToItem(&crlDER, begin))
+    if(ATOB_ConvertAsciiToItem(crl_der, begin))
       goto fail;
 
     SECITEM_FreeItem(&filedata, PR_FALSE);
   }
   else
     /* assume DER */
-    crlDER = filedata;
+    *crl_der = filedata;
 
   PR_Close(infile);
-  return nss_cache_crl(&crlDER);
+  return nss_cache_crl(crl_der);
 
 fail:
   PR_Close(infile);
+  SECITEM_FreeItem(crl_der, PR_TRUE);
   SECITEM_FreeItem(&filedata, PR_FALSE);
   return CURLE_SSL_CRL_BADFILE;
 }
-- 
2.13.5


From f2c35b7b7f50b691d3019783ce19cc6a8dd5b484 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Fri, 4 Jul 2014 00:39:23 +0200
Subject: [PATCH 3/5] nss: fix a memory leak when CURLOPT_CRLFILE is used

Upstream-commit: 52cd5ac21cdfdc0a6c016de97fe70d3a50baa526
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/nss.c     | 38 +++++++++++++++++++++++++++++++++-----
 lib/urldata.h |  1 +
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/lib/nss.c b/lib/nss.c
index 4e210bb..c3247c8 100644
--- a/lib/nss.c
+++ b/lib/nss.c
@@ -425,6 +425,14 @@ static void nss_destroy_object(void *user, void *ptr)
   PK11_DestroyGenericObject(obj);
 }
 
+/* same as nss_destroy_object() but for CRL items */
+static void nss_destroy_crl_item(void *user, void *ptr)
+{
+  SECItem *crl_der = (SECItem *)ptr;
+  (void) user;
+  SECITEM_FreeItem(crl_der, PR_TRUE);
+}
+
 static CURLcode nss_load_cert(struct ssl_connect_data *ssl,
                               const char *filename, PRBool cacert)
 {
@@ -463,7 +471,7 @@ static CURLcode nss_load_cert(struct ssl_connect_data *ssl,
 }
 
 /* add given CRL to cache if it is not already there */
-static CURLcode nss_cache_crl(SECItem *crl_der)
+static CURLcode nss_cache_crl(struct ssl_connect_data *ssl, SECItem *crl_der)
 {
   CERTCertDBHandle *db = CERT_GetDefaultCertDB();
   CERTSignedCrl *crl = SEC_FindCrlByDERCert(db, crl_der, 0);
@@ -474,12 +482,17 @@ static CURLcode nss_cache_crl(SECItem *crl_der)
     return CURLE_SSL_CRL_BADFILE;
   }
 
+  /* store the CRL item so that we can free it in Curl_nss_close() */
+  if(!Curl_llist_insert_next(ssl->crl_list, ssl->crl_list->tail, crl_der)) {
+    SECITEM_FreeItem(crl_der, PR_FALSE);
+    return CURLE_OUT_OF_MEMORY;
+  }
+
   /* acquire lock before call of CERT_CacheCRL() */
   PR_Lock(nss_crllock);
   if(SECSuccess != CERT_CacheCRL(db, crl_der)) {
     /* unable to cache CRL */
     PR_Unlock(nss_crllock);
-    SECITEM_FreeItem(crl_der, PR_TRUE);
     return CURLE_SSL_CRL_BADFILE;
   }
 
@@ -489,7 +502,8 @@ static CURLcode nss_cache_crl(SECItem *crl_der)
   return CURLE_OK;
 }
 
-static CURLcode nss_load_crl(const char* crlfilename)
+static CURLcode nss_load_crl(struct ssl_connect_data *connssl,
+                             const char* crlfilename)
 {
   PRFileDesc *infile;
   PRFileInfo  info;
@@ -544,7 +558,7 @@ static CURLcode nss_load_crl(const char* crlfilename)
     *crl_der = filedata;
 
   PR_Close(infile);
-  return nss_cache_crl(crl_der);
+  return nss_cache_crl(connssl, crl_der);
 
 fail:
   PR_Close(infile);
@@ -1147,6 +1161,10 @@ void Curl_nss_close(struct connectdata *conn, int sockindex)
     connssl->obj_list = NULL;
     connssl->obj_clicert = NULL;
 
+    /* destroy all CRL items */
+    Curl_llist_destroy(connssl->crl_list, NULL);
+    connssl->crl_list = NULL;
+
     PR_Close(connssl->handle);
     connssl->handle = NULL;
   }
@@ -1325,6 +1343,8 @@ static CURLcode nss_fail_connect(struct ssl_connect_data *connssl,
   /* cleanup on connection failure */
   Curl_llist_destroy(connssl->obj_list, NULL);
   connssl->obj_list = NULL;
+  Curl_llist_destroy(connssl->crl_list, NULL);
+  connssl->crl_list = NULL;
   return curlerr;
 }
 
@@ -1367,6 +1387,14 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
   if(!connssl->obj_list)
     return CURLE_OUT_OF_MEMORY;
 
+  /* list of all CRL items we need to destroy in Curl_nss_close() */
+  connssl->crl_list = Curl_llist_alloc(nss_destroy_crl_item);
+  if(!connssl->crl_list) {
+    Curl_llist_destroy(connssl->obj_list, NULL);
+    connssl->obj_list = NULL;
+    return CURLE_OUT_OF_MEMORY;
+  }
+
   /* FIXME. NSS doesn't support multiple databases open at the same time. */
   PR_Lock(nss_initlock);
   curlerr = nss_init(conn->data);
@@ -1468,7 +1496,7 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
   }
 
   if(data->set.ssl.CRLfile) {
-    const CURLcode rv = nss_load_crl(data->set.ssl.CRLfile);
+    const CURLcode rv = nss_load_crl(connssl, data->set.ssl.CRLfile);
     if(CURLE_OK != rv) {
       curlerr = rv;
       goto error;
diff --git a/lib/urldata.h b/lib/urldata.h
index f4c6222..3624af1 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -301,6 +301,7 @@ struct ssl_connect_data {
   PRFileDesc *handle;
   char *client_nickname;
   struct SessionHandle *data;
+  struct curl_llist *crl_list;
   struct curl_llist *obj_list;
   PK11GenericObject *obj_clicert;
   ssl_connect_state connecting_state;
-- 
2.13.5


From 6f93eefb3361e430274eb9e76ff84380289c6164 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Fri, 4 Jul 2014 12:41:53 +0200
Subject: [PATCH 4/5] nss: make the list of CRL items global

Otherwise NSS could use an already freed item for another connection.

Upstream-commit: ca2aa61b66d684a1076d43025048f1a43d5755b6
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/nss.c     | 46 ++++++++++++++++++++++------------------------
 lib/urldata.h |  1 -
 2 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/lib/nss.c b/lib/nss.c
index c3247c8..acbd09a 100644
--- a/lib/nss.c
+++ b/lib/nss.c
@@ -77,6 +77,7 @@ PRFileDesc *PR_ImportTCPSocket(PRInt32 osfd);
 static PRLock *nss_initlock = NULL;
 static PRLock *nss_crllock = NULL;
 static PRLock *nss_findslot_lock = NULL;
+struct curl_llist *nss_crl_list = NULL;
 NSSInitContext * nss_context = NULL;
 
 volatile int initialized = 0;
@@ -471,7 +472,7 @@ static CURLcode nss_load_cert(struct ssl_connect_data *ssl,
 }
 
 /* add given CRL to cache if it is not already there */
-static CURLcode nss_cache_crl(struct ssl_connect_data *ssl, SECItem *crl_der)
+static CURLcode nss_cache_crl(SECItem *crl_der)
 {
   CERTCertDBHandle *db = CERT_GetDefaultCertDB();
   CERTSignedCrl *crl = SEC_FindCrlByDERCert(db, crl_der, 0);
@@ -482,14 +483,16 @@ static CURLcode nss_cache_crl(struct ssl_connect_data *ssl, SECItem *crl_der)
     return CURLE_SSL_CRL_BADFILE;
   }
 
-  /* store the CRL item so that we can free it in Curl_nss_close() */
-  if(!Curl_llist_insert_next(ssl->crl_list, ssl->crl_list->tail, crl_der)) {
-    SECITEM_FreeItem(crl_der, PR_FALSE);
+  /* acquire lock before call of CERT_CacheCRL() and accessing nss_crl_list */
+  PR_Lock(nss_crllock);
+
+  /* store the CRL item so that we can free it in Curl_nss_cleanup() */
+  if(!Curl_llist_insert_next(nss_crl_list, nss_crl_list->tail, crl_der)) {
+    SECITEM_FreeItem(crl_der, PR_TRUE);
+    PR_Unlock(nss_crllock);
     return CURLE_OUT_OF_MEMORY;
   }
 
-  /* acquire lock before call of CERT_CacheCRL() */
-  PR_Lock(nss_crllock);
   if(SECSuccess != CERT_CacheCRL(db, crl_der)) {
     /* unable to cache CRL */
     PR_Unlock(nss_crllock);
@@ -502,8 +505,7 @@ static CURLcode nss_cache_crl(struct ssl_connect_data *ssl, SECItem *crl_der)
   return CURLE_OK;
 }
 
-static CURLcode nss_load_crl(struct ssl_connect_data *connssl,
-                             const char* crlfilename)
+static CURLcode nss_load_crl(const char* crlfilename)
 {
   PRFileDesc *infile;
   PRFileInfo  info;
@@ -558,7 +560,7 @@ static CURLcode nss_load_crl(struct ssl_connect_data *connssl,
     *crl_der = filedata;
 
   PR_Close(infile);
-  return nss_cache_crl(connssl, crl_der);
+  return nss_cache_crl(crl_der);
 
 fail:
   PR_Close(infile);
@@ -996,6 +998,11 @@ static CURLcode nss_init(struct SessionHandle *data)
   if(initialized)
     return CURLE_OK;
 
+  /* list of all CRL items we need to destroy in Curl_nss_cleanup() */
+  nss_crl_list = Curl_llist_alloc(nss_destroy_crl_item);
+  if(!nss_crl_list)
+    return CURLE_OUT_OF_MEMORY;
+
   /* First we check if $SSL_DIR points to a valid dir */
   cert_dir = getenv("SSL_DIR");
   if(cert_dir) {
@@ -1096,6 +1103,11 @@ void Curl_nss_cleanup(void)
     NSS_ShutdownContext(nss_context);
     nss_context = NULL;
   }
+
+  /* destroy all CRL items */
+  Curl_llist_destroy(nss_crl_list, NULL);
+  nss_crl_list = NULL;
+
   PR_Unlock(nss_initlock);
 
   PR_DestroyLock(nss_initlock);
@@ -1161,10 +1173,6 @@ void Curl_nss_close(struct connectdata *conn, int sockindex)
     connssl->obj_list = NULL;
     connssl->obj_clicert = NULL;
 
-    /* destroy all CRL items */
-    Curl_llist_destroy(connssl->crl_list, NULL);
-    connssl->crl_list = NULL;
-
     PR_Close(connssl->handle);
     connssl->handle = NULL;
   }
@@ -1343,8 +1351,6 @@ static CURLcode nss_fail_connect(struct ssl_connect_data *connssl,
   /* cleanup on connection failure */
   Curl_llist_destroy(connssl->obj_list, NULL);
   connssl->obj_list = NULL;
-  Curl_llist_destroy(connssl->crl_list, NULL);
-  connssl->crl_list = NULL;
   return curlerr;
 }
 
@@ -1387,14 +1393,6 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
   if(!connssl->obj_list)
     return CURLE_OUT_OF_MEMORY;
 
-  /* list of all CRL items we need to destroy in Curl_nss_close() */
-  connssl->crl_list = Curl_llist_alloc(nss_destroy_crl_item);
-  if(!connssl->crl_list) {
-    Curl_llist_destroy(connssl->obj_list, NULL);
-    connssl->obj_list = NULL;
-    return CURLE_OUT_OF_MEMORY;
-  }
-
   /* FIXME. NSS doesn't support multiple databases open at the same time. */
   PR_Lock(nss_initlock);
   curlerr = nss_init(conn->data);
@@ -1496,7 +1494,7 @@ static CURLcode nss_setup_connect(struct connectdata *conn, int sockindex)
   }
 
   if(data->set.ssl.CRLfile) {
-    const CURLcode rv = nss_load_crl(connssl, data->set.ssl.CRLfile);
+    const CURLcode rv = nss_load_crl(data->set.ssl.CRLfile);
     if(CURLE_OK != rv) {
       curlerr = rv;
       goto error;
diff --git a/lib/urldata.h b/lib/urldata.h
index 3624af1..f4c6222 100644
--- a/lib/urldata.h
+++ b/lib/urldata.h
@@ -301,7 +301,6 @@ struct ssl_connect_data {
   PRFileDesc *handle;
   char *client_nickname;
   struct SessionHandle *data;
-  struct curl_llist *crl_list;
   struct curl_llist *obj_list;
   PK11GenericObject *obj_clicert;
   ssl_connect_state connecting_state;
-- 
2.13.5


From de0742d4141ede4d1849ff1ebffd820faea53ad7 Mon Sep 17 00:00:00 2001
From: Kamil Dudka <kdudka@redhat.com>
Date: Wed, 8 Oct 2014 17:13:59 +0200
Subject: [PATCH 5/5] nss: do not fail if a CRL is already cached

This fixes a copy-paste mistake from commit 2968f957.

Upstream-commit: 9e37a7f9a5cd141c717aa0262e8dee7713c25200
Signed-off-by: Kamil Dudka <kdudka@redhat.com>
---
 lib/nss.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/nss.c b/lib/nss.c
index acbd09a..1b8abd3 100644
--- a/lib/nss.c
+++ b/lib/nss.c
@@ -480,7 +480,7 @@ static CURLcode nss_cache_crl(SECItem *crl_der)
     /* CRL already cached */
     SEC_DestroyCrl(crl);
     SECITEM_FreeItem(crl_der, PR_TRUE);
-    return CURLE_SSL_CRL_BADFILE;
+    return CURLE_OK;
   }
 
   /* acquire lock before call of CERT_CacheCRL() and accessing nss_crl_list */
-- 
2.13.5