bug 1239344 - remove error alert for successful PKCS12 operations r?Cykesiopka
MozReview-Commit-ID: Hr6s2v2GmZQ
--- a/security/manager/locales/en-US/chrome/pipnss/pipnss.properties
+++ b/security/manager/locales/en-US/chrome/pipnss/pipnss.properties
@@ -231,18 +231,16 @@ CertDumpECsect239k1=SECG elliptic curve
CertDumpECsect283k1=SECG elliptic curve sect283k1 (aka NIST K-283)
CertDumpECsect283r1=SECG elliptic curve sect283r1 (aka NIST B-283)
CertDumpECsect409k1=SECG elliptic curve sect409k1 (aka NIST K-409)
CertDumpECsect409r1=SECG elliptic curve sect409r1 (aka NIST B-409)
CertDumpECsect571k1=SECG elliptic curve sect571k1 (aka NIST K-571)
CertDumpECsect571r1=SECG elliptic curve sect571r1 (aka NIST B-571)
CertDumpRawBytesHeader=Size: %S Bytes / %S Bits
PK11BadPassword=The password entered was incorrect.
-SuccessfulP12Backup=Successfully backed up your security certificate(s) and private key(s).
-SuccessfulP12Restore=Successfully restored your security certificate(s) and private key(s).
PKCS12DecodeErr=Failed to decode the file. Either it is not in PKCS #12 format, has been corrupted, or the password you entered was incorrect.
PKCS12UnknownErrRestore=Failed to restore the PKCS #12 file for unknown reasons.
PKCS12UnknownErrBackup=Failed to create the PKCS #12 backup file for unknown reasons.
PKCS12UnknownErr=The PKCS #12 operation failed for unknown reasons.
PKCS12InfoNoSmartcardBackup=It is not possible to back up certificates from a hardware security device such as a smart card.
PKCS12DupData=The certificate and private key already exist on the security device.
AddModuleFailure=Unable to add module
AddModuleDup=Security Module already exists
--- a/security/manager/ssl/nsNSSCertHelper.cpp
+++ b/security/manager/ssl/nsNSSCertHelper.cpp
@@ -84,19 +84,23 @@ GetPIPNSSBundle(nsIStringBundle** pipnss
do_GetService(NS_STRINGBUNDLE_CONTRACTID));
if (!bundleService) {
return NS_ERROR_NOT_AVAILABLE;
}
return bundleService->CreateBundle("chrome://pipnss/locale/pipnss.properties",
pipnssBundle);
}
-static nsresult
+nsresult
GetPIPNSSBundleString(const char* stringName, nsAString& result)
{
+ MOZ_ASSERT(NS_IsMainThread());
+ if (!NS_IsMainThread()) {
+ return NS_ERROR_NOT_SAME_THREAD;
+ }
MOZ_ASSERT(stringName);
if (!stringName) {
return NS_ERROR_INVALID_ARG;
}
nsCOMPtr<nsIStringBundle> pipnssBundle;
nsresult rv = GetPIPNSSBundle(getter_AddRefs(pipnssBundle));
if (NS_FAILED(rv)) {
return rv;
--- a/security/manager/ssl/nsNSSCertHelper.h
+++ b/security/manager/ssl/nsNSSCertHelper.h
@@ -8,16 +8,19 @@
#ifndef INET6_ADDRSTRLEN
#define INET6_ADDRSTRLEN 46
#endif
#include "certt.h"
#include "nsString.h"
uint32_t
-getCertType(CERTCertificate *cert);
+getCertType(CERTCertificate* cert);
nsresult
-GetCertFingerprintByOidTag(CERTCertificate* nsscert,
- SECOidTag aOidTag,
- nsCString &fp);
+GetCertFingerprintByOidTag(CERTCertificate* nsscert, SECOidTag aOidTag,
+ nsCString& fp);
+
+// Must be used on the main thread only.
+nsresult
+GetPIPNSSBundleString(const char* stringName, nsAString& result);
#endif // nsNSSCertHelper_h
--- a/security/manager/ssl/nsNSSCertificateDB.cpp
+++ b/security/manager/ssl/nsNSSCertificateDB.cpp
@@ -964,30 +964,36 @@ nsNSSCertificateDB::ImportCertsFromFile(
}
return NS_ERROR_FAILURE;
}
NS_IMETHODIMP
nsNSSCertificateDB::ImportPKCS12File(nsIFile* aFile)
{
+ if (!NS_IsMainThread()) {
+ return NS_ERROR_NOT_SAME_THREAD;
+ }
nsNSSShutDownPreventionLock locker;
if (isAlreadyShutDown()) {
return NS_ERROR_NOT_AVAILABLE;
}
NS_ENSURE_ARG(aFile);
nsPKCS12Blob blob;
return blob.ImportFromFile(aFile);
}
NS_IMETHODIMP
nsNSSCertificateDB::ExportPKCS12File(nsIFile* aFile, uint32_t count,
nsIX509Cert** certs)
{
+ if (!NS_IsMainThread()) {
+ return NS_ERROR_NOT_SAME_THREAD;
+ }
nsNSSShutDownPreventionLock locker;
if (isAlreadyShutDown()) {
return NS_ERROR_NOT_AVAILABLE;
}
NS_ENSURE_ARG(aFile);
if (count == 0) {
return NS_OK;
--- a/security/manager/ssl/nsNSSComponent.cpp
+++ b/security/manager/ssl/nsNSSComponent.cpp
@@ -1962,42 +1962,16 @@ nsNSSComponent::GetNewPrompter(nsIPrompt
NS_ENSURE_SUCCESS(rv, rv);
rv = wwatch->GetNewPrompter(0, result);
NS_ENSURE_SUCCESS(rv, rv);
return rv;
}
-/*static*/ nsresult
-nsNSSComponent::ShowAlertWithConstructedString(const nsString& message)
-{
- nsCOMPtr<nsIPrompt> prompter;
- nsresult rv = GetNewPrompter(getter_AddRefs(prompter));
- if (prompter) {
- rv = prompter->Alert(nullptr, message.get());
- }
- return rv;
-}
-
-NS_IMETHODIMP
-nsNSSComponent::ShowAlertFromStringBundle(const char* messageID)
-{
- nsString message;
- nsresult rv;
-
- rv = GetPIPNSSBundleString(messageID, message);
- if (NS_FAILED(rv)) {
- NS_ERROR("GetPIPNSSBundleString failed");
- return rv;
- }
-
- return ShowAlertWithConstructedString(message);
-}
-
nsresult nsNSSComponent::LogoutAuthenticatedPK11()
{
nsCOMPtr<nsICertOverrideService> icos =
do_GetService("@mozilla.org/security/certoverride;1");
if (icos) {
icos->ClearValidityOverride(
NS_LITERAL_CSTRING("all:temporary-certificates"),
0);
--- a/security/manager/ssl/nsNSSComponent.h
+++ b/security/manager/ssl/nsNSSComponent.h
@@ -49,18 +49,16 @@ MOZ_MUST_USE
extern bool EnsureNSSInitializedChromeOrContent();
class NS_NO_VTABLE nsINSSComponent : public nsISupports
{
public:
NS_DECLARE_STATIC_IID_ACCESSOR(NS_INSSCOMPONENT_IID)
- NS_IMETHOD ShowAlertFromStringBundle(const char* messageID) = 0;
-
NS_IMETHOD GetPIPNSSBundleString(const char* name,
nsAString& outString) = 0;
NS_IMETHOD PIPBundleFormatStringFromName(const char* name,
const char16_t** params,
uint32_t numParams,
nsAString& outString) = 0;
NS_IMETHOD GetNSSBundleString(const char* name,
@@ -102,18 +100,16 @@ public:
nsNSSComponent();
NS_DECL_THREADSAFE_ISUPPORTS
NS_DECL_NSIOBSERVER
nsresult Init();
static nsresult GetNewPrompter(nsIPrompt** result);
- static nsresult ShowAlertWithConstructedString(const nsString& message);
- NS_IMETHOD ShowAlertFromStringBundle(const char* messageID) override;
NS_IMETHOD GetPIPNSSBundleString(const char* name,
nsAString& outString) override;
NS_IMETHOD PIPBundleFormatStringFromName(const char* name,
const char16_t** params,
uint32_t numParams,
nsAString& outString) override;
NS_IMETHOD GetNSSBundleString(const char* name, nsAString& outString) override;
--- a/security/manager/ssl/nsPKCS12Blob.cpp
+++ b/security/manager/ssl/nsPKCS12Blob.cpp
@@ -2,37 +2,36 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
#include "nsPKCS12Blob.h"
#include "ScopedNSSTypes.h"
#include "mozilla/Assertions.h"
#include "mozilla/Casting.h"
+#include "mozilla/Unused.h"
#include "nsICertificateDialogs.h"
#include "nsIFile.h"
#include "nsIInputStream.h"
+#include "nsNSSCertHelper.h"
#include "nsNSSCertificate.h"
-#include "nsNSSComponent.h"
#include "nsNSSHelper.h"
#include "nsNetUtil.h"
#include "nsReadableUtils.h"
#include "nsString.h"
#include "nsThreadUtils.h"
#include "pkix/pkixtypes.h"
#include "prmem.h"
#include "secerr.h"
using namespace mozilla;
extern LazyLogModule gPIPNSSLog;
#define PIP_PKCS12_TMPFILENAME NS_LITERAL_CSTRING(".pip_p12tmp")
#define PIP_PKCS12_BUFFER_SIZE 2048
-#define PIP_PKCS12_RESTORE_OK 1
-#define PIP_PKCS12_BACKUP_OK 2
#define PIP_PKCS12_USER_CANCELED 3
#define PIP_PKCS12_NOSMARTCARD_EXPORT 4
#define PIP_PKCS12_RESTORE_FAILED 5
#define PIP_PKCS12_BACKUP_FAILED 6
#define PIP_PKCS12_NSS_ERROR 7
// constructor
nsPKCS12Blob::nsPKCS12Blob()
@@ -129,17 +128,16 @@ nsPKCS12Blob::ImportFromFileHelper(nsIFi
if (srv) goto finish;
// validate bags
srv = SEC_PKCS12DecoderValidateBags(dcx, nickname_collision);
if (srv) goto finish;
// import cert and key
srv = SEC_PKCS12DecoderImportBags(dcx);
if (srv) goto finish;
// Later - check to see if this should become default email cert
- handleError(PIP_PKCS12_RESTORE_OK);
finish:
// If srv != SECSuccess, NSS probably set a specific error code.
// We should use that error code instead of inventing a new one
// for every error possible.
if (srv != SECSuccess) {
if (SEC_ERROR_BAD_PASSWORD == PORT_GetError()) {
if (unicodePw.len == sizeof(char16_t))
{
@@ -300,17 +298,16 @@ nsPKCS12Blob::ExportToFile(nsIFile *file
file = localFileRef;
}
rv = file->OpenNSPRFileDesc(PR_RDWR|PR_CREATE_FILE|PR_TRUNCATE, 0664,
&mTmpFile);
if (NS_FAILED(rv) || !this->mTmpFile) goto finish;
// encode and write
srv = SEC_PKCS12Encode(ecx, write_export_file, this);
if (srv) goto finish;
- handleError(PIP_PKCS12_BACKUP_OK);
finish:
if (NS_FAILED(rv) || srv != SECSuccess) {
handleError(PIP_PKCS12_BACKUP_FAILED);
}
if (ecx)
SEC_PKCS12DestroyExportContext(ecx);
if (this->mTmpFile) {
PR_Close(this->mTmpFile);
@@ -424,27 +421,25 @@ nsPKCS12Blob::inputToDecoder(SEC_PKCS12D
}
// nickname_collision
// what to do when the nickname collides with one already in the db.
// TODO: not handled, throw a dialog allowing the nick to be changed?
SECItem *
nsPKCS12Blob::nickname_collision(SECItem *oldNick, PRBool *cancel, void *wincx)
{
- static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);
-
nsNSSShutDownPreventionLock locker;
*cancel = false;
- nsresult rv;
- nsCOMPtr<nsINSSComponent> nssComponent(do_GetService(kNSSComponentCID, &rv));
- if (NS_FAILED(rv)) return nullptr;
int count = 1;
nsCString nickname;
nsAutoString nickFromProp;
- nssComponent->GetPIPNSSBundleString("P12DefaultNickname", nickFromProp);
+ nsresult rv = GetPIPNSSBundleString("P12DefaultNickname", nickFromProp);
+ if (NS_FAILED(rv)) {
+ return nullptr;
+ }
NS_ConvertUTF16toUTF8 nickFromPropC(nickFromProp);
// The user is trying to import a PKCS#12 file that doesn't have the
// attribute we use to set the nickname. So in order to reduce the
// number of interactions we require with the user, we'll build a nickname
// for the user. The nickname isn't prominently displayed in the UI,
// so it's OK if we generate one on our own here.
// XXX If the NSS API were smarter and actually passed a pointer to
// the CERTCertificate* we're importing we could actually just
@@ -511,32 +506,28 @@ pip_ucs2_ascii_conversion_fn(PRBool toUn
*outBufLen = inBufLen;
memcpy(outBuf, inBuf, inBufLen);
return true;
}
void
nsPKCS12Blob::handleError(int myerr)
{
- static NS_DEFINE_CID(kNSSComponentCID, NS_NSSCOMPONENT_CID);
-
+ MOZ_ASSERT(NS_IsMainThread());
if (!NS_IsMainThread()) {
- NS_ERROR("nsPKCS12Blob::handleError called off the mai nthread.");
return;
}
int prerr = PORT_GetError();
MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("PKCS12: NSS/NSPR error(%d)", prerr));
MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("PKCS12: I called(%d)", myerr));
const char * msgID = nullptr;
switch (myerr) {
- case PIP_PKCS12_RESTORE_OK: msgID = "SuccessfulP12Restore"; break;
- case PIP_PKCS12_BACKUP_OK: msgID = "SuccessfulP12Backup"; break;
case PIP_PKCS12_USER_CANCELED:
return; /* Just ignore it for now */
case PIP_PKCS12_NOSMARTCARD_EXPORT: msgID = "PKCS12InfoNoSmartcardBackup"; break;
case PIP_PKCS12_RESTORE_FAILED: msgID = "PKCS12UnknownErrRestore"; break;
case PIP_PKCS12_BACKUP_FAILED: msgID = "PKCS12UnknownErrBackup"; break;
case PIP_PKCS12_NSS_ERROR:
switch (prerr) {
// The following errors have the potential to be "handled", by asking
@@ -558,13 +549,23 @@ nsPKCS12Blob::handleError(int myerr)
case SEC_ERROR_PKCS12_DUPLICATE_DATA: msgID = "PKCS12DupData"; break;
}
break;
}
if (!msgID)
msgID = "PKCS12UnknownErr";
- nsresult rv;
- nsCOMPtr<nsINSSComponent> nssComponent = do_GetService(kNSSComponentCID, &rv);
- if (NS_SUCCEEDED(rv))
- (void) nssComponent->ShowAlertFromStringBundle(msgID);
+ nsCOMPtr<nsIWindowWatcher> wwatch(do_GetService(NS_WINDOWWATCHER_CONTRACTID));
+ if (!wwatch) {
+ return;
+ }
+ nsCOMPtr<nsIPrompt> prompter;
+ if (NS_FAILED(wwatch->GetNewPrompter(nullptr, getter_AddRefs(prompter)))) {
+ return;
+ }
+ nsAutoString message;
+ if (NS_FAILED(GetPIPNSSBundleString(msgID, message))) {
+ return;
+ }
+
+ Unused << prompter->Alert(nullptr, message.get());
}
--- a/security/manager/ssl/tests/unit/test_certDB_import_pkcs12.js
+++ b/security/manager/ssl/tests/unit/test_certDB_import_pkcs12.js
@@ -5,40 +5,90 @@
// Tests import PKCS12 file by nsIX509CertDB.
do_get_profile();
const gCertDB = Cc["@mozilla.org/security/x509certdb;1"]
.getService(Ci.nsIX509CertDB);
+const PKCS12_FILE = "test_certDB_import/cert_from_windows.pfx";
const CERT_COMMON_NAME = "test_cert_from_windows";
const TEST_CERT_PASSWORD = "黒い";
-let gGetPKCS12Password = false;
+// Has getPKCS12FilePassword been called since we last reset this?
+let gGetPKCS12FilePasswordCalled = false;
+let gCurrentTestcase = null;
+
+let gTestcases = [
+ // Test that importing a PKCS12 file with the wrong password fails.
+ {
+ name: "import using incorrect password",
+ filename: PKCS12_FILE,
+ // cancel after the first failed password prompt
+ multiplePasswordPromptsMeans: "cancel",
+ expectingAlert: true,
+ expectedAlertRegexp: /^The password entered was incorrect\.$/,
+ passwordToUse: "this is the wrong password",
+ successExpected: false,
+ },
+ // Test that importing something that isn't a PKCS12 file fails.
+ {
+ name: "import non-PKCS12 file",
+ filename: "test_certDB_import_pkcs12.js",
+ // cancel after the first failed password prompt
+ multiplePasswordPromptsMeans: "cancel",
+ expectingAlert: true,
+ expectedAlertRegexp: /^Failed to decode the file\./,
+ passwordToUse: TEST_CERT_PASSWORD,
+ successExpected: false,
+ },
+ // Test that importing a PKCS12 file with the correct password succeeds.
+ // This needs to be last because currently there isn't a way to delete the
+ // imported certificate (and thus reset the test state) that doesn't depend on
+ // the garbage collector running.
+ {
+ name: "import PKCS12 file",
+ filename: PKCS12_FILE,
+ // If we see more than one password prompt, this is a test failure.
+ multiplePasswordPromptsMeans: "test failure",
+ expectingAlert: false,
+ expectedAlertRegexp: null,
+ passwordToUse: TEST_CERT_PASSWORD,
+ successExpected: true,
+ },
+];
// Mock implementation of nsICertificateDialogs.
const gCertificateDialogs = {
QueryInterface: XPCOMUtils.generateQI([Ci.nsICertificateDialogs]),
getPKCS12FilePassword: (ctx, password) => {
- ok(!gGetPKCS12Password,
- "getPKCS12FilePassword should be called only once.");
+ if (gGetPKCS12FilePasswordCalled) {
+ if (gCurrentTestcase.multiplePasswordPromptsMeans == "cancel") {
+ return false;
+ }
+ ok(false, "getPKCS12FilePassword should be called only once");
+ }
- password.value = TEST_CERT_PASSWORD;
- do_print("getPKCS12FilePassword() is called");
- gGetPKCS12Password = true;
+ password.value = gCurrentTestcase.passwordToUse;
+ do_print("getPKCS12FilePassword() called");
+ gGetPKCS12FilePasswordCalled = true;
return true;
},
};
const gPrompt = {
QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt]),
alert: (title, text) => {
- do_print("alert('" + text + "')");
+ do_print(`alert('${text}')`);
+ ok(gCurrentTestcase.expectingAlert,
+ "alert() should only be called if we're expecting it");
+ ok(gCurrentTestcase.expectedAlertRegexp.test(text),
+ "alert text should match expected message");
},
};
const gPromptFactory = {
QueryInterface: XPCOMUtils.generateQI([Ci.nsIPromptFactory]),
getPrompt: (aWindow, aIID) => gPrompt,
};
@@ -53,28 +103,32 @@ function doesCertExist(commonName) {
if (cert.commonName == commonName) {
return true;
}
}
return false;
}
-function testImportPKCS12Cert() {
+function runOneTestcase(testcase) {
+ do_print(`running ${testcase.name}`);
ok(!doesCertExist(CERT_COMMON_NAME),
- "Cert should not be in the database before import");
+ "cert should not be in the database before import");
- // Import and check for success.
- let certFile = do_get_file("test_certDB_import/cert_from_windows.pfx");
+ // Import and check for failure.
+ let certFile = do_get_file(testcase.filename);
+ ok(certFile, `${testcase.filename} should exist`);
+ gGetPKCS12FilePasswordCalled = false;
+ gCurrentTestcase = testcase;
gCertDB.importPKCS12File(certFile);
+ ok(gGetPKCS12FilePasswordCalled,
+ "getPKCS12FilePassword should have been called");
- ok(gGetPKCS12Password, "PKCS12 password should be asked");
-
- ok(doesCertExist(CERT_COMMON_NAME),
- "Cert should now be found in the database");
+ equal(doesCertExist(CERT_COMMON_NAME), testcase.successExpected,
+ `cert should${testcase.successExpected ? "" : " not"} be found now`);
}
function run_test() {
// We have to set a password and login before we attempt to import anything.
loginToDBWithDefaultPassword();
let certificateDialogsCID =
MockRegistrar.register("@mozilla.org/nsCertificateDialogs;1",
@@ -82,11 +136,12 @@ function run_test() {
let promptFactoryCID =
MockRegistrar.register("@mozilla.org/prompter;1", gPromptFactory);
do_register_cleanup(() => {
MockRegistrar.unregister(certificateDialogsCID);
MockRegistrar.unregister(promptFactoryCID);
});
- // Import PKCS12 file with utf-8 password
- testImportPKCS12Cert();
+ for (let testcase of gTestcases) {
+ runOneTestcase(testcase);
+ }
}