Closed Bug 1713621 Opened 3 years ago Closed 2 years ago

OpenPGP ECC key created by Thunderbird, backed up to file, cannot be imported into GnuPG 2.2.27 (error shown is: Bad secret key)

Categories

(MailNews Core :: Security: OpenPGP, defect)

Product:
MailNews Core
Component:
Security: OpenPGP
Type:
defect

Tracking

(thunderbird_esr91+ fixed)

Status:
RESOLVED FIXED
Milestone:
99 Branch
Tracking Flags:
Tracking Status
thunderbird_esr91 + fixed

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

Attachments

(4 files, 1 obsolete file)

secring.gpg
4.25 KB, application/pgp-encrypted
Details
pubring.gpg
2.10 KB, application/pgp-encrypted
Details
ecc-20cc.asc
879 bytes, text/plain
Details
Bug 1713621 - Ensure exported secret ECC keys are compatible with GnuPG. r=mkmelin
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-esr91+
Details | Review

I used Thunderbird 78.10 to create an OpenPGP key of type ECC.

I used OpenPGP key manager, file, backup secret key, to create a backup file (password protected).

Then I used gpg --import to import the file into a fresh GnuPG keyring (--homedir /tmp).

GnuPG prompts twice for a password, which is surprising. I pasted the correct password, so it shouldn't ask more than once.

After having entered the password the second time, gpg reports a failure:

gpg: key 571BFB61C18020CC: public key "test <testmail@kuix.de>" imported
gpg: key 571BFB61C18020CC/1A5ABE91B032EB43: error sending to agent: Bad secret key
gpg: error reading '/tmp/ecc-20cc.asc': Bad secret key
gpg: import from '/tmp/ecc-20cc.asc' failed: Bad secret key
gpg: Total number processed: 0
gpg:               imported: 1
gpg:       secret keys read: 1

It's surprising that gpg reports that a secret key was imported, in addition to the error. The key can be listed:

$ gpg --homedir=/tmp/imp --list-secret-keys --with-subkey-fingerprints
/tmp/imp/pubring.kbx
--------------------
sec   ed25519 2021-05-31 [SC] [expires: 2021-08-29]
      B8710B1FF8EDDFD03E0539C2571BFB61C18020CC
uid           [ unknown] test <testmail@kuix.de>
ssb#  cv25519 2021-05-31 [E] [expires: 2021-08-29]
      50BB93B18A8EE40CD178D44E1A5ABE91B032EB43

Encrypting using this key works, however, decrypting fails with "no secret key".

So apparently GnuPG imported some incomplete or corrupted key.

We need to investigate which component has a bug, RNP, Thunderbird or GnuPG.
I will attach the files that I've created.

Attached file secring.gpg —

secring.gpg file from the Thunderbird profile directory.

Note it contains two keys. The RSA key works fine. The problem is seen with the ECC key, last 4 digits are 20cc.

Password for the secret keys is: 6af158db75776f5f464736138455c864

Attached file pubring.gpg —
Attached file ecc-20cc.asc —

This is the result of using Thunderbird to backup the ECC secret key to file.

Password: 20cc

I ran the pgpdump utility on both secring.gpg and backup file. It reports this failure then aborts:

Old: Secret Key Packet(tag 5)(until eof)
pgpdump: unknown version (116).
	Ver 116 -

Note Thundebird still uses RNP v0.14.0 - however - I tried locally using v0.15.0 to create the backup file, and it gives the same results.

Flags: needinfo?(o.nickolay)

I need some time to investigate it further, currently noticed following:

  • secret subkey, once imported to the RNP, is still valid and usable (i.e. we can encrypt by GnuPG and decrypt by RNP), so there is no key corruption and I suspect some bit problem, like with the EdDSA case.
  • unlike EdDSA, when problem happens 50% time, in this case it happens each time (or I was unlucky to get 4-5 wrong keys in a row).
Flags: needinfo?(o.nickolay)

Tried multiple things, but still cannot see what may go wron from the RNP side - each generate subkey seems to be pretty valid an working.
Filed an issue on GnuPG bug tracker: https://dev.gnupg.org/T5464

Nickolay, thanks for your work on this issue.

I see from the discussion in the gnupg ticket that RNP needs to use a different binary storage format.
I conclude the secret keyring files stored in the Thunderbird profile currently use an incorrect format for ECC keys.

I assume you will change how RNP saves those ECC keys, so that keyrings saved in the future will be correct.

I assume the complicated part is, will you be able to fix this in a backwards compatible way?
In other words, will future versions of RNP be able to distinguish if a keyring file contains the old incorrect or the new correct key material?

