Closed Bug 524403 Opened 11 years ago Closed 8 months ago

Summary: softtoken's master password KDF process should be stronger (currently easily brute forced due to low iteration count)

Categories

(NSS :: Libraries, defect, P5)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1562671
Future

People

(Reporter: Dolske, Assigned: KaiE, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: sec-want, Whiteboard: [sg:want P2])

Attachments

(4 obsolete files)

I recently ran across "Firemaster", a master password cracking tool (http://www.securityxploded.com/firemaster.php). The keys-per-second claimed seemed higher than I was expecting, so I went looking at exactly how the master password was handled by softtoken.

Skimming over a few details, sftkdb_passwordToKey() first computes an intermediate key with just SHA1(salt,passphrase). This is then used in sftkdb_DecryptAttribute() as input to a PKCS#5 PBKDF1 routine. The default iteration count is just 1, though [set by sftkdb_EncryptAttribute() passing 1 to nsspkcs5_NewParam()].

A higher iteration count would make this more resistant to brute forcing (by increasing the cost of testing password), the PKCS#5 spec suggests a "modest value" of 1000 iterations. And that was 10 years ago. :)

As an implementation detail, it might be better to change sftkdb_passwordToKey() to actually use PKCS#5 with a high iteration count (instead of just a single simple hash), and leave sftkdb_EncryptAttribute() as-is. The intermediate key is only computed once, so attribute crypting would remain fast. [And, of course, there's still the whole issue of making this compatible with existing DBs.]
Bob, could you take a look at this bug?
Duplicate of this bug: 581528
Whiteboard: [sg:want P2]
Need an owner for this.

/be
Flags: needinfo?(rrelyea)
(In reply to Brendan Eich [:brendan] from comment #3)
> Need an owner for this.

What is driving this?
Some inbound media mention. needinfo?rrelyea is unanswered, looks like a bug that could use some guidance *in the bug* for anyone interested -- especially potential volunteers.

/be
I did some work on this previously. We also had some discussions about what to do and how to prioritize this on a few different occasions. Most of those were face-to-face meetings but I took some notes that I need to dig up. A short summary of the previous conclusions, that I remember off the top of my head are quite specific to Firefox and other Gecko-based products. Since this is an NSS bug, and NSS is a shared component amongst many products, including Google Chrome and many Oracle products in particular, I cloned this bug into a Gecko-specific bug so we can discuss the Firefox-specific (Gecko-specific) aspects without dragging the other NSS organizations into it. I trimmed the CC list of that bug; please add yourself back to the CC list if you are interested in the Firefox/Gecko-specific issues. Please don't discuss Firefox-/Gecko-specific issues in this bug. The bug to talk about Firefox's Master Password crypto strength is the new bug, which is bug 973759.
(Tweaking summary since I always have trouble finding this)
Summary: softtoken's master password KDF process should be stronger → softtoken's master password KDF process should be stronger (currently easily brute forced due to low iteration count)
9 years later I looked the same topic without being aware of this discussion and was shocked to see a single SHA1 iteration being used to hash passwords. This has been highly discouraged for a while already, see http://cynosureprime.blogspot.de/2017/08/320-million-hashes-exposed.html for example. A single GTX 1080 GPU can calculate 8.5 billion SHA1 hashes per second. With humans choosing passwords that on average have merely 40 bits of entropy (see http://research.microsoft.com/pubs/74164/www2007.pdf) the average password can be guessed in a minute. I'm sure that Mozilla can do better. Any plans to address this? At the very least, PBKDF2 should be used with an appropriate number of iterations (at least 100k, more is better). Algorithms like bcrypt or scrypt are more resistant to bruteforcing but these aren't implemented by NSS yet.
(In reply to Wladimir Palant (for Adblock Plus info Cc bugzilla@adblockplus.org) from comment #8)
> I'm sure that Mozilla can do better. Any plans to address this?

Maybe https://mozilla-lockbox.github.io/ will at some point replace the current password manager?
I don't think that waiting for some potential password manager rewrite is the solution here. Also, keep in mind that Lockbox also expects a key as input (they call it "app prekey"). With the current design, this key comes from Firefox Accounts. But relying on Firefox Accounts isn't an option if Lockbox is to replace a fundamental feature such as the password manager. That means that it will again be up to NSS to generate this key.
Hmm, I thought we had fixed this a while back, but I bet it was for PKCS #12, not for the database. We definately should change this, the current iteration count for the database is 1. 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. 

The only affect of changing the interaction count is the performance of login and private key operations. Old versions of NSS can still read the database (the iteration count for decrypt is read from the database record). Old databases will still be able to be read, though they will still have an iteration count of 1. Changing the password (even to the same password) on those database will update the iteration count.

The only other concession is what we need to change that value to. pkcs12 is set to 10000 (10K) in debug builds and 1000000 (1M) in optimized builds.
Flags: needinfo?(rrelyea)
Some kind of automated batch upgrade mechanism would also be very desirable. I have over 300 passwords that would need to have their iteration counts bumped.
We are talking about the NSS database password (Firefox Master Password), not your saved, encrypted passwords. Those passwords are encrypted with a fixed key, stored in the NSS database (encrypted with the Master Password). Changing your Firefox Master Password will change the iteration count on all the records in the NSS database (including the the record on the fixed key used to encrypt all your passwords), so you only need to change your Firefox Master Password (once) and all your keys will get the new iteration count.

If you have lots of databases, then you could use certutil to build a script (but you'd have to supply the password to each database to make this work).
One other consideration. NSS treats the empty password "" as "doesn't require a password", but that is at the higher level (check to see if I can decrypt the password record with "" and if I can just pass "" as the password and don't prompt the user). This means if we just change the iteration count, even users with no passwords will see the slower response when accessing certain operations. It would require some surgery if we think we need to special case the NULL password because by the time it hits the PBE, its already hash and salted into a random password, so it's no longer distinguished as "".

I'm I just noticed this was marked fix and duplicate, so someone else also thought it was fixed when we fixed the PKCS #12 issues, but I'm not seeing the change for the databases so I'm reopening this.
OK, well I wasn't able to reopen this. Hmm...
(In reply to Robert Relyea from comment #13)
> We are talking about the NSS database password (Firefox Master Password),
> not your saved, encrypted passwords. Those passwords are encrypted with a
> fixed key, stored in the NSS database (encrypted with the Master Password).
> Changing your Firefox Master Password will change the iteration count on all
> the records in the NSS database (including the the record on the fixed key
> used to encrypt all your passwords), so you only need to change your Firefox
> Master Password (once) and all your keys will get the new iteration count.

Ahh. I misunderstood how the pieces fit together. Sorry about that.
(In reply to Robert Relyea from comment #15)
> OK, well I wasn't able to reopen this. Hmm...

The bug is still open.
FYI. There's a new blog post which discusses this bug. It's currently the second most popular link in /r/programming.

https://www.bleepingcomputer.com/news/security/firefox-master-password-system-has-been-poorly-secured-for-the-past-9-years/
ouch seeing this really hurts. why didnt this get any real action?

it wouldnt have hurt to bump the iteration count a bit every few years to count in the advance of computer power, but well...

I personally would go even further and run a piece of argon2id as the KDF, this thing after all doesnt just eat computation cycles but also RAM for breakfast and even relatively low values as 16MB are helping because trying many attacks in parallel like with gpus or so can end pretty quickly because the RAM is getting an issue.

one thing that would be even more awesome would be obviously about:config settings for how crazy it should go so that users who know their stuff can even increase the RAM usage or iteration count and therefore the security of the thing.
Two things:

First, any way to address this needs to have a way to completely rekey the password database, because if the passphrase has already been hacked the database key is already lost -- thus any changing of the passphrase would be closing the barn door after all the cows are gone, especially for future entries.

Second, :openjck's link suggests that Mozilla's official response is to push people toward using something like the Lockbox extension (currently in fairly early alpha).  Please ensure that any replacement doesn't use the OS clipboard.  I use Mozilla's password manager specifically because it doesn't, in case undetected malware is monitoring the clipboard.  (Since there's no way to apply an ACL to the clipboard for particular applications to be able to read/paste, anything using it is less secure than the in-app channels that already exist between the password manager and the login forms.)
I don't think that Lockbox could be a solution - mainly because it is Firefox only. 

However the weak master password problem affects Firefox and Thunderbird!
@comment #22

I think rekeying is needed AS WELL AS changing the passwords in the database. because if the key is already known, re-encypting it with a new scheme wont help.

regarding argon2, full ack, as I already noted in comment #20.
I think we should not lose sight of what's going on here, and what's relevant.

If the key is already compromised it means an attacker already has your data, period, so that's not a situation you should consider here because in that case you should change all your passwords anyway.
In case an attacker does NOT have your data (and you make sure they don't get a hold of a backup of your profile) simply letting the master key be re-keyed with the new value would certainly be sufficient, since the actual encryption used otherwise is sound (although a bit dated, des-ede3 is certainly good enough for this purpose) and the only weak link here is being able to brute-force guess the master password.

A fast and simple solution would definitely be to not use 1 round. However, it needs a few adjustments because nsspkcs5_NewParam takes a raw int (=short in most implementations) for iterations, which won't accept a suggested high value; it'll have to be at least a long/int32_t.
@comment #24

I personally think security should always assume the worsz. Murphy's Law FTW.

of course if an attacher has the key it also has your PWs meaning you would need to change them, true, which is why it would be best in my opinion to obviously both rekey the DB and change the PWs.

and even if it hasnt been stolen, a rekey still isnt gonna do something bad nor is it particularly hard.
> a rekey still isnt gonna do something bad nor is it particularly hard.

It isn't, but it's also not necessary as stated in comment 23.

As for using a different encryption/keying scheme altogether: anything like that, although a nice new and shiny and super-secure way of doing things, will require (probably complex) one-way migration code which is going to be a total PITA with a lot of people potentially losing their stored passwords if anything goes wrong. It might be good for a follow-up bug, but making shorter master passwords significantly more difficult to guess should be an immediate goal, so K.I.S.S. this for now?

The encryption itself isn't weak, IIUC the only risk is, as the bug title says, a possibility to correctly brute-force guess the master password in a feasible amount of computing time. If that brute forcing is mitigated by making every guess take a million times as long, your average 40-bit password will still take probably a few years to brute-force guess.
Keeping the method and changing the rounds, as stated, would be pretty much fully transparent

I've done some quick tests by changing the rounds indicated in comment 11 (to something within int range), but it breaks the use of the master password as in making it error with "unable to change" and having an unrecoverably locked store afterwards, so it must have been missing something, elsewhere. Unfortunately I'm not at all familiar with this piece of code so probably better if the NSS folks look at this directly.
@comment #26

certainly it does need migration but is it that hard? I dont precisely know the format of the encrypted database, but if it has something to give away whether it's the old scheme or the new one it's gonna get even easier.

at least in theory the flow should be as follows:

find out whether the old scheme is used (either by checking the data or by trying)

if it's in the old format:

-> decrypt everything and put it in RAM
-> create a new key
-> encrypt the database with the key
-> run the password through whatever new KDF we want
-> encrypt the key using the KDF'd password
-> store both the encrypted database and the encrypted key.

ELSE:
-> just use the database

also I implemented migration for password hashes in websites before, so it's not like I am completely oblivious to this whole thing, and there such a thing should always exist for the sole reason to easily change the parameters or the hash functions used.

this kind of migration (even if you dont need to rekey, you do need to re-encrypt the database key with the newly hashed password) would be needed in all cases no matter whether you just bump the number of iterations or whether you straight up go to make it use an entirely new hash function.

regarding the necissity of a rekey, the problem is that a database stolen once would have it's key revealed to to the easy bruteforce, and the problem is that if it continues to use the same key later future passwords would also be compromised, which wouldnt be nice.
@comment #23
@comment #26

I don't fully agree. Security considerations and practical solutions seem to being talked down in this bug forum in favour of implementation of new features, application re-designs and individual software architecture preferences of sorts, at times.

Where the mitigating solution you suggest would certainly qualify as a practial quick-fix for now, please keep the following in mind:

Given the horrendously long times, (e. g. like with Bug 278689) until certain security-related issues are being properly adressed, it seems to be a good idea to take at least one step ahead, if finally taking care of an issue. 

Fixing it with general progressions in the field of security counter-measures to come, will probably help preventing the next several years of non-maintenance in this area becoming a more serious problem, than it needs to.

Specifically, NSS seems not getting the kind of attention it would require, given its delicate nature, compared to, say, the UI or gecko itself. If a quick-fix will be applied to this problem, I'm afraid, a real (and necessary) solution will be put into backlog for an indefinite time.
Sorry but I disagree. I've seen enough bugs never get any progress because people want to get a "perfect solution" and nothing else in.
Considering the sensitivity of this and the risk involved for the security of passwords of browser users, I think that putting in a quick, low-risk fix is exactly what this needs right now, and that changing the security altogether (either by switching to a different key system or an external service or whatever else has been discussed) should become a follow-up bug after the major risk has been mitigated in this one.

Let's move this forward instead of running aground in exchanges of opinion, shall we?
Meant to refer to @comment #24, not #23 above, sorry.

I see your point, and it was exactly, why things got out of range in bug 278689 mentioned above.
Basically, I agree with not waiting for the perfect solution for major or critical bugs and blockers.
It just should be really ensured, that this issue will be prioritized and adressed with a real solution according to its nature in a timely manner. NSS in general seems to be dire need for some major overhauling. Mark Mayo should definitely provide more resources for these kind of problems and re-designs soon.

With "taking a step ahead" above, I mean applying a better hashing algorithm, a more modern and secure combination of encryption/cipher mode, like sha-256, maybe even stronger, which offers a decent bf protection layer against physical access attacks, because "des-ede3" does obviously not - see also Bug 973759 and:

https://security.stackexchange.com/questions/92593/why-is-there-a-des-ede3-cbc-in-my-rsa-private-key
https://www.openssl.org/blog/blog/2016/08/24/sweet32/

Since the Mozilla password safe holds numerous other, potentially very sensitive passwords, e. g. to banking accounts, there should definitely be no false compromises in terms of security anymore. The performance costs for higher encryption standards (if any) would be well-justified, and should not be a real issue anyway, given the low number of manual accesses to the pw safe.

When combined with securely tunneled access per smartcard and U2F, along with a re-designed browser architecture, a modern means of pw safe encrption should provide much better security for bf and other kinds of attacks, helping to maintain privacy, to some extent for malware attacks, and even if the mail data base was fetched.
A few weeks of silence have passed. Robert, what do you think? I clearly like the simple short-term solution to secure the pw store without a complicated migration path for a different crypto approach, with further improvements down the line.
Flags: needinfo?(rrelyea)
Restrict Comments: true
Priority: -- → P5
QA Contact: franziskuskiefer

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

See Also: → 1278071

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.

Flags: needinfo?(rrelyea)
Attached patch 524403-bob-v2.patch (obsolete) — Splinter Review

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)
Assignee: nobody → kaie

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

Attached patch 524403-bob-v3.patch (obsolete) — Splinter Review

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

Attachment #9074156 - Attachment is obsolete: true
Attached patch 524403-numbers-v3.patch (obsolete) — Splinter Review

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.

Attached patch 524403-taskcluster-v3.patch (obsolete) — Splinter Review

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.

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

Flags: needinfo?(rjesup)

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

Flags: needinfo?(rjesup) → needinfo?(mconley)
Whiteboard: [sg:want P2] → [sg:want P2][qf]

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.

Flags: needinfo?(mconley) → needinfo?(sfoster)

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

Status: REOPENED → ASSIGNED

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)

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

Flags: needinfo?(sfoster)

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

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

Depends on: 1562671
Depends on: 1562672
See Also: → 1562674

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.

Comment on attachment 9074180 [details] [diff] [review]
524403-bob-v3.patch

Moving patch to bug 1562671.
Attachment #9074180 - Attachment is obsolete: true
Attachment #9074181 - Attachment is obsolete: true
Attachment #9074182 - Attachment is obsolete: true

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.

See Also: → 1562683

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

See Also: → 1562687

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.

Flags: needinfo?(rjesup)
Whiteboard: [sg:want P2][qf] → [sg:want P2]

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

Flags: needinfo?(rjesup) → needinfo?(mstange)

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

Blocks: 1589740

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.

Summary: softtoken's master password KDF process should be stronger (currently easily brute forced due to low iteration count) → softtoken's master password KDF process should be stronger
No longer blocks: 1589740

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

Status: ASSIGNED → RESOLVED
Closed: 1 year ago8 months ago
No longer depends on: 1562671
Resolution: --- → DUPLICATE
Summary: softtoken's master password KDF process should be stronger → Summary: softtoken's master password KDF process should be stronger (currently easily brute forced due to low iteration count)
Duplicate of bug: 1562671
No longer depends on: 1562672
You need to log in before you can comment on or make changes to this bug.