1779022 - Improve Thunderbird profile import, to safely import logins (account and password) and potentially other keys

Status: NEW

(MailNews Core :: Import, defect, P2)

Description

[Rob Lemley [:rjl]]

OpenPGP keys are not imported when using the Thunderbird Profile importer.

STR:

• Set up a profile on Thunderbird 91 and create an account with an OpenPGP key
• Run Thunderbird 102.0.2 and use the Import tool to import the 91 profile
• Open the OpenPGP Key Manager to verify the key was imported

Expected:
The Key Manager should display the key created in the 91 profile

Actual:
No keys are displayed in the Key Manager
In Account Settings, a message is shown "This key could not be found"

Comment 1

[Ping Chen [:rnons] (away)]

Attachment: Bug 1779022 - Show a separate checkbox to import passwords and OpenPGP keys. r=kaie,mkmelin

Similar to importing passwords, this only works when there is no existing key in the current profile.

Comment 2

[Kai Engert [:KaiE:]]

The importer says "Any existing or identical data will not be overwritten."
It's good that it will never delete what you already have in the destination profile.
But what will happen if both source and destination already have data of the same type? Will it be merged?
I think the text shown to the user should explain that better.

In your initial patch, you attempt to simply copy files, including key4.db and logins.json etc.
This approach cannot work.

Every profile immediately has file key4.db. You can never copy it over, because copying would replace the file the destination already has.

File key4.db in each profile may contain many things, however, at the very least, it contains a unique, randomly generated, symmetric encryption key.

Files login.json and encrypted-openpgp-passphrase.txt directly depend on that key. They are unusable without it.

Let's say source contains key4.db, login.json and encrypted-openpgp-passphrase.txt and secring.gpg

If you copy logins.json to destination, the destination is unable to use it, because the destination profile doesn't have the symmetric key from source/key4.db. The key in destination/key4.db cannot decrypt it.

The same applies to encrypted-openpgp-passphrase.txt
And secring.gpg depends on the decrypted encrypted-openpgp-passphrase.txt, so you cannot use a copied secring.gpg either.

Comment 3

[Kai Engert [:KaiE:]]

Besides OpenPGP keys, the source profile may also contain S/MIME keys and certificates, which are stored in files key4.db and cert9.db

Copying over OpenPGP and S/MIME keys and certificates from one profile to another will require nontrivial merging code.

Comment 4

[Kai Engert [:KaiE:]]

In my opinion, achieving an automatic import is difficult.

You would have to open the source databases, read all entries, and then import it into the destination databases.
Keys and certificates can have user defined flags associated to them, such as trust and acceptance.

You need to be prepared for the situation that source and destination could have the same attributes with different flags. At least you would require sane handling for that.

Simply copying over all entries is not what you want to do - because the profile importer on screen says it will NOT overwrite.

There's the additional complexity that the source profile could be protected by a primary password. In order to handle that, you'd have to unlock the source profile NSS database, to ensure that you can use its key4.db symmetric key to decrypt the source data.

This means prompting for the user for the primary password of the source profile. Then creating another instance of the code that can properly read and provide the data from the source profile (such as the list of decrypted logins).

Comment 5

[Kai Engert [:KaiE:]]

Given the complexity of this task, I think we need to take a step back, and meet and discuss what functionality we want, and how much we're willing to invest in the code that would be required.

I think that a direct importer, reading from the source, might not be the best approach.

It might be better (much easier) to avoid having to open two databases in one Thunderbird sessions.
Rather, we could consider a combined export and import functionality for secrets.

For example, when running the source profile, the user could use a functionality to "export all secrets" to a password protected archive. Then the user could run the destination profile, and use an "import secrets from backup file" function.

We already have some of that functionality. A user is able to export all public certificates, export all personal S/MIME certificates, export all public OpenPGP keys, backup all secret OpenPGP keys, and use corresponding functionality in Thunderbird to import those created files.

I don't know if we already have that for exporting and importing saved logins.

Comment 6

[Martin Giger [:freaktechnik]]

Good point, we should also think about how the pgp import/export should interact with our global import/export. Especially since we grouped all the other import features into the new wizard.

Comment 7

[Ping Chen [:rnons] (away)]

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

The importer says "Any existing or identical data will not be overwritten."
It's good that it will never delete what you already have in the destination profile.
But what will happen if both source and destination already have data of the same type? Will it be merged?
I think the text shown to the user should explain that better.