And another scenario we should take care of:
What happens if a user starts a newer version of Thunderbird once, modifies OpenPGP data, and then starts an older version of Thunderbird again?
If the newer TB version modified the key material storage, would the old TB version then see incorrect key data?

I'm worried this might be complicated to fix.

Hi Kai,

I assume the complicated part is, will you be able to fix this in a backwards compatible way?

I'm thinking about a good way to fix this issue. As such secret key is used only for the decryption, we may check for correspondence between the secret key and public key before starting the decryption, and if not - change byte order and try again. The same can be done after the key unprotection (so exported key will be compatible with GnuPG). So, new RNP/TB will be compatible with old RNP, however newly generated keys will not be compatible with old RNP.

What happens if a user starts a newer version of Thunderbird once, modifies OpenPGP data, and then starts an older version of Thunderbird again?
If the newer TB version modified the key material storage, would the old TB version then see incorrect key data?

If newer version doesn't re-write keyring, then thing would work fine. However, if user will generate new ECC key in new TB (or export-import it), and then run old TB, he will not be able to decrypt messages.

Will think more on this, and see how things are resolved on GnuPG side.

Update: after discussion on https://dev.gnupg.org/T5464 thing are cleared up, and will be fixed on RNP side.
It's not a byte order issue, but problem with some bit tweaking for 25519 keys: for the secret keys low 3 bits and high one must be 0, and high-1 must be 1. Botan handles this automatically in secret-key related operations, while libgcrypt doesn't (so stored secret key must have correct those bits).

So the solution would/could be as following:

  • in newly generated keys set those bits as they should be when saving.
  • add some flag to rnp_key_export() which would fix up x25519 key material during export
  • add some command line option to rnpkeys to allow to fix key as well.

Within these changes there would be no problems with simultaneous usage of old and new keys/rnp library versions.

Fix for this issue landed RNP master, and will be included to the RNP v0.16.0 release.
Within fix:

  • new keys will be generated with correctly tweaked bits
  • using secret key with non-tweaked bits would issue a warning
  • FFI functions rnp_key_25519_bits_tweaked()/rnp_key_25519_bits_tweak() added to be able to fix keys (during export, for instance)
  • CLI command --edit-key [--check-cv25519-bits | --fix-cv25519-bits] added, allowing to fix older key

Nickolay, a very common scenario will be:

  • user has created an ECC key with an old Thunderbird/RNP version
  • user updates to new Thunderbird/RNP version
  • user assumes Thunderbird has fixed this bug
  • user uses backup/export of secret key
  • user expects that this new export can be imported into GnuPG

I just tested this with a recent snapshot of RNP, but I still get the same behavior as initially reported here.
Well, my current version of GnuPG now reports it as having a bad secret key.

Does this mean RNP doesn't automatically fix an old key when trying to export it using new RNP ?

Looking at comment 13, maybe you require that the application calls rnp_key_25519_bits_tweak prior to export?

How do we know if calling this function is necessary for an old bad key?

Or could we just call the function always - or would that corrupt new, unaffected keys?

Kai, yep, RNP cannot automatically fix it as usually key is encrypted. So suggested way to fix it via Thunderbird is:

  • during the key export, call rnp_key_25519_bits_tweaked() on the unlocked key
  • if previous is true, then call rnp_key_25519_bits_tweak()
  • lock (protect) key back.

Also this is implemented in the RNP CLI via rnpkeys --edit-key --check-cv25519-bits/rnpkeys --edit-key --fix-cv25519-bits commands.

Nickolay, it isn't completely obvious what "tweaked true" and "tweaked false" mean.

I suggest to improve the documentation for rnp_key_25519_bits_tweaked and say clearly what each of these mean.

For example, "if rnp_key_25519_bits_tweaked returns true, then you must call rnp_key_25519_bits_tweak. If rnp_key_25519_bits_tweaked returns false, then do NOT call rnp_key_25519_bits_tweak."

I personally think with the naming "tweak" isn't easy to understand what's going on here. It seems that this is about "repairing to make standards compliant".

A possible easier to understand naming could be "rnp_key_25519_is_compliant(key, bool*)" and "rnp_key_25519_make_compliant(key)".

Not sure what's wrong, I'm calling rnp_key_unlock and directly afterwards rnp_key_25519_bits_tweaked, and I get result RNP_ERROR_BAD_PARAMETERS.
(This is with snapshot ec2a79ee9644f39eef53fca6cf364374d0617ec3 see bug 1750969.)

