(In reply to Robert Relyea from comment #11)

There are 2 places in sftkpwd.c that
need to change:
sftkdb_EncryptAttribute() and sftkdb_SignAttribute.
Both call nsspkcs5_NewParam with a hard coded interation count
of 1.

Bob, I tried, and it worked for sql/sharedb.

For dbm/legacydb we run into failures. For example, certutil trying to create a key reports:

Incorrect password/PIN entered.
certutil: unable to generate key(s)
: SEC_ERROR_BAD_PASSWORD: The security password entered is incorrect.

Does the legacy db record the number of iterations?

No, I have a patch that fixes that. I thought I had attached it. I got diverted but I was just looking at this today for another reason. I'll be away for a couple of weeks, I'll send you a copy of the patch that deals with dbm.

This is Bob's patch, merged.

(The patch that Bob had sent me contained a chunk in devtoken.c but I think it's unrelated, as it was about nicknames.)

I used a different iteration count than Bob sent me.

With this patch, executing the test suite takes up a long time. I think this isn't practical for our automated environments.
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=1069fdf9f54e7075277e3f2e030a02f364b90c9d

Can we invent a mechanism that either:

• allows the application to define the iteration count that should be used, using a new API (that's called before NSS gets initialized, to ensure it will be used in automatic conversions that happen during init)

or

• use a mechanism that will keep the number small whenever we build in a test suite environment, for both debug and opt (this approach might be easier to do)

Can we set the iteration count using an environment variable for testing? I wouldn't want to bake something in when it comes to building the final code, but an environment variable seems about right NSS_DBPBE_ITERATION_COUNT=10 when initializing test configurations wouldn't be terrible. It's not like someone will set something like that by accident.

We might want to consider different counts for aarch64/arm to reflect the generally less capable nature of those platforms. Startup time there is already bad, so we might want a lower default in those builds. Randell Jesup might be able to help with access to less powerful ARM devices. For reference, this is using PBKDF1 (not 2).

Bob's patch that implements the necessary funtionality, but excluding the increase of numbers.

Separate patch that raises the numbers.

With a new build time symbol, that can allows us to use a minimal number of iterations when executing the NSS test suite, to avoid that each test takes many hours.

A patch for the NSS taskcluster test environment, which uses the unsafe iteration count to speed up testing.

By default the higher number of iterations is used.

NSS try build of all three patches combined:
https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=b81700f6659d3c63f552f36d2272c65d9982819d

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

Can we set the iteration count using an environment variable for testing? I wouldn't want to bake something in when it comes to building the final code

IIUC we agree that the production NSS code shouldn't respond to an environment variable to lower the iteration count, and we need a build time configuration.

but an environment variable seems about right NSS_DBPBE_ITERATION_COUNT=10 when initializing test configurations wouldn't be terrible. It's not like someone will set something like that by accident.

What do you think about the attached taskcluster patch? Would you do it differently?

Let's see what a try build with Talos says about the performance impact.

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

Let's see what a try build with Talos says about the performance impact.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=17fb9078907c2ff7d303e9be9e129ecfc508ee05

We got timeouts.

I don't think that it is problematic to respond to an environment variable in this way. For any database that is created with a higher count, the only risk is that the environment variable is set when a password is replaced.

I'm more concerned about is making builds with low iteration counts, testing with those and finding that we aren't testing something. I also have a concern that a debug build of NSS might be used to create a database that is poorly protected. I would rather have all builds use an appropriate iteration count, then override in the instant, as necessary.

We got timeouts.

Yes, this will be a performance regression; that's why we need folks like Randell to give us some guidance on how much is acceptable. And maybe we can find a way to limit the impact of that regression. For those tests that timed out, we start and stop Firefox a LOT, so it makes sense to reduce iteration counts just for those databases we use there.

Kai, Bob, is there any hope of moving to PBKDF2? I don't care about the triple-DES part as much, but that isn't superb either.

Randell? We're going to take a hit here, almost certainly at startup. Can you give us some guidance on what the bounds are on that?

Note: I'm not saying that better encryption of this db isn't a good thing.

So: what's the attack scenario we're protecting against here?

If it's "someone with physical access to the machine can copy my password DB and crack it offline"... so someone with that access can likely do all sorts of damage without bothering to crack the master password. Keyloggers, playing with prefs.js, etc, etc. IIRC mhoye and others, the attack scenario for Master Password was mostly "someone walks up to my (logged in, not locked) machine, starts firefox, and tries to look at my passwords".

If it's "someone with read access to my profile directory" that's already pretty bad/wrong; they shouldn't be readable by other users; or it's some sort of bad bug in Firefox that exposes files on the disk to an attacker (that really would be a chemspill/0-day level bug). But it would be nice to have better defense in depth against this.

That said... we're scrounging for fractions of a second in startup. And you need to think "how fast will this be on a reference laptop" (like the 2017 acer or worse the 2018 Dell (celeron!). Just initing the SafeBrowsing DB takes us ~6 seconds on the acer in startup - down from 8 by switching to a anti-file-corruption hash from the strong hash used for distribution, because it was taking seconds to compute the hashes of the SB DB's.

Note: the timeouts were because a two single tests took about a minute - and IIRC this didn't involve starting or stopping firefox at all, just running a test (one was for disabling the master password).

I suggest a/b profiles on a reference laptop, without/with these patches. mconley can also help with startup issues. But the test failing here doesn't appear to be startup, so we'd need to a/b disabling a master password (and setting one, and pulling up the saved logins/etc)

Marked for qf triage.

If we must take a CPU hit here, we would strongly like to defer this to after startup (and initial load probably, if we were invoked with a URL) and preferably compute it off-main-thread, and if we need the decrypted DB and it's not done yet, wait (stall the load, or defer autofilling in fields, or simply don't autofill if it's not ready yet (though that's ugly)).

I think we'll also need someone to ballpark the security vs time (perf) impact of different choices for improving the MP's security against brute-forcing - again with an eye on the lowest-common-denominator user machine. Say a 5 year old, mid/low-end Staples special laptop.....

mt? kai?

Regardless if we do this, we should probably try to ensure that prompting for the Master Password for the logins database can only occur outside of the start-up window.

We should also probably check to see if there are any plans with how Master Password is expected to work in the browser moving forward with Lockwise. It probably doesn't make much sense to invest much time in optimizing this if the Lockwise Password Manager is going to use something else.

ni?ing sfoster about any plans Lockwise might have wrt Master Password.

(In reply to Mike Conley (:mconley) (:⚙️) from comment #51)

Regardless if we do this, we should probably try to ensure that prompting for the Master Password for the logins database can only occur outside of the start-up window.

Bug 1149500 probably would have addressed this but due to bug 1538460 and bug 1539091 it was reverted if a MP was set. In practice the prompt is generally triggered by page load or Sync (which is delayed until after startup) so it shouldn't happen too early.

We should also probably check to see if there are any plans with how Master Password is expected to work in the browser moving forward with Lockwise. It probably doesn't make much sense to invest much time in optimizing this if the Lockwise Password Manager is going to use something else.

There are no short term or concrete plans to replace Master Password, we have explored using an operating system key store but haven't tested it in the wild yet. Fixing this will help password manager as it is today including encryption of client certificates and it sounds like it's wanted for other NSS consumers too.

I'm going to suggest something that might be contentious. If we have mozilla_client set (i.e., we're building for Firefox), we set the iteration count to 1, or at least very low. That sounds terrible, but there are several reasons:

1. The performance regression. Even though this might be deferred, Firefox is not really in a state where it can deal with slow database access. This might be a temporary condition, so we should be prepared to strike this reason and reconsider, but only as long as other reasons remain.

2. The threat model. Randell was right to point out that the threat model is not serious. You have to compromise someone's machine and get access to files that are usually protected by the operating system before the password protection is even relevant. However, I don't think that this is a strong argument if you considered machines with shared access and other environments. Also, we still believe in defense in depth, and following good practices with respect to password handling, so I don't find this reason to be dispositive.

3. Firefox is looking to use OS keystores. This is relevant here because the design (as I understand it, correct me if I am wrong) would be to place a high entropy secret in the OS store. That would be unlocked using whatever mechanism the OS provides (password, fingerprint, retinal scan, etc...). As a high entropy secret, we could use that as the "password" here. That would not be vulnerable to a brute force at one SHA-1 iteration. The only remotely plausible concern would be that applying SHA-1 actually weakens the key, but as far as we know, SHA-1 is only susceptible to collisions, not preimage attacks, so this is not currently a problem.

So, if we are able to - for the moment - set reason #2 aside, then we can say that 1 iteration (or a very low number) is OK for the moment. If Firefox moves to the OS key store and a high entropy key for the "master password", then we can keep the iteration count low. If Firefox improves the startup and master password prompts so that they don't block startup, then we can lift the iteration count.

(FWIW, this makes me more enthusiastic about an environment variable override. Not so much to reduce the number for testing, though that is certainly worth doing, but to INCREASE the value for those who want stronger protection.)

Matt, are we looking at fixing Bug 1149500 again?

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

2. The threat model. Randell was right to point out that the threat model is not serious. You have to compromise someone's machine and get access to files that are usually protected by the operating system before the password protection is even relevant.

There's another attack vector: Backup files that are uploaded to the cloud, which an attacker might be able to access more easily than the victim's computer.

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

The threat model. Randell was right to point out that the threat model is not serious. You have to compromise someone's machine and get access to files that are usually protected by the operating system before the password protection is even relevant.

I respectfully disagree. You're talking about a password store that contains many logins, even one of which being compromised can be disastrous, so the risk factor of storing this on disk is very large, and any encryption of it should be considerably strong and not easy to brute-force guess the key of that would unlock it. So the password protection very quickly becomes relevant in any situation where the data might be accessed.
The operating system also does not protect these files at all in the common setup (O.S. user profiles are generally not credential-encrypted, and even more so, many users still do not use more than one user account on their system) since they are stored in the user space. Any malware can have access to them. Malware infections do happen and if the browser profile is accessed then the file is up for grabs - a swift way to recover the master password without needing access to the user's computer will then grant the attacker easy access to all accounts stored in it.
Then as Kai already pointed out people do regularly backup their browser profiles since they do not want to lose their passwords/bookmarks/etc. Not just the cloud but any medium carrying the password store (USB, external drives, etc.) becomes a risk of access to the data, so it's important for that too that recovery of the master password from just the password store is infeasible. Also consider stolen laptops as a risk factor.

I agree with Martin about point 2 "However, I don't think that this is a strong argument if you considered machines with shared access and other environments. Also, we still believe in defense in depth, and following good practices with respect to password handling, so I don't find this reason to be dispositive."

The best solution would be the OS keystore, correct? And the best way we could up the security on the current password mechanism would be to find a way to defer it better, a la bug 1149500.

Mirroring mt's question about bug 1149500 - is there a bug on moving to an OS Keystore? And what would be the effort/time period to do that if it were prioritized?

One concern I have is that the tests that timed out weren't startup tests, or the first test in a suite - they were tests of the Master Password in the middle of a sequence. This implies that running that test and (for example) disabling the MP, the test took over 60 seconds. This concerns me that the performance (especially on low end) for accessing and modifying the password-protected data may be problematically slow, even if we don't take any startup hit from it. It would be useful to verify this on a reference laptop; perhaps a profile of running that test, or just profiles of normal user operations that would touch it, plus startup. (Is the en/decryption here done with a CryptoTask? If so, we should ensure the profiles capture that.)

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

Matt, are we looking at fixing Bug 1149500 again?

The root cause which meant we had to leave the MP behavior as-is is tracked in bug 1544167. With that fixed, we could remove the exception for MP and defer form autofill for all tabs (and not prompt the user for their MP until it was actually necessary)

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #56)

Mirroring mt's question about bug 1149500 - is there a bug on moving to an OS Keystore? And what would be the effort/time period to do that if it were prioritized?

Using nsIOSKeystore [0] for the password database (and preferably the cookie jar, and client certificates...) didn't have have specific bugs filed, but does now, in Bug 1562324. The meta bug is Bug 1463865.

We have a year-old estimate where the remaining work is roughly 1.5 staff-months (+ an unknown amount of UX time). The big work is UX around handling password export/import, and figuring out the corner cases one way or another (through UX & keysplits, or via opt-out prefs, or something) for profiles shared on, say, network drives (See Bug 1562325 for that). The actual switchover and migration code would be comparatively easy (modulo that we haven't implemented OSKeystore on Android, Bug 1478921), but making it not cause undue grief for all the corner cases is the hard part.

I do not want to use this bug to debate those issues, though. Please go to the appropriate sub-bug:

• Bug 1562324: Using nsIOSKeystore for passwords, client certs, cookies
• Bug 1463865: nsIOSKeystore meta bug
• Bug 1562325: Handling profile copy / share / backup / restore in a world where we have Bug 1562324
• Bug 1478921: Catching up OSKeystore implementation for Android

[0] https://searchfox.org/mozilla-central/source/security/manager/ssl/nsIOSKeyStore.idl

A lot of the previous comments have been about Firefox, while this bug is about NSS in general, and my driver for improvement are the needs of Thunderbird.

Let me summarize the current situation, to allow us structurize the various thoughts and needs on this topic.

Historically, all our secret keys were located inside the NSS database, this covered:

• the private keys for certificates owned by the user
• a symmetric key (usually just one)

Those keys can be protected using the master password, which currently uses just one KDF iteration.

Historically, Firefox stored all login passwords in a file in its profile, and used the NSS DB symmetric key to encrypt and decrypt them.
The Mozilla code module inside Gecko is named "Secret Decoder Ring" (SDR).
Thunderbird also uses that for the user's login credentials for various servers.

I looked at the security/manager/{*Secret.*,*Store.*} code, and see that Firefox has implemented a new OS key store abstractions, which has platform specific implementations.

For Windows and Mac, you directly use a service provided by the OS, you hand over the unprotected passwords, and rely on the OS to store it securely.
For Linux, you attempt to use a similar service, but if it isn't available in the environment, you fall back to using NSS.
In the fall back to NSS, you don't use the classic approach from SDR, you don't reuse the existing symmetric key and use an external file for storage (SDR), but you rather invent a new mechanism to store symmetrically encrypted passwords directly inside the NSS DB.
You don't have a specific implementation for Android yet, and Android probably doesn't have the required Gnome services, so Android probably falls back to NSS.
I don't know if there additional Linux environments that may fall back to NSS.

You said, the Firefox application already uses this new OS key store for web payments and related credit cards.
You don't use it yet for logins, you don't use it yet for user private keys from user certificates.

This means that as of today, Firefox still depends on the protection that the Master Password provides (if the user decides to set it).

While migrating away from SDR to OS key store for Firefox password storage might be an easy short term project, it seems more difficult to move the storage for asymmetric private keys for certificates. If Firefox decides to move it out of NSS, then users will no longer be able to use the standard NSS tools to manage the certificates and keys that are available to Firefox. If Firefox wants to keep the store for the user's certificate keys inside NSS, then this could be sufficient reason for Firefox to be interested in an improvement of the security of the NSS key store, too.

Or, if Firefox still requires a fallback to NSS for some environments, that might also be a sufficient reason to be interested in an improvement to the MP KDF, even for the Firefox product.

However, I understand that from the Firefox product perspective, there might be less worry about the user's secret certificate keys. Very few web sites use SSL client certificates for authentication on the public Internet, and in corporate or agency environments where it's more common, users probably have their keys on smartcards.

It's a different situation for Thunderbird, where private users often don't use smartcards for the keys they use for email encryption and email signatures.

Thunderbird users might use a portable installation on USB disk, for convenient use of other computers. For them, it's helpful if the NSS key database is strongly protected, to protect the key, if the USB disk is lost.

If a TB user uses an environment with their home directory stored in a network location, then the protection is also needed.

Or the TB user might feel uncomfortable if the OS has access to the unprotected keys, and prefer that only the application directly handles the secret keys.

Furthermore, being able to backup the profile directory can be a quick and easy way to create a backup of your email encryption/signature keys. If certificates keys are moved to OS storage, this ease of use is lost.

Because of the above, I believe we have sufficient arguments why Thunderbird should get an improvement for the MP KDF, even if there are less worries for the Firefox product, or if Firefox' plans for the future lower those worries for Firefox even more.

Consequently, I suggest that we limit this bug about the general enhancement of NSS, and work out a compromise that suits both the Firefox and Thunderbird products.

IIUC, the primary worry from the Firefox perspective is performance. Based on that, I suggest the following to be done:

• as a first step, let's increase the default iteration count just a little, to a value that is perfectly tolerable for Firefox. Maybe 2000 iterations could be such a value. If a lower value is necessary, we'll use the lower value.

• as a second step, introduce a new API, that an application can use to query and set the iteration count inside the NSS database. Firefox wouldn't use it. Thunderbird would use it to request a higher value that is deemed secure.

Then I have another suggestion for the NSS implementation level.

For the modern NSS storage (key4 based on sqlite), the database already supports storage of the iteration count.

For the legacy NSS storage (key3 based on dbm), several changes are required to make it work, and that's the majority of Bob's attached patch. Bob told me that it's an incompatible change, that old versions of NSS/Firefox/Thunderbird couldn't read files produced by the newer versions. This seems too much hassle for code that nobody should be using any more anyway. Because we have already migrated the apps to the new storage, I suggest that we keep things simple and that we don't try to increase the security of the legacy storage, and hardcode the legacy DB code to always use just one iteration.

I filed individual bugs for the suggested separate steps. Let's use this bug for remaining overall discussion, if necessary. Let's use the individual bugs to discuss the details of those suggestions, and if they make sense, and to implement them.

Kai,

While you are correct that the scope should not be limited to Firefox for several reasons, it likewise should not be limited to "Firefox and Thunderbird" or "Mozilla products" only -- NSS is widely used outside of that scope. Other products (like e.g. my own) use DBM/LegacyDB and equally require proper MP protection, for which 1 round simply doesn't do. The one-way migration would be just fine, considering it requires user action to re-key the db to using the new schema (setting a new MP). Simply declining to do anything to improve legacyDB use is a mistake, IMHO. Ultimately in that situation we'd have to keep patching NSS to incorporate Bob's work to improve legacyDB (Which works like a charm by the way, thanks!) and disallow building with system NSS as a result if legacyDB improvements in NSS aren't made, which is not in anyone's best interest.

Please reconsider your stance.

(In reply to Mark Straver from comment #62)

Other products (like e.g. my own) use DBM/LegacyDB and equally require proper MP protection

I think it's fair to discuss it separately, because of the potential migration consequences.
I've filed bug 1562683, let's discuss over there.

Given that Bob has added support for Legacy DBM, he might have a need for it. Let's wait for his thoughts when he's back.

Randell, can you clarify what qf decision you want to have made here? Are you looking for a priority for having somebody figure out whether the hit impacts startup, or how much of a hit is acceptable? Removing the [qf] keyword for now.

Markus: probably both... (and maybe to a second level, determine how much jank and under what circumstances this would introduce). This isn't solely performance's decision - if it is decided this is critical for security, regardless of the impact, then we'll have to figure out what we can do to minimize/alleviate it. (btw, I'm on PTO ;-)

OK, I've been on vacation, so I didn't have a chance to short-circuit a lot of the performance issues.
I had performance in mind when I did the initial patch. It does a couple of very deliberate things that mitigate the performance costs:

1. null passwords are encrypted with a small iteration count.
2. when we check for a null password, we fail if it doesn't have a small iteration count.
   The pbe generated by the password is cached as long as you are logged in, so you only pay the price on login.

This means in Firefox, the only time you should see the slower performance is when you have the password prompt. There will be a longer delay between the time you check the password and Firefox 'does something else'. For startup, even those of use with a password on our data base, the first page loads without a password prompt.

This does effect the NSS automated tests because they run lots of small tests that all need a password to continue. This means a huge cost when running the tests. (so it makes sense that we would use a small count just for the tests, with maybe a could tests without the small count to make sure the basic code is working correctly)

Kai, Bob, is there any hope of moving to PBKDF2? I don't care about the triple-DES part as much, but that isn't superb either.

I need to look at that anyway for FIPS. The basic code is there, but we do need to make some tweaks to make it work. I think I had the same issue where I could make sql forward compatible, but not dbm (though that's a smaller patch). I think we should handle that as a separate bug to this one, though.

bob

A new patch is in bug 1562671, which should work as a first step. As explained by Bob, it shouldn't affect Firefox performance, because it keeps the iteration count for the default/empty password at 1.

I suggest that after bug 1562671 landed, we keep this bug 524403 open as a tracker bug for potential additional improvements, such as changing to a different PBE algorithm for the MP.

I just noticed that we already have separate bug 973759 to track the suggestion of using strong crypto mechanisms for the master password.

I conclude we can consider this issue fixed for the current NSS default storage.

Higher iterations have been enabled also for key3.db as an opt in, and potentially we'll make it a default at a later time (as explained in bug 1562683).