1561368 - NSS should delete key3.db after a successful migration to key4.db

Status: RESOLVED WONTFIX

(NSS :: Libraries, defect, P3)

Description

[Kai Engert (:KaiE:)]

The Mozilla applications migrated from key3/dbm to key4/sql.

Because of bug CVE-2018-12383 Firefox implemented code that deletes key3.db after a migration has "apparently" happened. Firefox uses the presence of a key4.db file to conclude that migration has happened (for full details, see https://searchfox.org/mozilla-central/search?q=MaybeCleanUpOldNSSFiles&path= ). However, despite the presence of key4.db the migration might still be incomplete.

This behavior has caused problems for Thunderbird, see bug 1510212, which is why we have backed out that Firefox code for TB 60.x, and keep the key3.db file. We'll probably do the same for TB 68.x, bug 1561366.

The correct solution is to change NSS. Only after NSS knows that it has successfully completed the migration, it should clean up the key3.db file.

Comment 1

[Kai Engert (:KaiE:)]

Bob, is there any existing API that could be used to find the answer for the following question:

"Is the currently open NSS database in a partially migrated state, that will require information from key3.db at a later time, e.g. when the master password gets eventually provided?"

We haven't yet fixed bug 1475775 for Thunderbird, because the approach that Firefox used was considered problematic for Thunderbird. (Firefox simply deletes key3.db, although it might still be required for migration. As a result of deleting early, users might no longer be able to access their stored passwords or private keys.)

I think that ideally, NSS should have had code to delete key3.db as soon as a migration was fully completed. Maybe it still makes sense to add that code to the automatic migration, but it won't be sufficient for already migrated database, as the migration code is not ran again, right?

I see two potential approaches to fix this bug:

Approach 1 - a future NSS version should fully clean up all leftover key3.db

In order to cover both scenarios "migration fully completed just now" and "key3.db was left over on disk during a previous migration", I think the cleanup should be done as a last step inside the NSS database init code. If currently open NSS DB is key4, and key4 is fully initialized and migrated, then check for a leftover key3, and if found, delete key3.

Approach 2 - allow an application to cleanup

For this to work without reliably, the application needs to be able to query two different properties: "are we running using key4?" and "is there a key migration pending?".

If there isn't a specific API for that, maybe we could use the following workaround to obtain that information:

• because the application requests "sql:" assume we're indeed using key4
• add the code at the time the application does PK11_Authenticate to the internal token, and if it succeeds, conclude that NSS had the password available, and must have been able to fully migrate already.

Bob, would you recommend us to use approach 1 (cleanup inside NSS) or approach 2 (cleanup in application code) ?

Comment 2

[Robert Relyea]

So in general, you should not expect NSS to delete old databases. They are maintained so that old applications continue to work. It's not even clear the Thunderbird should delete it in this case. If you saved passwords without a master password, then you later set the master password, only backed up database would still compromise any of your new passwords as well. Maybe, in that case, we should reencrypt all the passwords with a new SDR key.

That being said, it seems reasonable for application to be able to query 1) what type of database is in use, and 2) what is it's update state. Currently there isn't an explicit interface to do either. It seems reasonable to add an interface that returns information about the internal state of the database code.

It looks like you can implicitly infer that the database needs updating if it's open r/w and you try to modify a certificate object attribute in the database. If you get 'User not logged in' error, then it means the database needs a password to update (I haven't verified that you would get this error normally if you try to just change a certificate attribute). Determining what type of database you are opening is trickier. There's a private interface in nssutil that will tell you, given a config string, what database you will open (it's the call softoken used to determine that).

All that being said, your idea of checking for "sql:" in the database name will work if thunderbird always explicitly sets sql:, it won't handle the case where no explicit "sql:" is given and the sqlite database is either default, or the environment variable is set to make it so. Also your idea of assuming that the database is updated after a successful PK11_Authenticate() will work as long as 1) you are doing a normal update (not a merge update... and you the application would know if you are doing a merge update), and 2) you have opened the database r/w (if you only open it r/o, no update will happen).

Summary: 1) I think you may want to fix the issue in a way other than removing the old database. 2) Even if you accept 1, an interface that gives the application better database information is reasonable, but it doesn't exist now, and 3) if you still want to remove the old database, your proposed solution should work with caveats.