4144	    if (!seckey || seckey->is_locked() || (seckey->alg() != PGP_PKA_ECDH) ||
4145	        (seckey->curve() != PGP_CURVE_25519)) {

(gdb) print seckey->alg()
$3 = PGP_PKA_EDDSA

(gdb) print seckey->curve()
$4 = PGP_CURVE_ED25519

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

Nickolay, it isn't completely obvious what "tweaked true" and "tweaked false" mean.

Agree here, but I wasn't unable to find better naming for "set 2 high bits and unset 3 low bits". So I picked word 'tweak' from this comment: https://dev.gnupg.org/T5464#146922

I suggest to improve the documentation for rnp_key_25519_bits_tweaked and say clearly what each of these mean.

Thanks, I'll update the comment, missed to fill it up.

I personally think with the naming "tweak" isn't easy to understand what's going on here. It seems that this is about "repairing to make standards compliant".

A possible easier to understand naming could be "rnp_key_25519_is_compliant(key, bool*)" and "rnp_key_25519_make_compliant(key)".

This comment also explains this decision: https://dev.gnupg.org/T5464#146878
Basically, GnuPG implementation was done before the standartization, and the way we implemented is actually standard-compliant (as we have standard now).

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

4144	    if (!seckey || seckey->is_locked() || (seckey->alg() != PGP_PKA_ECDH) ||
4145	        (seckey->curve() != PGP_CURVE_25519)) {

(gdb) print seckey->alg()
$3 = PGP_PKA_EDDSA

(gdb) print seckey->curve()
$4 = PGP_CURVE_ED25519

You should call it on X25519 subkey, not a EdDSA primary key. EdDSA issue was on the GnuPG side and is already fixed: https://dev.gnupg.org/T5114
(it's a bit messy that both called 25519, I was also hit with this number of times).

It still doesn't work.
Now, I only try to process subkeys, I check that rnp_key_get_alg is ecdh and rnp_key_get_curve is curve25519. For such a key, rnp_key_25519_bits_tweaked returns false! (If I try to call it anyway, rnp_key_25519_bits_tweak returns failure.)

I'm using a keyring that only has this one key (and its subkey).
Exporting this key (without calling rnp_key_25519_bits_tweak) cannot be imported with gnupg.
I'll attach it.

Attached file ecc3.asc (obsolete) —

password is: x

Hmm, did you try to run this via CLI? For me it works perfectly well:

$ rnpkeys --homedir .rnp --edit-key 5ff7324b9d373c28 --check-cv25519-bits
ssb   255/ECDH 5ff7324b9d373c28 2022-01-19 [E] [EXPIRES 2022-04-19]
      e137e4d047b0efd5e339cf805ff7324b9d373c28
Enter password for key 0x5FF7324B9D373C28: 
Warning: Cv25519 key bits need fixing.

$ rnpkeys --homedir .rnp --edit-key 5ff7324b9d373c28 --fix-cv25519-bits
ssb   255/ECDH 5ff7324b9d373c28 2022-01-19 [E] [EXPIRES 2022-04-19]
      e137e4d047b0efd5e339cf805ff7324b9d373c28
Enter password for key 0x5FF7324B9D373C28: 

$ rnpkeys --homedir .rnp --edit-key 5ff7324b9d373c28 --check-cv25519-bits
ssb   255/ECDH 5ff7324b9d373c28 2022-01-19 [E] [EXPIRES 2022-04-19]
      e137e4d047b0efd5e339cf805ff7324b9d373c28
Enter password for key 0x5FF7324B9D373C28: 
Cv25519 key bits are set correctly and do not require fixing.

$ gpg --homedir .gpg --import .rnp/secring.gpg 
gpg: WARNING: unsafe permissions on homedir '/Users/nickolay/Ribose/rnp-build/.gpg'
gpg: keybox '/Users/nickolay/Ribose/rnp-build/.gpg/pubring.kbx' created
gpg: /Users/nickolay/Ribose/rnp-build/.gpg/trustdb.gpg: trustdb created
gpg: key E0E538F73CF285AD: public key "test <testmail@kuix.de>" imported
gpg: warning: lower 3 bits of the secret key are not cleared
gpg: key E0E538F73CF285AD: secret key imported
gpg: Total number processed: 1
gpg:               imported: 1
gpg:       secret keys read: 1
gpg:   secret keys imported: 1

Could you please provide the code snippet/sequence of calls you are using?

(In reply to Nickolay Olshevsky from comment #16)

  • during the key export, call rnp_key_25519_bits_tweaked() on the unlocked key
  • if previous is true, then call rnp_key_25519_bits_tweak()

I read fficli.cpp and cli_rnp_t::fix_cv25519_subkey

The above should be "if the result is FALSE, then call rnp_key_25519_bits_tweak()".

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

It still doesn't work.
Now, I only try to process subkeys, I check that rnp_key_get_alg is ecdh and rnp_key_get_curve is curve25519. For such a key, rnp_key_25519_bits_tweaked returns false! (If I try to call it anyway, rnp_key_25519_bits_tweak returns failure.)

The reason why I got the failure:

In addition to what you said above (and what the documentation in rnp.h says), function cli_rnp_t::fix_cv25519_subkey also calls rnp_key_unprotect.

I added a call to rnp_key_unprotect, and now my code works.

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

I read fficli.cpp and cli_rnp_t::fix_cv25519_subkey

The above should be "if the result is FALSE, then call rnp_key_25519_bits_tweak()".

Almost all of RNP API functions returns rnp_result_t, which is 0 (or RNP_SUCCESS) on success or some error code.
So for rnp_key_25519_bits_tweaked() you should first check whether call succeeded (function returned 0), and then whether bool result, pointer to which you pass to function, is true.

I added a call to rnp_key_unprotect, and now my code works.

Glad to hear it worked.

(In reply to Nickolay Olshevsky from comment #28)

The above should be "if the result is FALSE, then call rnp_key_25519_bits_tweak()".

Almost all of RNP API functions returns rnp_result_t, which is 0 (or RNP_SUCCESS) on success or some error code.
So for rnp_key_25519_bits_tweaked() you should first check whether call succeeded (function returned 0), and then whether bool result, pointer to which you pass to function, is true.

Except that it's the other way round.

If tweaked is true, you don't tweak.
https://github.com/rnpgp/rnp/blob/1a662452d8458a40c4046c5d6f3178ce3529eff2/src/rnp/fficli.cpp#L839

Attachment #9259750 - Attachment description: WIP: Bug 1713621 - Ensure OpenPGP ECC key compliance on secret key export. → WIP: Bug 1713621 - Ensure exported secret ECC keys are compatible with GnuPG.
Attachment #9259777 - Attachment is obsolete: true

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

(In reply to Nickolay Olshevsky from comment #28)

The above should be "if the result is FALSE, then call rnp_key_25519_bits_tweak()".

Almost all of RNP API functions returns rnp_result_t, which is 0 (or RNP_SUCCESS) on success or some error code.
So for rnp_key_25519_bits_tweaked() you should first check whether call succeeded (function returned 0), and then whether bool result, pointer to which you pass to function, is true.

Except that it's the other way round.

If tweaked is true, you don't tweak.
https://github.com/rnpgp/rnp/blob/1a662452d8458a40c4046c5d6f3178ce3529eff2/src/rnp/fficli.cpp#L839

Argh, my fault, sorry for messing it up, starting from the comment 16. It would be better to reply with more fresh head.
Definitely you are right: if tweaked = true then everything is okay with a key. And if it is false then key needs fixing.

Assignee: nobody → kaie
Attachment #9259750 - Attachment description: WIP: Bug 1713621 - Ensure exported secret ECC keys are compatible with GnuPG. → Bug 1713621 - Ensure exported secret ECC keys are compatible with GnuPG. r=mkmelin
Status: NEW → ASSIGNED

Thanks NIckolay, now that we have RNP v0.16.0 landed, we can proceed with this, I'll ask Magnus for review.

Depends on: 1750969
Keywords: checkin-needed-tb

setting needinfo for myself, this needs to be nominated for esr91 after landing

Flags: needinfo?(kaie)

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2ea685b29ffc
Ensure exported secret ECC keys are compatible with GnuPG. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch

jens, lingwang, did you ever test this using a thunderbird nightly build or beta build, to confirm it creates a compatible export?

Please note, don't use your regular thunderbird profile for testing.

Flags: needinfo?(kaie)

Comment on attachment 9259750 [details]
Bug 1713621 - Ensure exported secret ECC keys are compatible with GnuPG. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: broken ECC key export
Testing completed (on c-c, etc.): still waiting for end user feedback
Risk to taking this patch (and alternatives if risky): little risk, effects limited to broken ECC key export

Attachment #9259750 - Flags: approval-comm-esr91?

Yes, I can confirm that keys exported with TB 99.0b2 appear to be compatible with GPG 2.2.28. I have tested several operations (key and message signing, encryption, decryption, changing passphrase) without any issues.

Comment on attachment 9259750 [details]
Bug 1713621 - Ensure exported secret ECC keys are compatible with GnuPG. r=mkmelin

[Triage Comment]
Approved for esr91

Attachment #9259750 - Flags: approval-comm-esr91? → approval-comm-esr91+
See Also: → 1867765
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: