From 5c52dd525901f6458d9d47cc6f3e809538d6982a Mon Sep 17 00:00:00 2001
From: Nathan Kinder <nkinder@redhat.com>
Date: Aug 14 2013 21:29:49 +0000
Subject: Ticket 47466 - Importing CA cert with existing name crashes security CGI


When a CA certificate is imported through the Console, but a CA certificate
with the same name already exists in the certdb, the security CGI crashes.
We are not checking the return value of CERT_ImportCerts(), which leads to
a NULL dereference.

This patch checks the return value of CERT_ImportCerts for an error, and
we return an appropriate error back to the caller.  The functions we were
using to get the error text for NSS errors was returning garbage.  We
need to use PR_ErrorToString() to get the proper error strings.

---

diff --git a/admserv/cgi-src40/security.c b/admserv/cgi-src40/security.c
index fef3bea..1cee29d 100644
--- a/admserv/cgi-src40/security.c
+++ b/admserv/cgi-src40/security.c
@@ -1194,12 +1194,7 @@ generateKey(SECKEYPublicKey** publicKey, char* tokenName)
 
  loser:
   if (privateKey==NULL) {
-    char *tmpLine = NULL;
-
-    tmpLine = (char *)PR_Malloc(PR_GetErrorTextLength()+1);
-    PR_GetErrorText(tmpLine);
-    PR_snprintf(line, sizeof(line), "%d:%s", PR_GetError(), tmpLine);
-    PR_Free(tmpLine);
+    PR_snprintf(line, sizeof(line), "%d:%s", PR_GetError(), PR_ErrorToString(PR_GetError(), PR_LANGUAGE_EN));
 
     rpt_err(GENERAL_FAILURE, 
             getResourceString(DBT_INTERNAL_ERROR), 
@@ -1324,7 +1319,7 @@ static CERTDERCerts* decodeDERCert(char *derCertBase64) {
  * Decode and display a DER certificate.
  */
 static void printDERCert(int isCACert) {
-
+  SECStatus rv;
   char *derCertBase64 = getParameter("dercert",getResourceString(DBT_DER_CERT));
   CERTDERCerts *collectArgs = decodeDERCert(derCertBase64);
 
@@ -1341,12 +1336,23 @@ static void printDERCert(int isCACert) {
     char *nickname = NULL;
 
     /*add all cert to temp */
-    CERT_ImportCerts(certdb, certUsageSSLServer,
+    rv = CERT_ImportCerts(certdb, certUsageSSLServer,
                      collectArgs->numcerts,  &collectArgs->rawCerts,
                      &retCerts, keepCerts,
                      caOnly, nickname);
 
-    printCert(retCerts[collectArgs->numcerts-1], /*showDetail=*/PR_TRUE, certType);
+    if (rv == SECSuccess) {
+      printCert(retCerts[collectArgs->numcerts-1], /*showDetail=*/PR_TRUE, certType);
+    } else {
+      PR_snprintf(line, sizeof(line), "%d:%s", PR_GetError(),
+                  PR_ErrorToString(PR_GetError(), PR_LANGUAGE_EN));
+
+      /* if unable to import report error */
+      rpt_err(SYSTEM_ERROR,
+              getResourceString(DBT_INTERNAL_ERROR),
+              getResourceString(DBT_INSTALL_FAIL),
+              line);
+    }
   }
 }
 
@@ -1378,11 +1384,15 @@ installServerCert(char *tokenName, char *certname)
     PRBool caOnly = PR_FALSE;
     char *nickname = certname;
 
-    CERT_ImportCerts(certdb, certUsageSSLServer,
+    rv = CERT_ImportCerts(certdb, certUsageSSLServer,
                      ncerts,  &collectArgs->rawCerts,
                      &retCerts, keepCerts,
                      caOnly, nickname);
 
+    if (rv != SECSuccess) {
+        goto bail;
+    }
+
     cert = retCerts[0];
   }
 
@@ -1420,19 +1430,15 @@ installServerCert(char *tokenName, char *certname)
   /* import certificate to the PKCS11 module */
   rv  = PK11_ImportCertForKeyToSlot(slot, cert, certname, PR_TRUE, 0);
 
+bail:
   if (rv != SECSuccess) {
-    {
-      char *tmpLine;
+    PR_snprintf(line, sizeof(line), "%d:%s", PR_GetError(),
+                PR_ErrorToString(PR_GetError(), PR_LANGUAGE_EN));
 
-      tmpLine = (char *)PR_Malloc(PR_GetErrorTextLength()+1);
-      PR_GetErrorText(tmpLine);
-      PR_snprintf(line, sizeof(line), "%d:%s", PR_GetError(), tmpLine);
-      PR_Free(tmpLine);
-    }
     /* if unable to import report error */
-    rpt_err(SYSTEM_ERROR, 
-            getResourceString(DBT_INTERNAL_ERROR), 
-            getResourceString(DBT_INSTALL_FAIL), 
+    rpt_err(SYSTEM_ERROR,
+            getResourceString(DBT_INTERNAL_ERROR),
+            getResourceString(DBT_INSTALL_FAIL),
             line);
   }
 }
@@ -1482,14 +1488,20 @@ installCACert(char *tokenName, char *certname)
     rc = CERT_ImportCerts(certdb, (trustedCA ? certUsageSSLCA : certUsageAnyCA),
                      collectArgs->numcerts,  &collectArgs->rawCerts,
                      &retCerts, keepCerts, caOnly, nickname);
+
+    if (rc != SECSuccess) {
+        goto bail;
+    }
+
     CERT_FindCertByDERCert(certdb, collectArgs->rawCerts);
     cert = retCerts[0];
     rc  = PK11_ImportCert(slot, cert, CK_INVALID_HANDLE, certname, PR_FALSE);
+
+bail:
     if (rc != SECSuccess) {
-        char *tmpLine = (char *)PR_Malloc(PR_GetErrorTextLength()+1);
-        PR_GetErrorText(tmpLine);
-        PR_snprintf(line, sizeof(line), "%d:%s", PR_GetError(), tmpLine);
-        PR_Free(tmpLine);
+        PR_snprintf(line, sizeof(line), "%d:%s", PR_GetError(),
+                    PR_ErrorToString(PR_GetError(), PR_LANGUAGE_EN));
+
         /* if unable to import report error */
         rpt_err(SYSTEM_ERROR, getResourceString(DBT_INTERNAL_ERROR), 
                 getResourceString(DBT_INSTALL_FAIL), line);
diff --git a/admserv/cgi-src40/security.properties b/admserv/cgi-src40/security.properties
index fd4f91b..183bad0 100644
--- a/admserv/cgi-src40/security.properties
+++ b/admserv/cgi-src40/security.properties
@@ -39,7 +39,7 @@ security11 { "Unable to read alias directory." }
 security20 { "Slot not found." }
 security21 { "Unable to decode the certificate." }
 security22 { "Private key not found." }
-security23 { "Fail to install certificate." }
+security23 { "Failed to install certificate." }
 security24 { "Either this certificate is for another server, or this certificate was not requested using this server and the selected security device \"%s\"." }
 security25 { "Invalid DER certificate." }
 security26 { "Unable to extract any certificates." }