Closed Bug 1468224 Opened 6 years ago Closed 6 years ago

No More Dialogs from nsPKCS12Blob.cpp

Categories

(Core :: Security: PSM, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: jcj, Assigned: dipen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [psm-assigned] [psm-contractor])

Attachments

(1 file)

Three things for this file:

1) nsIX509CertDB.importPKCS12File uses nsICertificateDialogs.getPKCS12FilePassword. It should instead take a password provided by the frontend.

2) nsIX509CertDB.exportPKCS12File uses nsICertificateDialogs.setPKCS12FilePassword. It should instead take a password provided by the frontend (Bug 695043)

3) Errors are reported to the user via nsIPrompt.alert. They should instead be returned by the APIs and handled by the frontend.
nsICertificateDialogs.getPKCS12FilePassword is now called from Javascript layer from certManager.js.  However, reading the text of https://bugzilla.mozilla.org/show_bug.cgi?id=1468225 I am wonder whether I should have called the GUI layer directly.

:keeler can you clarify?
Flags: needinfo?(dkeeler)
I think the summary for bug 1468225 is misleading - basically, we want to remove the interface function (and implementation of) nsICertificateDialogs.viewCert and instead directly open the xul document (for now - that may change in the future as we de-xulify).
Similarly, we want to get rid of nsICertificateDialogs.getPKCS12FilePassword, but I think that can be done in a separate bug.
Flags: needinfo?(dkeeler)
Comment on attachment 8997491 [details]
Bug 1468224 - remove dialogs from nsIX509CertDB PKCS12File methods.

https://reviewboard.mozilla.org/r/261236/#review268300

