Closed Bug 1562671 Opened 11 months ago Closed 7 months ago

Increase NSS MP KDF default iteration count, by default for modern key4 storage, optionally for legacy key3.db storage

Categories

(NSS :: Libraries, enhancement, P2)

enhancement

Tracking

(relnote-firefox -)

RESOLVED FIXED
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)

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.

Type: task → enhancement
Attached patch 1562671-v1.patch (obsolete) — Splinter Review

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?

Attachment #9075176 - Flags: review?(rrelyea)

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?

Summary: Slightly increase NSS MP KDF iteration count for modern key4 storage → Slightly increase NSS MP KDF default iteration count for modern key4 storage
See Also: → 1562683

(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 on attachment 9075176 [details] [diff] [review]
1562671-v1.patch

Review of attachment 9075176 [details] [diff] [review]:
-----------------------------------------------------------------

Need some comments answered before r+
Mostly about why we don't set the iteration count for legacy db's to 1 at a higher level.
Also I'd like to see a run time setting for the iteration count rather than a compile time. I think we already have an environment variable we use for the pkcs12 code, though I could be remembering incorrectly.

::: lib/softoken/sftkdb.c
@@ +529,5 @@
>                  crv = CKR_USER_NOT_LOGGED_IN;
>                  goto loser;
>              }
>              rv = sftkdb_SignAttribute(arena, &keyHandle->passwordKey,
> +                                      keyHandle->usesLegacyStorage ? 1 : keyHandle->defaultIterationCount,

See comment is sftkpwd.c

@@ +663,5 @@
>                      *crv = CKR_USER_NOT_LOGGED_IN;
>                      break;
>                  }
>                  rv = sftkdb_EncryptAttribute(arena, &handle->passwordKey,
> +                                             handle->usesLegacyStorage ? 1 : handle->defaultIterationCount,

See comment is sftkpwd.c

@@ +2462,5 @@
>      handle->oldKey = NULL;
>      handle->updatePasswordKey = NULL;
>      handle->updateID = NULL;
>      handle->type = type;
> +    handle->usesLegacyStorage = legacy;

I'm good with the code identifying that we have a legacy db.

::: lib/softoken/sftkpwd.c
@@ +40,5 @@
> +#else
> +    2000
> +#endif
> +    ;
> +

I think I' d prefer a runtime switch for this rather than a compile time. We run the NSS tests in as part of our release builds.

I don't remember if this was part of my initial patch or if kai added it.

@@ +518,5 @@
>      data = keydb->passwordKey.data;
>      len = keydb->passwordKey.len;
>      keydb->passwordKey.data = passKey->data;
>      keydb->passwordKey.len = passKey->len;
> +    keydb->defaultIterationCount = keydb->usesLegacyStorage ? 1 : iterationCount;

This is a kai addition that I'm OK with, at least for now. I'd still like to code for the legacyDB to handle the non-1 iteration count included, so that if we change this in the future, we have a little runway of NSS versions that could still handle that database.

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

@@ +1039,5 @@
>          SECItem plainText;
>          plainText.data = authAttr.pValue;
>          plainText.len = authAttr.ulValueLen;
> +        if (sftkdb_SignAttribute(arena, newKey,
> +                                 handle->usesLegacyStorage ? 1 : iterationCount,

Same question as above about changing the iteration count at the lower level.

@@ +1098,5 @@
>          SECItem *result;
>          plainText.data = privAttr.pValue;
>          plainText.len = privAttr.ulValueLen;
> +        if (sftkdb_EncryptAttribute(arena, newKey,
> +                                    keydb->usesLegacyStorage ? 1 : iterationCount,

Same question as above about changing the iteration count at the lower level.

@@ +1246,5 @@
>      }
>  
> +    if (newPin && *newPin == 0) {
> +        iterationCount = 1;
> +    }

here's where I would have expected the DB based iteration count. Also this is where we could to the runtime change for the iteration count.
Attachment #9075176 - Flags: review?(rrelyea)

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

bob

Flags: needinfo?(kaie)

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.

Flags: needinfo?(kaie)
Attachment #9075176 - Attachment is obsolete: true
Summary: Slightly increase NSS MP KDF default iteration count for modern key4 storage → Increase NSS MP KDF default iteration count, by default for modern key4 storage, optionally for legacy key3.db storage
See Also: 1562683
Blocks: 1562683

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.)

Attachment #9105213 - Flags: review?(rrelyea)

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.)

Attached patch 1562671-taskcluster-v2.patch (obsolete) — Splinter Review

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.

Comment on attachment 9105213 [details] [diff] [review]
1562671-v2.patch (should have excluded the change to taskcluster)

Review of attachment 9105213 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Kai.
Attachment #9105213 - Flags: review?(rrelyea) → review+
Comment on attachment 9105213 [details] [diff] [review]
1562671-v2.patch (should have excluded the change to taskcluster)

Thanks Bob!

The patch given to you for review accidentally included the changes for taskcluster/.../extend.js, so let's still consider those unreviewed, they are attached separately.
Attachment #9105213 - Attachment description: 1562671-v2.patch → 1562671-v2.patch (should have excluded the change to taskcluster)
Comment on attachment 9105218 [details] [diff] [review]
1562671-taskcluster-v2.patch

JC, could you review this taskcluster change, or should I ask someone else?

I didn't try hard to minimize the amount of places that set the new environment variable - any idea for simplification?
(I haven't checked if the merge calls will do nested merging, i.e. if two lists contains an entry for "env", will the merge call merge the contents of both "env" lists?)
Attachment #9105218 - Flags: review?(jjones)

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

Comment on attachment 9105218 [details] [diff] [review]
1562671-taskcluster-v2.patch

Thanks Martin! I'll prepare and test a replacement patch for taskcluster tomorrow.
Attachment #9105218 - Attachment is obsolete: true
Attachment #9105218 - Flags: review?(jjones)
Attachment #9105217 - Attachment description: 1562671-v2-incremental.patch → 1562671-v2-incremental.patch (to help Bob's review)
Attachment #9105217 - Attachment is obsolete: true
Attachment #9105524 - Flags: review?(mt)

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

Attachment #9105213 - Attachment is obsolete: true
Attachment #9105525 - Flags: review+
Attachment #9105524 - Flags: review?(jjones)
Attachment #9105525 - Attachment is patch: true
Comment on attachment 9105524 [details] [diff] [review]
1562671-taskcluster-v3.patch

Review of attachment 9105524 [details] [diff] [review]:
-----------------------------------------------------------------

Splinter still?
Attachment #9105524 - Flags: review?(mt) → review+
Attachment #9105524 - Flags: review?(jjones)

(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.

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

Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Whiteboard: [relnotes: comment 26]
Target Milestone: --- → 3.48
Duplicate of this bug: 1589740
No longer blocks: 524403
Duplicate of this bug: 524403
Duplicate of this bug: 1562672
Blocks: 1562674
Blocks: 1562687

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

relnote-firefox: --- → ?

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!

Blocks: 1609189

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

You need to log in before you can comment on or make changes to this bug.