Increase NSS MP KDF default iteration count, by default for modern key4 storage, optionally for legacy key3.db storage
Categories
(NSS :: Libraries, enhancement, P2)
Tracking
(relnote-firefox -)
Tracking | Status | |
---|---|---|
relnote-firefox | --- | - |
People
(Reporter: KaiE, Assigned: KaiE)
References
(Blocks 3 open bugs)
Details
(Whiteboard: [relnotes: comment 26])
Attachments
(2 files, 4 obsolete files)
765 bytes,
patch
|
mt
:
review+
|
Details | Diff | Splinter Review |
38.30 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 2•5 years ago
|
||
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?
Assignee | ||
Comment 3•5 years ago
|
||
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?
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 4•5 years ago
|
||
(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
Comment 5•5 years ago
|
||
Comment 6•5 years ago
|
||
Just pinging for answers to my review questions so I can give it an r+:).
bob
Assignee | ||
Comment 7•5 years ago
|
||
I expect to have a new patch and answers ready by tomorrow.
Assignee | ||
Comment 8•5 years ago
|
||
(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.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
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.)
Assignee | ||
Comment 10•5 years ago
|
||
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.)
Assignee | ||
Comment 11•5 years ago
|
||
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.
Assignee | ||
Comment 12•5 years ago
|
||
Assignee | ||
Comment 13•5 years ago
|
||
Assignee | ||
Comment 14•5 years ago
|
||
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
Comment 15•5 years ago
|
||
Assignee | ||
Comment 16•5 years ago
|
||
Assignee | ||
Comment 17•5 years ago
|
||
Comment 18•5 years ago
|
||
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
Assignee | ||
Comment 19•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 20•5 years ago
|
||
Assignee | ||
Comment 21•5 years ago
|
||
That seems to work, nss-try is still fast with this taskcluster change:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=5f3aa0f3a501c1d10bd5a975f9257b85e942c422
Assignee | ||
Comment 22•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 23•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
(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.
Assignee | ||
Comment 25•5 years ago
|
||
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
Assignee | ||
Comment 26•5 years ago
|
||
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.
Assignee | ||
Comment 27•5 years ago
|
||
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
Comment 31•5 years ago
•
|
||
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
Comment 32•5 years ago
|
||
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?
Assignee | ||
Comment 33•5 years ago
|
||
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.
Comment 34•5 years ago
|
||
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.
Comment 35•5 years ago
|
||
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!
Comment 36•5 years ago
|
||
IIUC, this would have been applicable to the Fx72 release. Given that Fx73 is about to ship, I think this ship already sailed.
Description
•