# HG changeset patch # Parent c8f9e7eadcb3068b796b575310a85411a0d90da1 diff --git a/security/manager/ssl/nsCertTree.cpp b/security/manager/ssl/nsCertTree.cpp --- a/security/manager/ssl/nsCertTree.cpp +++ b/security/manager/ssl/nsCertTree.cpp @@ -16,6 +16,7 @@ #include "nsIX509CertValidity.h" #include "nsNSSCertHelper.h" #include "nsNSSCertificate.h" +#include "nsNSSCertificateDB.h" #include "nsNSSComponent.h" // for PIPNSS string bundle calls. #include "nsNSSHelper.h" #include "nsReadableUtils.h" @@ -800,8 +801,8 @@ nsCertTree::DeleteEntryObject(uint32_t i SECStatus srv = CERT_DecodeTrustString(&trust, ""); // no override if (srv == SECSuccess) { - CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), nsscert.get(), - &trust); + ChangeCertTrustWithPossibleAuthentication(nsscert, trust, + nullptr); } } } diff --git a/security/manager/ssl/nsNSSCertTrust.h b/security/manager/ssl/nsNSSCertTrust.h --- a/security/manager/ssl/nsNSSCertTrust.h +++ b/security/manager/ssl/nsNSSCertTrust.h @@ -57,8 +57,7 @@ public: /* set p <--> P */ void AddPeerTrust(bool ssl, bool email, bool objSign); - /* get it (const?) (shallow?) */ - CERTCertTrust * GetTrust() { return &mTrust; } + CERTCertTrust& GetTrust() { return mTrust; } private: void addTrust(unsigned int *t, unsigned int v); diff --git a/security/manager/ssl/nsNSSCertificateDB.cpp b/security/manager/ssl/nsNSSCertificateDB.cpp --- a/security/manager/ssl/nsNSSCertificateDB.cpp +++ b/security/manager/ssl/nsNSSCertificateDB.cpp @@ -232,6 +232,38 @@ nsNSSCertificateDB::getCertsFromPackage( return collectArgs; } +// When using the sql-backed softoken, trust settings are authenticated using a +// key in the secret database. Thus, if the user has a password, we need to +// authenticate to the token in order to be able to change trust settings. +SECStatus +ChangeCertTrustWithPossibleAuthentication(const UniqueCERTCertificate& cert, + CERTCertTrust& trust, void* ctx) +{ + MOZ_ASSERT(cert, "cert must be non-null"); + if (!cert) { + PR_SetError(SEC_ERROR_LIBRARY_FAILURE, 0); + return SECFailure; + } + // NSS ignores the first argument to CERT_ChangeCertTrust + SECStatus srv = CERT_ChangeCertTrust(nullptr, cert.get(), &trust); + if (srv == SECSuccess || PR_GetError() != SEC_ERROR_TOKEN_NOT_LOGGED_IN) { + return srv; + } + if (cert->slot) { + // If this certificate is on an external PKCS#11 token, we have to + // authenticate to that token. + srv = PK11_Authenticate(cert->slot, PR_TRUE, ctx); + } else { + // Otherwise, the certificate is on the internal module. + UniquePK11SlotInfo internalSlot(PK11_GetInternalKeySlot()); + srv = PK11_Authenticate(internalSlot.get(), PR_TRUE, ctx); + } + if (srv != SECSuccess) { + return srv; + } + return CERT_ChangeCertTrust(nullptr, cert.get(), &trust); +} + nsresult nsNSSCertificateDB::handleCACertDownload(NotNull x509Certs, nsIInterfaceRequestor *ctx, @@ -356,8 +388,8 @@ nsNSSCertificateDB::handleCACertDownload if (srv != SECSuccess) { return MapSECStatus(srv); } - // NSS ignores the first argument to CERT_ChangeCertTrust - srv = CERT_ChangeCertTrust(nullptr, tmpCert.get(), trust.GetTrust()); + srv = ChangeCertTrustWithPossibleAuthentication(tmpCert, trust.GetTrust(), + ctx); if (srv != SECSuccess) { return MapSECStatus(srv); } @@ -798,8 +830,8 @@ nsNSSCertificateDB::DeleteCertificate(ns // the cert onto the card again at which point we *will* want to // trust that cert if it chains up properly. nsNSSCertTrust trust(0, 0, 0); - srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), - cert.get(), trust.GetTrust()); + srv = ChangeCertTrustWithPossibleAuthentication(cert, trust.GetTrust(), + nullptr); } MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("cert deleted: %d", srv)); return (srv) ? NS_ERROR_FAILURE : NS_OK; @@ -815,38 +847,33 @@ nsNSSCertificateDB::SetCertTrust(nsIX509 if (isAlreadyShutDown()) { return NS_ERROR_NOT_AVAILABLE; } - nsNSSCertTrust trust; - nsresult rv; - UniqueCERTCertificate nsscert(cert->GetCert()); - SECStatus srv; - if (type == nsIX509Cert::CA_CERT) { - // always start with untrusted and move up - trust.SetValidCA(); - trust.AddCATrust(!!(trusted & nsIX509CertDB::TRUSTED_SSL), - !!(trusted & nsIX509CertDB::TRUSTED_EMAIL), - !!(trusted & nsIX509CertDB::TRUSTED_OBJSIGN)); - srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), - nsscert.get(), - trust.GetTrust()); - } else if (type == nsIX509Cert::SERVER_CERT) { - // always start with untrusted and move up - trust.SetValidPeer(); - trust.AddPeerTrust(trusted & nsIX509CertDB::TRUSTED_SSL, 0, 0); - srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), - nsscert.get(), - trust.GetTrust()); - } else if (type == nsIX509Cert::EMAIL_CERT) { - // always start with untrusted and move up - trust.SetValidPeer(); - trust.AddPeerTrust(0, !!(trusted & nsIX509CertDB::TRUSTED_EMAIL), 0); - srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), - nsscert.get(), - trust.GetTrust()); - } else { - // ignore user certs - return NS_OK; + nsNSSCertTrust trust; + switch (type) { + case nsIX509Cert::CA_CERT: + trust.SetValidCA(); + trust.AddCATrust(!!(trusted & nsIX509CertDB::TRUSTED_SSL), + !!(trusted & nsIX509CertDB::TRUSTED_EMAIL), + !!(trusted & nsIX509CertDB::TRUSTED_OBJSIGN)); + break; + case nsIX509Cert::SERVER_CERT: + trust.SetValidPeer(); + trust.AddPeerTrust(trusted & nsIX509CertDB::TRUSTED_SSL, false, false); + break; + case nsIX509Cert::EMAIL_CERT: + trust.SetValidPeer(); + trust.AddPeerTrust(false, !!(trusted & nsIX509CertDB::TRUSTED_EMAIL), + false); + break; + default: + // Ignore any other type of certificate (including invalid types). + return NS_OK; } + + UniqueCERTCertificate nsscert(cert->GetCert()); + SECStatus srv = ChangeCertTrustWithPossibleAuthentication(nsscert, + trust.GetTrust(), + nullptr); return MapSECStatus(srv); } @@ -1311,7 +1338,7 @@ nsNSSCertificateDB::AddCertFromBase64(co } nsNSSCertTrust trust; - if (CERT_DecodeTrustString(trust.GetTrust(), PromiseFlatCString(aTrust).get()) + if (CERT_DecodeTrustString(&trust.GetTrust(), PromiseFlatCString(aTrust).get()) != SECSuccess) { return NS_ERROR_FAILURE; } @@ -1344,8 +1371,8 @@ nsNSSCertificateDB::AddCertFromBase64(co if (srv != SECSuccess) { return MapSECStatus(srv); } - // NSS ignores the first argument to CERT_ChangeCertTrust - srv = CERT_ChangeCertTrust(nullptr, tmpCert.get(), trust.GetTrust()); + srv = ChangeCertTrustWithPossibleAuthentication(tmpCert, trust.GetTrust(), + nullptr); return MapSECStatus(srv); } @@ -1373,7 +1400,7 @@ nsNSSCertificateDB::SetCertTrustFromStri } UniqueCERTCertificate nssCert(cert->GetCert()); - srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), nssCert.get(), &trust); + srv = ChangeCertTrustWithPossibleAuthentication(nssCert, trust, nullptr); return MapSECStatus(srv); } diff --git a/security/manager/ssl/nsNSSCertificateDB.h b/security/manager/ssl/nsNSSCertificateDB.h --- a/security/manager/ssl/nsNSSCertificateDB.h +++ b/security/manager/ssl/nsNSSCertificateDB.h @@ -74,4 +74,8 @@ private: {0xb3, 0x2c, 0x80, 0x12, 0x46, 0x93, 0xd8, 0x71} \ } +SECStatus +ChangeCertTrustWithPossibleAuthentication( + const mozilla::UniqueCERTCertificate& cert, CERTCertTrust& trust, void* ctx); + #endif // nsNSSCertificateDB_h diff --git a/security/manager/ssl/nsNSSComponent.cpp b/security/manager/ssl/nsNSSComponent.cpp --- a/security/manager/ssl/nsNSSComponent.cpp +++ b/security/manager/ssl/nsNSSComponent.cpp @@ -682,8 +682,8 @@ nsNSSComponent::MaybeImportFamilySafetyR 0, 0 }; - if (CERT_ChangeCertTrust(nullptr, nssCertificate.get(), &trust) - != SECSuccess) { + if (ChangeCertTrustWithPossibleAuthentication(nssCertificate, trust, + nullptr) != SECSuccess) { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't trust certificate for TLS server auth")); return NS_ERROR_FAILURE; @@ -769,8 +769,8 @@ nsNSSComponent::UnloadFamilySafetyRoot() // they're the same. To work around this, we set a non-zero flag to ensure // that the trust will get updated. CERTCertTrust trust = { CERTDB_USER, 0, 0 }; - if (CERT_ChangeCertTrust(nullptr, mFamilySafetyRoot.get(), &trust) - != SECSuccess) { + if (ChangeCertTrustWithPossibleAuthentication(mFamilySafetyRoot, trust, + nullptr) != SECSuccess) { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't untrust certificate for TLS server auth")); } @@ -912,7 +912,9 @@ nsNSSComponent::UnloadEnterpriseRoots() ("library failure: CERTCertListNode null or lacks cert")); continue; } - if (CERT_ChangeCertTrust(nullptr, n->cert, &trust) != SECSuccess) { + UniqueCERTCertificate cert(CERT_DupCertificate(n->cert)); + if (ChangeCertTrustWithPossibleAuthentication(cert, trust, nullptr) + != SECSuccess) { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't untrust certificate for TLS server auth")); } @@ -1068,8 +1070,8 @@ nsNSSComponent::ImportEnterpriseRootsFor MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't add cert to list")); continue; } - if (CERT_ChangeCertTrust(nullptr, nssCertificate.get(), &trust) - != SECSuccess) { + if (ChangeCertTrustWithPossibleAuthentication(nssCertificate, trust, + nullptr) != SECSuccess) { MOZ_LOG(gPIPNSSLog, LogLevel::Debug, ("couldn't trust certificate for TLS server auth")); } diff --git a/security/manager/ssl/tests/unit/test_certDB_import_with_master_password.js b/security/manager/ssl/tests/unit/test_certDB_import_with_master_password.js new file mode 100644 --- /dev/null +++ b/security/manager/ssl/tests/unit/test_certDB_import_with_master_password.js @@ -0,0 +1,120 @@ +// -*- indent-tabs-mode: nil; js-indent-level: 2 -*- +// Any copyright is dedicated to the Public Domain. +// http://creativecommons.org/publicdomain/zero/1.0/ +"use strict"; + +// Tests that a CA certificate can still be imported if the user has a master +// password set. + +do_get_profile(); + +const gCertDB = Cc["@mozilla.org/security/x509certdb;1"] + .getService(Ci.nsIX509CertDB); + +const CA_CERT_COMMON_NAME = "importedCA"; + +let gCACertImportDialogCount = 0; + +// Mock implementation of nsICertificateDialogs. +const gCertificateDialogs = { + confirmDownloadCACert: (ctx, cert, trust) => { + gCACertImportDialogCount++; + equal(cert.commonName, CA_CERT_COMMON_NAME, + "CA cert to import should have the correct CN"); + trust.value = Ci.nsIX509CertDB.TRUSTED_EMAIL; + return true; + }, + setPKCS12FilePassword: (ctx, password) => { + // This is only relevant to exporting. + ok(false, "setPKCS12FilePassword() should not have been called"); + }, + getPKCS12FilePassword: (ctx, password) => { + // We don't test anything that calls this method yet. + ok(false, "getPKCS12FilePassword() should not have been called"); + }, + viewCert: (ctx, cert) => { + // This shouldn't be called for import methods. + ok(false, "viewCert() should not have been called"); + }, + + QueryInterface: XPCOMUtils.generateQI([Ci.nsICertificateDialogs]) +}; + +var gMockPrompter = { + passwordToTry: "password", + numPrompts: 0, + + // This intentionally does not use arrow function syntax to avoid an issue + // where in the context of the arrow function, |this != gMockPrompter| due to + // how objects get wrapped when going across xpcom boundaries. + promptPassword(dialogTitle, text, password, checkMsg, checkValue) { + this.numPrompts++; + if (this.numPrompts > 1) { // don't keep retrying a bad password + return false; + } + equal(text, + "Please enter the master password for the Software Security Device.", + "password prompt text should be as expected"); + equal(checkMsg, null, "checkMsg should be null"); + ok(this.passwordToTry, "passwordToTry should be non-null"); + password.value = this.passwordToTry; + return true; + }, + + QueryInterface: XPCOMUtils.generateQI([Ci.nsIPrompt]), + + // Again with the arrow function issue. + getInterface(iid) { + if (iid.equals(Ci.nsIPrompt)) { + return this; + } + + throw new Error(Cr.NS_ERROR_NO_INTERFACE); + } +}; + +function getCertAsByteArray(certPath) { + let certFile = do_get_file(certPath, false); + let certBytes = readFile(certFile); + + let byteArray = []; + for (let i = 0; i < certBytes.length; i++) { + byteArray.push(certBytes.charCodeAt(i)); + } + + return byteArray; +} + +function run_test() { + let certificateDialogsCID = + MockRegistrar.register("@mozilla.org/nsCertificateDialogs;1", + gCertificateDialogs); + do_register_cleanup(() => { + MockRegistrar.unregister(certificateDialogsCID); + }); + + // Set a master password. + let tokenDB = Cc["@mozilla.org/security/pk11tokendb;1"] + .getService(Ci.nsIPK11TokenDB); + let token = tokenDB.getInternalKeyToken(); + token.initPassword("password"); + token.logoutSimple(); + + // Sanity check the CA cert is missing. + throws(() => gCertDB.findCertByNickname(CA_CERT_COMMON_NAME), + /NS_ERROR_FAILURE/, + "CA cert should not be in the database before import"); + + // Import and check for success. + let caArray = getCertAsByteArray("test_certDB_import/importedCA.pem"); + gCertDB.importCertificates(caArray, caArray.length, Ci.nsIX509Cert.CA_CERT, + gMockPrompter); + equal(gCACertImportDialogCount, 1, + "Confirmation dialog for the CA cert should only be shown once"); + + let caCert = gCertDB.findCertByNickname(CA_CERT_COMMON_NAME); + notEqual(caCert, null, "CA cert should now be found in the database"); + ok(gCertDB.isCertTrusted(caCert, Ci.nsIX509Cert.CA_CERT, + Ci.nsIX509CertDB.TRUSTED_EMAIL), + "CA cert should be trusted for e-mail"); +} diff --git a/security/manager/ssl/tests/unit/xpcshell.ini b/security/manager/ssl/tests/unit/xpcshell.ini --- a/security/manager/ssl/tests/unit/xpcshell.ini +++ b/security/manager/ssl/tests/unit/xpcshell.ini @@ -55,6 +55,7 @@ run-sequentially = hardcoded ports [test_cert_version.js] [test_certDB_import.js] [test_certDB_import_pkcs12.js] +[test_certDB_import_with_master_password.js] [test_certviewer_invalid_oids.js] skip-if = toolkit == 'android' [test_constructX509FromBase64.js]