From 664776a2f8b4574ab8c80e7bc6986ef62ef24b77 Mon Sep 17 00:00:00 2001 From: Kamil Dudka 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 --- 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 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 --- 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 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 --- 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 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 --- 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 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 --- 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