In your initial patch, you attempt to simply copy files, including key4.db and logins.json etc.
This approach cannot work.

Every profile immediately has file key4.db. You can never copy it over, because copying would replace the file the destination already has.

File key4.db in each profile may contain many things, however, at the very least, it contains a unique, randomly generated, symmetric encryption key.

Files login.json and encrypted-openpgp-passphrase.txt directly depend on that key. They are unusable without it.

Let's say source contains key4.db, login.json and encrypted-openpgp-passphrase.txt and secring.gpg

If you copy logins.json to destination, the destination is unable to use it, because the destination profile doesn't have the symmetric key from source/key4.db. The key in destination/key4.db cannot decrypt it.

In the code, we don't copy logins.json alone, we always copy it together with key4.db.

The same applies to encrypted-openpgp-passphrase.txt
And secring.gpg depends on the decrypted encrypted-openpgp-passphrase.txt, so you cannot use a copied secring.gpg either.

Even if a new profile has its own key4.db and encrypted-openpgp-passphrase.txt, in practice, if we assume it's an empty proflie (no logins.json, no ring.gpg), copy should work, at least in my testings.

I'm not saying copy is the best approach, a specific process for export/import secrets would be good as well.

Comment 8

[Magnus Melin [:mkmelin]]

Agreed it might be too difficult to merge anything existing. But the main use case for import is very likely to init a fresh profile from backup when moving to another computer. For those cases more or less copy should work I'd assume.

Comment 9

[Andrei Hajdukewycz [:sancus]]

I think there's two approaches here:

1. Check if the profile has existing data that would be messed up by the copy. If not, just copy. If it does have data, return an error message accordingly. It's definitely NOT acceptable to just naively copy and hope you don't damage a profile, even if it's an edge case.

2. Support proper import/export of keys the same way the OpenPGP Key Manager does, but just group this functionality into the global import/export. To me it would be most logical to re-use that functionality for this. Include a proper key manager export in the profile zip file.

If #2 is substantially more work as I would guess, I'm not sure it's worth doing.

Comment 10

[Ping Chen [:rnons] (away)]

I created bug 1779687. Let's talk about whether my patch is acceptable.

I think the critical part is this

    if (
      targetLoginsJson.exists() || targetPubring.exists() || targetSecring.exists()
    ) {
      // Do nothing if logins.json or keyrings exist in the current profile.
      return;
    }

Does it work? Yes.
Is it safe to overwrite key4.db after this check? Maybe, I'd like to see a concrete counter example that shows something can be broken by the patch. Thanks.

Comment 11

[Magnus Melin [:mkmelin]]

Aren't the key rings initialized as soon as the profile is initialized though?

Comment 12

[Ping Chen [:rnons] (away)]

Tested just now,

key4.db
encrypted-openpgp-passphrase.txt
openpgp.sqlite

are created in new profile, pubring.gpg and secring.gpg are created when creating/importing the first key.

Comment 13

[Kai Engert [:KaiE:]]

