This bug suggests a minimal invasive improvement for the NSS master password storage, by keeping the current mechanism, and increasing the number of iterations.

The requirement is to use a default iteration count that doesn't decrease the performance of Firefox.

Other applications might want to use higher values, but that should be handled in other bugs.

I said "for modern storage". I suggest that we keep the legacy NSS DB storage at the fixed iteration count 1. The legacy storage has lived with the fixed iteration count "1" for a very long time, and going through the hassle of a file format change might not be worth it. Everyone should migrate to the new storage anyway, as it will probably go unsupported in the near future.

This patch is based on Bob's patch for bug 524403.

I've modified it in the following way:

• I removed Bob's code to store the iteration count in the legacy database

• I added a boolean attribute to the softoken file handle, to track if the current database uses legacy storage (that's determined at open time)

• if we're using legacy storage, the iteration count is fixed at 1. This avoids having to deal with versioning of the legacy database.

Bob, does this change to your patch look good?

Martin, Randell, do you have any concerns on this approach?

Here is a NSS test build with the patch:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=c274c8e3a60e7878f8d9fe9b9f64cd4f71de1294

Looking at the runtime of the "Cert" test on aarch64 opt, the total increase for the many tests performed is from 8.5 minutes to 11 minutes.
I'll also start a Firefox try build.

2000 is better than 1, I guess.
Does that number work for you, or would you prefer a lower number, to avoid any performance impact on Firefox?

(In reply to Kai Engert (:kaie:) from comment #3)

I'll also start a Firefox try build.

It can be found here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a3a3a8bc47118105757d560b98cc2c4c48f7176

I don't have experience reading Talos comparisons, could you please help understand if this change has a negative performance impact?
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=2a3a3a8bc47118105757d560b98cc2c4c48f7176

Just pinging for answers to my review questions so I can give it an r+:).

bob

I expect to have a new patch and answers ready by tomorrow.

(In reply to Robert Relyea from comment #5)

I'd like to see a run time setting for the iteration count rather than
a compile time.

Ok, this is aligned with Martin's request, who also asked for an environment variable. I'll add a compile time default, and two environment variables to define a maximum or a minimum count.

The minimum can be used by applications that would like to raise the library default.
The maximum can be used in test environments to limit the iterations for speedup.

I think we already have an environment variable we use for
the pkcs12 code, though I could be remembering incorrectly.

No, that currently uses compile time definition, only:
https://hg.mozilla.org/projects/nss/file/tip/lib/pkcs7/p7create.c#l21

why are we doing this check down here rather than at the
higher level when we select the iterationCount?

I previously wasn't diligent, I had not attempted to fully understand the flow/order of function calls.
After reading the code in more detail, I agree with your suggestion.

Updated library patch based on feedback.

As also stated in bug 1562683 comment 12, this patch:

• raises the default count for modern sql key4
• add the code that can optionally read/write the iteration count from legacy dbm key3
• keep the default for DBM at 1
• add an environment variable, than an application can set, to request that NSS enables the higher count for DBM, too

Also, the environment variables as described in comment 8.

I earlier mentioned that we should have an API for that. However, that code lives in the softokn module, which doesn't have regular APIs, only PKCS#11 APIs.
Given that we need to support environment variables anyway for overriding, it seems easiest to use that as the only offered API for the iteration count.

Supporting the higher iteration count in key3.db DBM storage requires the introduction of a new file format revision.

As soon as a newer version of software writes keys produced with an iteration != 1 to the key3.db file, older software versions will fail to work with that key3.db file.

To avoid introducing this incompatibility to environments that use a mix of different software versions, the increased iteration count shouldn't be rolled out immediately.
Instead, there should be two rollout steps.

In a first step (as suggested in this patch), we'd roll out library code that is able to read the iteration count from the legacy storage, if it's present. If it's missing, count 1 is assumed. When writing key3.db, this software version would continue to use count 1 by default, if the legacy key3.db storage is used.

The intention is to allow a few months of time for system administrators to consume this intermediate software version, in the hope that after this delay, all NSS versions deployed in mixed software environments will run at least the NSS version that is able to understand this enhanced key3.db storage.

Consumers who don't worry about mixed software version deplyoments, can set environment variable NSS_ALLOW_LEGACY_DBM_ITERATION_COUNT that allows writing the higher iteration count to legacy key3.db databases immediately.

In a second step, at a later time that is to be determined (tracked by bug 1562683), NSS could be changed to enable using and storing a higher iteration count in legacy key3.db by default. (Setting the environment variable NSS_ALLOW_LEGACY_DBM_ITERATION_COUNT would no longer be necessary, and the variable will be ignored.)

Bob, the other patch I request to review is based on your original patch from bug 524403 attachment 9074180 [details] [diff] [review].

To ease your review, this additional attachment shows the changes I made on top of your original patch.
I think I have addressed your comments.

Using the fprintf trace statement that you see in the patch (commented out), I interactively tested the behavior of the environment variables with DBM and SQL storage. (Using certutil -W, pk12util import/export.)

This patch sets the environment variable for NSS CI, to limit the iterations while testing. I'll postpone the request for review until we've agreed on env var naming etc.

try build:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=db6dccf8367b94a4722880d4acd86807342d3fda

Firefox try talos:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=931c1d74d9e6083cd918993b2f8d7b13cc923d52

Does this talos comparison show any performance impact?
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=53ebedebdb63ded40f5f3fef677ccc60fa6deeac&newProject=try&newRevision=931c1d74d9e6083cd918993b2f8d7b13cc923d52&framework=1

Kai, there is a single place you can add this NSS_MAX_MP_PBE_ITERATION_COUNT override in queue.js, which should avoid the need to replicate the change in multiple places: https://searchfox.org/nss/rev/a01e1099314b89e2e544c2646e4fa235d1faab10/automation/taskcluster/graph/src/queue.js#93

That seems to work, nss-try is still fast with this taskcluster change:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=5f3aa0f3a501c1d10bd5a975f9257b85e942c422

(In reply to Martin Thomson [:mt:] from comment #23)

Splinter still?

Yes, I still use it for small stuff. I consider it less overhead than going through phab.

To correctly record that Bob wrote the majority of this code, I split the patch into separate commits.
https://hg.mozilla.org/projects/nss/rev/6619bb43d746307df1db57bdd5bbaa17aaec6337
https://hg.mozilla.org/projects/nss/rev/ced91a705aa399e63e2084c608a31faf947a06a4
https://hg.mozilla.org/projects/nss/rev/c8b490583b86a752b585fd11c20fcea05a8a7878

For the release notes:
The master password PBE now uses 10000 iterations by default when using the default sql (key4.db) storage. Because using an iteration count higher than 1 with the legacy dbm (key3.db) storage creates files that are incompatible with previous versions of NSS, applications that wish to enable it for key3.db are required to set environment variable NSS_ALLOW_LEGACY_DBM_ITERATION_COUNT=1.
Applications may set environment variable NSS_MIN_MP_PBE_ITERATION_COUNT to request a higher iteration count than the library's default, or NSS_MAX_MP_PBE_ITERATION_COUNT to request a lower iteration count for test environments.

There are some test failures in fuzz and asan tests. I didn't get those with a previous try run. Let's assume those were infrastructure failures for now. https://treeherder.mozilla.org/#/jobs?repo=nss&revision=c8b490583b86a752b585fd11c20fcea05a8a7878

Release Note Request (optional, but appreciated)
See comment #26

[Why is this notable]:
Increasing data-at-rest protections for users of the Master Password. This feature protects the Firefox profile against offline attacks, like backups.

[Affects Firefox for Android]:
Yes

[Suggested wording]:
When setting or updating a Master Password, the encryption strength is higher. For users currently using the Master Password feature, to get this increase of strength, use the "Change Master Password" button in the "Privacy & Security" Firefox preferences.

[Links (documentation, blog post, etc)]:
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/NSS_3.48_release_notes

Sorry if I don't fully grasp the impact of this bug. Could you please clarify if this means that, if we are currently using a patched version of NSS with Bob's patch for storing iteration count in DBM, that we could move to an unpatched NSS 3.48 now and it would be able to read our current legacy databases with stored rounds? Or would we still need a custom migration patch to return to a standard NSS library?

Mark, please ensure you don't rely on my explanation without testing, no guarantees given.

If your application has been using Bob's earlier patch, it might still work with the most recent NSS code. However, you should ensure that your application sets environment variable NSS_ALLOW_LEGACY_DBM_ITERATION_COUNT=1 very early at startup, prior to NSS init. Assuming the files from your application (Bob's earlier patch) use the same storage format as we've included in NSS 3.48, it should continue to be able to read and write those files.

Thanks Kai, I appreciate the explanation. To be sure the requirements are met I'll probably shortcut the environment variable check with a minimal patch to NSS 3.48 (and keep system-NSS disabled as an option for the time being) and always enable it at build time. I'm a little confused why you built in this double guard (both NSS_ALLOW_LEGACY_DBM_ITERATION_COUNT and NSS_MIN_MP_PBE_ITERATION_COUNT which would default to 1 anyway for dbm) to begin with...

I was hoping that you could tell me if the storage format is actually the same as what was done with Bob's earlier patch. I'll of course do some thorough testing but I'd prefer to avoid unnecessary trial and error, of course.

I can confirm that our previous 3.41 patched version with Bob's patch and NSS 3.48 with the environment variables set early from application initialization are compatible, and that NSS 3.48 has no issues reading the other's db files in an un-patched state.
Thanks for pointing me in the right direction!

IIUC, this would have been applicable to the Fx72 release. Given that Fx73 is about to ship, I think this ship already sailed.