This looks good. One general nit I wanted to mention was make sure function arguments that are references are formatted like so: `Type& aArg` (i.e. the & is on the left - same situation with * ).
The only substantial change I'd like to see is to push the null password retry logic to the front-end completely. That is, while the backend should be able to differentiate between and empty string ("") and a null string (`null`) for a password, it shouldn't be responsible for the retrying itself - the front-end should. Thanks! (Everything else is really just formatting nits and removing things that aren't used any longer.)

::: security/manager/pki/resources/content/certManager.js:12
(Diff revision 1)
>  const nsIFilePicker = Ci.nsIFilePicker;
>  const nsFilePicker = "@mozilla.org/filepicker;1";
>  const nsIX509CertDB = Ci.nsIX509CertDB;
>  const nsX509CertDB = "@mozilla.org/security/x509certdb;1";
>  const nsIX509Cert = Ci.nsIX509Cert;
> +const nsStringBundle = "@mozilla.org/intl/stringbundle;1";

This is actually an anti-pattern I'd like to get rid of (but not as part of this bug - feel free to file a follow-up). We should just use `Ci.nsIWhatever` and `"mozilla.org/whatever;1"` directly.

::: security/manager/pki/resources/content/certManager.js:215
(Diff revision 1)
>  
>    return false;
>  }
>  
> +function promptError(aErrorCode) {
> +  if (aErrorCode != certdb.Success) {

For constants I find it easier to use the interface directly (e.g. `Ci.nsIX509CertDB.Success`) (that way, it's immediately obvious where these are coming from).

::: security/manager/pki/resources/content/certManager.js:320
(Diff revision 1)
>    fp.open(rv => {
>      if (rv == nsIFilePicker.returnOK || rv == nsIFilePicker.returnReplace) {
> -      certdb.exportPKCS12File(fp.file, selected_certs.length, selected_certs);
> +      let password = {};
> +      if (certdialogs.setPKCS12FilePassword(window, password)) {
> +        let errorCode = certdb.exportPKCS12File(fp.file, selected_certs.length,
> +                      selected_certs, password.value);

nit: align with `fp.file` on previous line

::: security/manager/pki/resources/content/certManager.js:389
(Diff revision 1)
>        };
>        certdb.importUserCertificate(dataArray, dataArray.length, interfaceRequestor);
>      } else {
>        // Otherwise, assume it's a PKCS12 file and import it that way.
> -      certdb.importPKCS12File(fp.file);
> +      let password = {};
> +      let errorCode = certdb.ERROR_BAD_PASSWORD;

Same idea with the constants here.

::: security/manager/pki/resources/content/certManager.js:391
(Diff revision 1)
>      } else {
>        // Otherwise, assume it's a PKCS12 file and import it that way.
> -      certdb.importPKCS12File(fp.file);
> +      let password = {};
> +      let errorCode = certdb.ERROR_BAD_PASSWORD;
> +      while (errorCode == certdb.ERROR_BAD_PASSWORD
> +             && certdialogs.getPKCS12FilePassword(window, password)) {

nit: && on the previous line

::: security/manager/ssl/nsIX509CertDB.idl:188
(Diff revision 1)
>    /**
>     *  Import a PKCS#12 file containing cert(s) and key(s) into the database.
>     *
>     *  @param aFile Identifies a file that contains the data to be imported.
> +   *  @param password The password used to protect the file.
> +   *  @return Success or the specific error code on failure

Maybe specify that the error codes are defined in this interface (i.e. they're not NSPR errors or anything).

::: security/manager/ssl/nsIX509CertDB.idl:205
(Diff revision 1)
> +   *  @param password The password used to protect the file.
> +   *  @return Success or the specific error code on failure
>     */
>    [must_use]
> -  void exportPKCS12File(in nsIFile aFile,
> +  uint32_t exportPKCS12File(in nsIFile aFile,
>                          in unsigned long count,

nit: let's keep these aligned with the `in nsIFile...` parameter from the first line

::: security/manager/ssl/nsNSSCertificateDB.cpp:842
(Diff revision 1)
>  
>    return NS_ERROR_FAILURE;
>  }
>  
>  NS_IMETHODIMP
> -nsNSSCertificateDB::ImportPKCS12File(nsIFile* aFile)
> +nsNSSCertificateDB::ImportPKCS12File(nsIFile* aFile, const nsAString & aPassword,

nit: & to the left

::: security/manager/ssl/nsNSSCertificateDB.cpp:856
(Diff revision 1)
>    }
>  
>    NS_ENSURE_ARG(aFile);
>    nsPKCS12Blob blob;
> -  rv = blob.ImportFromFile(aFile);
> -
> +  rv = blob.ImportFromFile(aFile, aPassword, *aError, false);
> +  if (*aError == nsIX509CertDB::ERROR_BAD_PASSWORD && aPassword.Length() == 0) {

I think the front-end should take care of this retry logic (see some related comments about null passwords below). That is, if the user passes in an empty string and the front-end determines that that isn't the right password, it should retry with `null`.

::: security/manager/ssl/nsPKCS12Blob.h:55
(Diff revision 1)
>    //   An empty password should be represented as an empty string (a SECItem
>    //   that contains a single terminating null UTF16 character), but some
>    //   applications use a zero length SECItem. We try both variations, zero
>    //   length item and empty string, without giving a user prompt when trying
>    //   the different empty password flavors.
>    enum class RetryReason

These two enums can be removed now.

::: security/manager/ssl/nsPKCS12Blob.cpp:11
(Diff revision 1)
>  
>  #include "ScopedNSSTypes.h"
>  #include "mozilla/Assertions.h"
>  #include "mozilla/Casting.h"
>  #include "mozilla/Unused.h"
> +#include "nsIX509CertDB.h"

nit: this sorts later

::: security/manager/ssl/nsPKCS12Blob.cpp:44
(Diff revision 1)
>  }
>  
>  // Given a file handle, read a PKCS#12 blob from that file, decode it, and
>  // import the results into the internal database.
>  nsresult
> -nsPKCS12Blob::ImportFromFile(nsIFile* file)
> +nsPKCS12Blob::ImportFromFile(nsIFile* aFile, const nsAString & aPassword, uint32_t& aError, bool aNullPass)

nit: & to the left (i.e. `const nsAString& aPassword`)

::: security/manager/ssl/nsPKCS12Blob.cpp:54
(Diff revision 1)
>    UniquePK11SlotInfo slot(PK11_GetInternalKeySlot());
>    if (!slot) {
>      return NS_ERROR_FAILURE;
>    }
>  
> -  uint32_t passwordBufferLength;
> +  if (aNullPass) {

I think instead of specially handling null passwords like this, we should push this part down to `stringToBigEndianBytes` (I'll comment there with more details).

::: security/manager/ssl/nsPKCS12Blob.cpp:98
(Diff revision 1)
> -    handleImportError(PR_GetError(), aWantRetry, unicodePw.len);
> +    aError = handlePRErrorCode(PR_GetError());
> -  }
> -  return NS_OK;
> +    return NS_OK;
> -}
> +  }
> +  aError = nsIX509CertDB::Success;
> +  return rv;

Let's just return `NS_OK` here.

::: security/manager/ssl/nsPKCS12Blob.cpp:128
(Diff revision 1)
> -  bool informedUserNoSmartcardBackup = false;
>  
>    // get file password (unicode)
>    uint32_t passwordBufferLength;
>    UniquePtr<uint8_t[]> passwordBuffer;
> -  nsresult rv = newPKCS12FilePassword(passwordBufferLength, passwordBuffer);
> +  nsresult rv;

We can go ahead and move this declaration to where it's first assigned.

::: security/manager/ssl/nsPKCS12Blob.cpp:222
(Diff revision 1)
>  }
>  
>  // For the NSS PKCS#12 library, must convert PRUnichars (shorts) to a buffer of
>  // octets. Must handle byte order correctly.
>  UniquePtr<uint8_t[]>
> -nsPKCS12Blob::stringToBigEndianBytes(const nsString& uni, uint32_t& bytesLength)
> +nsPKCS12Blob::stringToBigEndianBytes(const nsAString& uni, uint32_t& bytesLength)

One of our string classes has `IsVoid`, which I think we can use to differentiate between an empty string ("") and `null` (in JS) or `VoidString` (in C++). Let's check for that and return `null`/set `bytesLength` to `0` here.

::: security/manager/ssl/nsPKCS12Blob.cpp:344
(Diff revision 1)
> -    case PIP_PKCS12_BACKUP_FAILED:
> -      msgID = "PKCS12UnknownErrBackup";
> -      break;
> -    case PIP_PKCS12_NSS_ERROR:
> -      switch (prerr) {
> -        case 0:
> +    case 0:

I think encountering 0 would be an error here - add an assertion?

::: security/manager/ssl/nsPKCS12Blob.cpp:352
(Diff revision 1)
> -        case SEC_ERROR_PKCS12_CERT_COLLISION:
> +    case SEC_ERROR_PKCS12_CERT_COLLISION:
> -          msgID = "PKCS12DupData";
> +      error = nsIX509CertDB::ERROR_PKCS12_DUPLICATE_DATA;
> -          break;
> +      break;
> +    // INVALID_ARGS is returned on bad password when importing cert
> +    // exported from firefox or generated by openssl
> +    case SEC_ERROR_INVALID_ARGS:

Do you have a link to where in NSS this error is set? I think this is a bug (or at least a not great situation that we could improve).

::: security/manager/ssl/tests/unit/test_certDB_export_pkcs12.js:38
(Diff revision 1)
>    // Import the certificate and key so we have something to export.
>    let cert = findCertByCommonName(CERT_COMMON_NAME);
>    equal(cert, null, "cert should not be found before import");
>    let certFile = do_get_file(PKCS12_FILE);
>    ok(certFile, `${PKCS12_FILE} should exist`);
>    gPasswordToUse = TEST_CERT_PASSWORD;

Looks like `gPasswordToUse` was only necessary because of the dialog - we can use `TEST_CERT_PASSWORD` directly now.

::: security/manager/ssl/tests/unit/test_certDB_import_pkcs12.js:85
(Diff revision 1)
> -};
> -
> -const gPrompt = {
> -  QueryInterface: ChromeUtils.generateQI([Ci.nsIPrompt]),
> -  alert: (title, text) => {
> -    info(`alert('${text}')`);
> +  // Same cert file protected with no password
> +  {
> +    name: "import PKCS12 file no password",
> +    filename: PKCS12_FILE_NO_PASS,
> +    // If we see more than one password prompt, this is a test failure.
> +    multiplePasswordPromptsMeans: "test failure",

It looks like `multiplePasswordPromptsMeans` isn't used now, so these can all be deleted (same with `expectingAlert` and `expectedalertRegexp`).
Attachment #8997491 - Flags: review?(dkeeler) → review-
Comment on attachment 8997491 [details]
Bug 1468224 - remove dialogs from nsIX509CertDB PKCS12File methods.

https://reviewboard.mozilla.org/r/261236/#review268300

> This is actually an anti-pattern I'd like to get rid of (but not as part of this bug - feel free to file a follow-up). We should just use `Ci.nsIWhatever` and `"mozilla.org/whatever;1"` directly.

Created [Bug 1480925](https://bugzilla.mozilla.org/show_bug.cgi?id=1480925)

> Do you have a link to where in NSS this error is set? I think this is a bug (or at least a not great situation that we could improve).

All certs seem to go through the path to where SEC_ERROR_BAD_PASSWORD error is set in sec_pkcs12_decoder_safe_contents_callback() when an invalid password is supplied.

However, something seems to happen after that which hists the SEC_ERROR_INVALID_ARGS error in source/security/nss/lib/pkcs12/p12d.c SEC_PKCS12DecoderUpdate(). The original test cert test_certDB_import/cert_from_windows_emptypass.pfx doesn't hit this path.  However, the openssl generated p12 cert and exported versions of the orginal test_certDB_import/cert_from_windows_emptypass.pfx cert do hit this error.
Comment on attachment 8997491 [details]
Bug 1468224 - remove dialogs from nsIX509CertDB PKCS12File methods.

https://reviewboard.mozilla.org/r/261236/#review268608

Great - r=me. I thought we had some integration mochitests for importing PKCS#12 files, but it looks like we don't (we have one for exporting: https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/security/manager/ssl/tests/mochitest/browser/browser_exportP12_passwordUI.js ). I think it would be good to have coverage for at least: a) correct, non-empty password, b) incorrect password, c) empty password -> null password fallback. This can be a follow-up, though.

::: security/manager/ssl/tests/unit/test_certDB_export_pkcs12_with_master_password.js:29
(Diff revisions 1 - 2)
>    QueryInterface: ChromeUtils.generateQI([Ci.nsIPrompt]),
>  
>    // This intentionally does not use arrow function syntax to avoid an issue
>    // where in the context of the arrow function, |this != gPrompt| due to
>    // how objects get wrapped when going across xpcom boundaries.
>    alert(title, text) {

Looks like I missed this last time - we're not expecting any alerts now, right? I'm pretty sure we can just remove all `expectingAlert` and `expectingAlertRegexp` references, and put something like `ok(false, "not expecting alert() to be called");` in the body of `alert`.
Attachment #8997491 - Flags: review?(dkeeler) → review+
Comment on attachment 8997491 [details]
Bug 1468224 - remove dialogs from nsIX509CertDB PKCS12File methods.

https://reviewboard.mozilla.org/r/261236/#review268300

> All certs seem to go through the path to where SEC_ERROR_BAD_PASSWORD error is set in sec_pkcs12_decoder_safe_contents_callback() when an invalid password is supplied.
> 
> However, something seems to happen after that which hists the SEC_ERROR_INVALID_ARGS error in source/security/nss/lib/pkcs12/p12d.c SEC_PKCS12DecoderUpdate(). The original test cert test_certDB_import/cert_from_windows_emptypass.pfx doesn't hit this path.  However, the openssl generated p12 cert and exported versions of the orginal test_certDB_import/cert_from_windows_emptypass.pfx cert do hit this error.

Oh, I see. Nevermind, then.
Pushed by dvarga@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6d894ce78322
remove dialogs from nsIX509CertDB PKCS12File methods. r=keeler
Keywords: checkin-needed
Backed out changeset 08fa47a24e89 (Bug 1445451) for failing Btup

Backout:
https://hg.mozilla.org/integration/autoland/rev/6d19dd7ed3e564b88ab5b962e90ebf73c21fc257

Push link;
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=08fa47a24e89697e4e43177860b55cc28298bbff&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Failure log:
[task 2018-08-08T22:39:57.269Z] 22:39:57     INFO -  tup error: File '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/lmdb-sys-5a4ee75129adb007/out/liblmdb.a' was written to, but is not in .tup/db. You probably should specify it as an output
[task 2018-08-08T22:39:57.270Z] 22:39:57     INFO -   -- Delete: /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/lmdb-sys-5a4ee75129adb007/out/liblmdb.a
[task 2018-08-08T22:39:57.271Z] 22:39:57     INFO -  tup error: File '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/lmdb-sys-5a4ee75129adb007/out/midl.o' was written to, but is not in .tup/db. You probably should specify it as an output
[task 2018-08-08T22:39:57.272Z] 22:39:57     INFO -   -- Delete: /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/lmdb-sys-5a4ee75129adb007/out/midl.o
[task 2018-08-08T22:39:57.273Z] 22:39:57     INFO -  tup error: File '/builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/lmdb-sys-5a4ee75129adb007/out/mdb.o' was written to, but is not in .tup/db. You probably should specify it as an output
[task 2018-08-08T22:39:57.274Z] 22:39:57     INFO -   -- Delete: /builds/worker/workspace/build/src/obj-firefox/toolkit/library/x86_64-unknown-linux-gnu/release/build/lmdb-sys-5a4ee75129adb007/out/mdb.o
[task 2018-08-08T22:39:57.275Z] 22:39:57     INFO -   *** Command failed due to errors processing the output dependencies.
[task 2018-08-08T22:40:23.427Z] 22:40:23     INFO -   *** tup: 1 job failed.
[task 2018-08-08T22:40:23.507Z] 22:40:23     INFO -  53 compiler warnings present.
[task 2018-08-08T22:40:23.562Z] 22:40:23    ERROR - Return code: 1
[task 2018-08-08T22:40:23.562Z] 22:40:23  WARNING - setting return code to 2
[task 2018-08-08T22:40:23.562Z] 22:40:23    FATAL - 'mach build -v' did not run successfully. Please check log for errors.
[task 2018-08-08T22:40:23.562Z] 22:40:23    FATAL - Running post_fatal callback...
[task 2018-08-08T22:40:23.562Z] 22:40:23    FATAL - Exiting -1
[task 2018-08-08T22:40:23.562Z] 22:40:23     INFO - [mozharness: 2018-08-08 22:40:23.562588Z] Finished build step (failed)
[task 2018-08-08T22:40:23.562Z] 22:40:23     INFO - Running post-run listener: _summarize
[task 2018-08-08T22:40:23.562Z] 22:40:23    ERROR - # TBPL FAILURE #
[task 2018-08-08T22:40:23.562Z] 22:40:23     INFO - [mozharness: 2018-08-08 22:40:23.562826Z] FxDesktopBuild summary:
[task 2018-08-08T22:40:23.562Z] 22:40:23    ERROR - # TBPL FAILURE #
[task 2018-08-08T22:40:23.562Z] 22:40:23     INFO - Running post-run listener: copy_logs_to_upload_dir
[task 2018-08-08T22:40:23.563Z] 22:40:23     INFO - Copying logs to upload dir...
https://hg.mozilla.org/mozilla-central/rev/6d894ce78322
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.