(In reply to Ping Chen (:rnons) from comment #10)

    if (
      targetLoginsJson.exists() || targetPubring.exists() || targetSecring.exists()
    ) {

Is it safe to overwrite key4.db after this check?

No, it isn't safe.

Maybe, I'd like to see a concrete counter example that shows something can be broken by the patch. Thanks.

In the following scenario you'd wipe user data:

• when setting up an email account, the user UNchecks "remember password".
  This means targetLoginsJson.exists() is false

• user does not use OpenPGP.
  This means targetPubring.exists() || targetSecring.exists() is false

• user imports a personal S/MIME certificate. The imported secret key is stored in file key4.db

If you overwrite key4.db, the secret key will be lost.

Comment 14

[Ping Chen [:rnons] (away)]

Thanks, I missed the case of unchecking "remember password". What do you say if I also check accounts?

    if (MailServices.accounts.accounts.length ||targetLoginsJson.exists() || targetPubring.exists() || targetSecring.exists()) {
      return;
    }

Seems OpenPGP keys can be imported before creating accounts, but S/MIME certificate can't be imported without a mail account, right?

Comment 15

[Kai Engert [:KaiE:]]

You can import S/MIME certificates without an account.
Settings, Privacy&Security, Manage Certificates. Your Certificates, Import.

Also consider the scenario that a user might have created an email account, then deletes the email account, then goes to import.

Comment 16

[Ping Chen [:rnons] (away)]

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

Also consider the scenario that a user might have created an email account, then deletes the email account, then goes to import.

Because Local Folders can't be deleted, I guess this is not a problem.

Comment 17

[[:emanuel_le] Ema]

Since for me the passwords were transferred correctly from the old profile (seen in Settings > Privacy& Security > Passwords), a temporary fix for me was to simply rename the encrypted-openpgp-passphrase.txt and then manually export the keys from the old profile and import them in the new one.
Just a quick note for those who are still looking for a solution.

Comment 18

[Kai Engert [:KaiE:]]

In my opinion, the Thunderbird profile import must never copy any of the NSS and OpenPGP files.
Users should be required to backup secret keys in the old profile, and import them in the new file.

Comment 19

[Magnus Melin [:mkmelin]]

Wouldn't that mean people would have to re-enter all passwords for all accounts? I'd think people definitely expect those to transfer over.

I do think it would be important to fix this bug.
Since it seems hard to determine 100% if it's ok to proceed, perhaps a confirmation dialog is in order for the cases that are problematic. And a notification of what didn't get imported if something didn't.

Comment 20

[Kai Engert [:KaiE:]]

I fully disagree with the suggested "solution" to add checkboxes for importing.
That isn't a solution.

You cannot solve the problem by transferring the responsibility to the user, by requiring them to check a checkbox. They cannot know that checking the checkbox will cause profile corruption.

We must not ship functionality that corrupts the profile. Period.

Despite my comments in this bug, which clearly explain which scenarios cannot work, I haven't yet seen steps to disable the functionality, and I haven't yet see any attempts to help users repair their broken functionality.

I think it's necessary that I jump in to help.

Comment 21

[Kai Engert [:KaiE:]]

This problem will require several steps to handle.

First, we must stop breaking profiles immediately: bug 1790605
Second, we must find a way to help users to corrupt their broken profiles: bug 1790610
Third, we can consider to improve the feature, possibly in this bug here.

Comment 22

[Andrei Hajdukewycz [:sancus]]

I agree we should disable password copying until we have a proper fix for this import.

Comment 23

[Kai Engert [:KaiE:]]

We are done handling the urgent steps mentioned in comment 21.

Let's morph this bug, use it for tracking a better implementation of logins/key importing.

Comment 25

[Kai Engert [:KaiE:]]

(In reply to Magnus Melin [:mkmelin] from comment #19)

Wouldn't that mean people would have to re-enter all passwords for all accounts?

No.

The Firefox password manager offers to "export" logins. And it's possible to backup OpenPGP secret keys and S/MIME secret keys.

The correct way is to start Thunderbird using the existing profile, and run the export functionality for all that data.
That will use of the NSS secret symmetric key stored in the old profile to decrypt and export (without requiring to copy that NSS secret symmetric key into the new profile).

A long time ago, we had code in core Firefox that allowed us to open two NSS databases in parallel, and also to shutdown an NSS database. That code wasn't very reliable, because of NSS strict shutdown requirements paired with JS delayed cleanup of objects. I suggest to avoid the approach.

It's easiest to ask the user to run Thunderbird using the old profile (maybe we can assist the user somehow in doing so, by restarting Thunderbird on that profile?), and asking the user to start a new "export all secrets" functionality, which would ask the user for a password that can be used to protect all those secrets while in transport. That would create a couple of files in a new directory. Then, the user should open Thunderbird with the desired target profile, and then access a new functionality "import my previously exported secrets".

Comment 26

[Matt]

Magnus directed me here when I was complaining about profile import not bringing in passwords, so I am assuming this is the right bug for that I actually have any PGP keys. I don't use it. I limit my encryption to s/mime. I was truly astounded to see a discussion on some sort of ethical nature about PGP keys on a failure to import my profile bug.

I asked Thunderbird to import my profile and I pointed the import tool to my profile folder that I had backed up for the purpose of importing it. I did not get that, I got some fragment of the profile.

I could have just dropped the backed up profile into Thunderbird as a copy and replace on an existing profile and got what I wanted. I could have used the profile manager or about:profiles to load the copy and made it default. Instead, I tried out, the user interface that I have been pointing other users to using and was disappointed to find it was broken. It did not import my complete profile. Ipso facto it is broken.

Then I see progress on fixing the bug is basically stalled while there is some philosophical discussion on what is "right" and that this bug should be "morphed" into some sort of tracking bug.

While all this is happening, a fix is not bubbling to the top and no bug exists to actually fix that which is broken, profile import in it's entirety. That is unacceptable, really.

Comment 27

[Andrei Hajdukewycz [:sancus]]

(In reply to Matt from comment #26)

Then I see progress on fixing the bug is basically stalled while there is some philosophical discussion on what is "right" and that this bug should be "morphed" into some sort of tracking bug.

While all this is happening, a fix is not bubbling to the top and no bug exists to actually fix that which is broken, profile import in it's entirety. That is unacceptable, really.

Seems like you have misunderstood some comments. Password import never worked properly because passwords are stored encrypted in key4.db. Simply copying on top of this file creates a risk of silently breaking profiles. Which is why we disabled this in bug 1790605.

There is no "philosophical" argument here at all. There was a technical discussion on the possible ways to fix this while avoiding the old problems. Currently, this issue isn't assigned to anyone and that's why it's not moving forward. We will get to it, and to the whole mess that is Import/Export.

I would suggest you avoid making demands, or declaring things "astounding" and "unacceptable". It won't speed up bug fixes but it definitely will lead you to a Bugzilla vacation.

Comment 28

[Matt]

(In reply to Andrei Hajdukewycz [:sancus] from comment #27)

I would suggest you avoid making demands, or declaring things "astounding" and "unacceptable". It won't speed up bug fixes but it definitely will lead you to a Bugzilla vacation.

I just love the way Mzla handles dissent. I will show myself out.

Comment 29

[Kai Engert [:KaiE:]]

I'm repeatedly seeing requests to restore a profile, users discover the import, and notice it's incomplete, and are unhappy.

Merging would be complicated, as explained above.

Here's a new suggestion:

Tell the user that merging in secret keys and passwords is impossible. Tell the user, if their intention is to restore that data from the source, the only way is to fully overwrite all such data in the current profile. Ask them to confirm if that's really what they want, tell them that existing keys and passwords stored in the current profile will be gone.

If the user accepts, overwrite. If this sounds acceptable, we can try if that works.

Because the current profile might contain a newer software version than the backup, the newer profile might cause additional files that cannot coexist with the backup. So, the code performing this step would have to use a list of all security related files of the current versions, then in a first step remove (backup?) all of those files, and in a second step, copy over all the files from the source that exist, then restart.

Comment 30

[Kai Engert [:KaiE:]]

As of today, the list of files are:
key3.db
key4.db
cert8.db
cert9.db
secmod.db
pkcs11.txt
encrypted-openpgp-passphrase.txt*
logins.json*
enigmail.sqlite
openpgp.sqlite
pubring.gpg*
secring.gpg*
openpgp_alias_to_keys.json
otr.fingerprints
otr.instance_tags
otr.private_key

Note the wildcards

Comment 31

[Kai Engert [:KaiE:]]

Well, instead of dealing with the wildcards, it might be sufficient to deal with

encrypted-openpgp-passphrase.txt
encrypted-openpgp-passphrase.txt.old
logins.json
pubring.gpg
pubring.gpg.old
secring.gpg
secring.gpg.old

(plus all the other files, which I listed in comment 30 without a * wildcard)

However, I'd prefer handling the wildcards, to result in a clean state, without old leftover files.

Comment 32

[Kai Engert [:KaiE:]]

I need to take a step back.

I realize that my new suggestion appears to be similar to the suggestion that Ping made in the attached patch.
So we need to clarify what's different.

First, when the suggestion was made in the past, we were still analyzing what the cause for corruption was.
I mentioned that in phabricator.
I said, the first step should be about repairing, and maybe afterwards we could get back to the suggestion that Ping made.

The repairing work has been completed in the meantime, and afterwards I forgot / was distracted, and didn't look at the potential strategy "replace everything" again (until now).

I had also warned about some additional details that need to be considered, like ensuring that all files getting copied to keep consistency, not just a few of them.

Then I brought up the possibility of an Add-On using keys from key4.db that might protect data in the existing profile, and the inability to learn about that. That's true. For that reason, I think any full copying of that data into an existing profile should explicitly remind the user to make a full backup of a profile.

But what is a backup of a profile worth, if we don't offer an easy way to restore it?

This brings me to the following:

More importantly than "importing" a profile, in the sense of "merging" a profile, we need a way to "restore" a profile.

We need a way to allow the user to say "take this backup, or this other profile, and restore it as a fresh profile on this machine".
I think this is what many users are looking for actually.
Do they really want merging, or it it restoring that they want?

Maybe "import profile" should create a local profile that is an exact copy of the source data the user selects.