Comment 3

[Kai Engert (:KaiE:)]

(In reply to Robert Relyea from comment #2)

So in general, you should not expect NSS to delete old databases. They are maintained so that old applications continue to work.

I conclude that you'd like this bug marked as wontfix, and that we rather implement a solution at the thunderbird application level. I can accept that.

It's not even clear the Thunderbird should delete it in this case. If you saved passwords without a master password, then you later set the master password, only backed up database would still compromise any of your new passwords as well. Maybe, in that case, we should reencrypt all the passwords with a new SDR key.

The master password also protects secret keys in the key database. If the user still uses a private key with key4.db, which was already available at the time the key3.db file was used, then that private key is left without protection in the key3.db file, too. Reencrypting the externally stored passwords wouldn't solve that detail, so it can be seen as an incomplete approach.

That being said, it seems reasonable for application to be able to query 1) what type of database is in use, and 2) what is it's update state. Currently there isn't an explicit interface to do either.

Thanks for confirming.

It seems reasonable to add an interface that returns information about the internal state of the database code.

Although that might be useful in general, maybe it's too late for this specific problem at hand.

I'm looking for a solution that could be backported to the existing ESR 68 branch, without requiring a new NSS with new APIs.

A new NSS API could only be used by a future major version of Thunderbird. However, given that DBM has already been deprecated by the Mozilla code in the most recent development version, we'll no longer have the ability to migrate in future versions anyway. So, a fix for the current stable TB 68 branch is our last opportunity to get this cleaned up safely.

In other words, migration from DBM to SQL can only be done using the Thunderbird 68.x branch. Any future user of the Thunderbird 78.x release (summer 2020), who migrates from Thunderbird 52.x or earlier, will no longer be able to get their DBM key storage migrated. (For users who will have a need to do so, to go from 52 to 78, and wonder why their files no longer work, will have to get our recommendation to temporarily start 68.x for the migration.)

So, I think we'll need an alternative solution, based on what you're describing below:

your idea of checking for "sql:" in the database name will work if thunderbird always explicitly sets sql:, it won't handle the case where no explicit "sql:" is given and the sqlite database is either default, or the environment variable is set to make it so.

PSM explicitly sets the "sql:" prefix on NSS init.
https://hg.mozilla.org/mozilla-central/file/af4f2eb3220595694f9c59015114e375a43fca6b/security/certverifier/NSSCertDBTrustDomain.cpp#l1442

Determining what type of database you are opening is trickier. There's a private interface in nssutil that will tell you, given a config string, what database you will open (it's the call softoken used to determine that).

Given that we're hardcoding "sql:" I realize we don't need to check this detail.

It looks like you can implicitly infer that the database needs updating if it's open r/w and you try to modify a certificate object attribute in the database. If you get 'User not logged in' error, then it means the database needs a password to update (I haven't verified that you would get this error normally if you try to just change a certificate attribute).
...
Also your idea of assuming that the database is updated after a successful PK11_Authenticate() will work as long as 1) you are doing a normal update (not a merge update... and you the application would know if you are doing a merge update), and 2) you have opened the database r/w (if you only open it r/o, no update will happen).

PSM never drives a merge, it never calls NSS_InitWithMerge.

Thanks for pointing out that must be certain that our attempt to open r/w has succeeded.
I assume we can call PK11_IsReadOnly to confirm it.

Instead of doing PK11_Authenticate() and checking for a potential error, I wonder if we could check for the opposite. If we called PK11_IsLoggedIn() on the internal token, and we get a success, can we conclude that the necessary password for upgrade has been supplied and that the upgrade is fully completed?

If this is a correct assumption, it allows a simpler implementation outside of NSS.

Comment 4

[Kai Engert (:KaiE:)]

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

Thanks for pointing out that must be certain that our attempt to open r/w has succeeded.
I assume we can call PK11_IsReadOnly to confirm it.

Instead of doing PK11_Authenticate() and checking for a potential error, I wonder if we could check for the opposite. If we called PK11_IsLoggedIn() on the internal token, and we get a success, can we conclude that the necessary password for upgrade has been supplied and that the upgrade is fully completed?

If this is a correct assumption, it allows a simpler implementation outside of NSS.

Bug 1606619 has a patch that implements this strategy at the Thunderbird code level.