Closed Bug 1764242 Opened 4 years ago Closed 4 years ago

OpenPGP integration refusing BrainpoolP512r1 ECC key after 91.8 upgrade

Categories

(MailNews Core :: Security: OpenPGP, defect, P1)

Product:
MailNews Core
Component:
Security: OpenPGP
Version:
Thunderbird 91
Type:
defect

Tracking

(thunderbird_esr91? fixed, thunderbird100? fixed)

Status:
RESOLVED FIXED
Milestone:
101 Branch
Tracking Flags:
Tracking Status
thunderbird_esr91 ? fixed
thunderbird100 ? fixed

People

(Reporter: pege, Assigned: KaiE)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [regression 91.7->91.8])

Attachments

(1 file)

Bug 1764242 - Re-enable RNP features that got accidentally disabled during upgrade to v0.16.0. r=rjl
48 bytes, text/x-phabricator-request
wsmwk
: approval-comm-beta+
wsmwk
: approval-comm-esr91+
Details | Review

Steps to reproduce:

After updating from 1:91.7.0-2deb11u1 to 1:91.8.0-1deb11u1 my PGP key
disappeared from the OpenPGP Key Manager and can no longer be imported.
I verified this on a second machine and the key was still there before
the upgrade and could also be imported properly. This changed after
upgrading.

Steps to reproduce:

  1. In menu, select "Tools" → "OpenPGP Key Manager"
  2. "edit" → "Import Key from URL" 1
  3. An error is shown

I suspect it's related to it being an BrainpoolP512r1 ECC key which
happens to be rather uncommon. I've not checked if any other keys are
affected.

Initially reported at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1009321.

Component: Security → Security: OpenPGP
Keywords: regression
Product: Thunderbird → MailNews Core
See Also: → http://bugs.debian.org/1009321
Whiteboard: [regression 91.7->91.8]

Nickolay, did anything change regarding this type of keys in RNP v0.16.0 ?

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

Nickolay, did anything change regarding this type of keys in RNP v0.16.0 ?

Magnus asked me to document how I analyze these kinds of bugs.

Let's check how RNP treats this key.
The Thunderbird build output tree contains the RNP "rnpkeys" utility that we can use.

I downloaded the key found in the debian report and saved it as /tmp/key.asc
mkdir /tmp/ring
rnpkeys --homedir /tmp/ring --import /tmp/key.asc
rnpkeys --homedir /tmp/ring --list-keys

That prints:

warning: no keys were loaded from the keyring '/tmp/r2/secring.gpg'.
7 keys found

pub   512/ECDSA 4a72372baf48dade 2020-12-31 [C]
      e6bfd964aa7cebd360537ddb4a72372baf48dade
uid           [redacted]
uid           [redacted]
sub   255/EdDSA 698a1d72c12c82b4 2022-01-06 [A] [EXPIRES 2023-01-31]
      1ce607601ea103ae876966d4698a1d72c12c82b4
sub   512/ECDSA 2b359167f46f6c59 2022-01-06 [S] [EXPIRES 2023-01-31]
      c12d10f4d38e3990fd75aa652b359167f46f6c59
sub   512/ECDH abeda274b4fc6718 2022-01-06 [E] [EXPIRES 2023-01-31]
      87cbea8a9a7b76a22463f561abeda274b4fc6718
sub   255/EdDSA 01e26e0aae5d9796 2020-12-31 [A] [EXPIRED 2022-02-04]
      37a75cf1798b4320ddf2a86601e26e0aae5d9796
sub   255/EdDSA a9bc0494bd1ae92e 2020-12-31 [S] [EXPIRED 2022-02-04]
      a6ce4e7e294bd1b02d389298a9bc0494bd1ae92e
sub   255/ECDH ea5164adf741144d 2020-12-31 [E] [EXPIRED 2022-02-04]
      e000043481dddff426f15a1cea5164adf741144d

So apparently we have three non-expired ECC subkeys for the various uses [A], [S], [E].

Hi Kai and Peter!
Nope, we didn't change anything related to the Brainpool curves support. This key seems to be okay, however key flags could be something which may cause problems: usually primary key has [sign, certify] flags.

Because the previous comment looks fine apparently, let's continue testing.

In a terminal I ran
export RNP_LOG_CONSOLE=1
and then I started Thunderbird from that terminal window.

Thunderbird says t failed to import the key.
On the console I see multiple error messages from RNP:

ECDSA verify: curve 8 is not supported.

Nickolay, does RNP refuse the ECDSA keys? Is that the reason why import fails?

I have to debug, with additional logging in our TB code, to understand at which point we decide we cannot continue, or which RNP functions gives us an error.

8 is definitely the BP512 curve, and it sits behind the ENABLE_BRAINPOOL define. It is enabled/disabled during CMake build, depending on use backend (enabled for Botan, and disabled for the OpenSSL backend as by default it doesn't have Brainpool curves). As I understand TB uses some custom build scripts, do those utilize CMake?

P.S. Also there are ENABLE_TWOFISH, ENABLE_SM2 and ENABLE_AEAD which are set depending on the backend detection.

We don't use CMake, we use the Mozilla build system, with two files moz.build and rnpdefs.mozbuild
found here:
https://hg.mozilla.org/comm-central/file/tip/third_party/rnp

We very likely don't set the define.
Is this a new define in v0.16.0 ? Were those always enabled in the earlier RNP version?

We need to decide which defines are safe for us to enable, e.g. we don't want SM2.

Yeah, those defines were added in v0.16.0 to be able to work with different backends. Also we may introduce new ones in the future. All of those are set by CMake and transferred to code via config.h, which is generated from config.h.in.

As from https://hg.mozilla.org/comm-central/file/tip/third_party/rnp/src/lib/config.h.in , you would need to change it contents to define ENABLE_BRAINPOOL, ENABLE_TWOFISH, CRYPTO_BACKEND_BOTAN and possibly ENABLE_AEAD.

Thanks Nickolay.

Rob, do you understand why our scripts added the following?

+#undef CRYPTO_BACKEND_BOTAN
+#undef CRYPTO_BACKEND_OPENSSL
+
+#undef ENABLE_SM2
+#undef ENABLE_AEAD
+#undef ENABLE_TWOFISH
+#undef ENABLE_BRAINPOOL

I don't recall having added those manually.

( from https://hg.mozilla.org/comm-central/rev/3a5532c4eba0fd92d41ea77113ba201028bdb927 )

Regressed by: 1750969

I should have taken more time to sufficiently review the changes introduced by the v0.16.0 upgrade.

The release notes have this sentence:
"Added compile-time switches to disable certain features (AEAD, Brainpool curves, SM2/SM3/SM4 algorithms, Twofish)"

It sounded to me, if we don't do anything, we wouldn't be using these new switches. I assumed that the default behavior would be to keep the functionality. It might be worth to have more detailed RNP release notes in the future, for example a sentence like:
"To ensure that you get the same functionality as before, make sure you define the following symbols: ..."

(In reply to Nickolay Olshevsky from comment #9)

define ENABLE_BRAINPOOL, ENABLE_TWOFISH, CRYPTO_BACKEND_BOTAN and possibly ENABLE_AEAD.

Nickolay, can you please clarify why you said "possibly" ENABLE_AEAD ?
Do you have any concerns regarding that?

In v0.15.2, was the code controlled by ENABLE_AEAD enabled or disabled?

Severity: -- → S2
status-thunderbird100: --- → affected
status-thunderbird_esr91: --- → affected
tracking-thunderbird100: --- → ?
tracking-thunderbird_esr91: --- → ?
Priority: -- → P1

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

Nickolay, can you please clarify why you said "possibly" ENABLE_AEAD ?
Do you have any concerns regarding that?

In v0.15.2, was the code controlled by ENABLE_AEAD enabled or disabled?

In v0.15.2 it was enabled, and it was and still compatible with GnuPG, despite being 'experimental'.
However, within the latest OpenPGP draft specification (https://datatracker.ietf.org/doc/html/draft-ietf-openpgp-crypto-refresh-05) proposed format for the AEAD encrypted data was changed. That gives concerns whether TB should understand old format or not, and the decision is up to you.
From my side, I'd prefer approach 'understand everything, emit only data as per specification', i.e. define ENABLE_AEAD.
In the next version(s) we'll add support for new specification as well.

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

Rob, do you understand why our scripts added the following?

I found this:
https://hg.mozilla.org/comm-central/file/tip/python/thirdroc/thirdroc/rnp.py#l99

This kind of automatism feels risky.

We should make sure that all changes to config need a manual inspection.

I recommend that we separate config changes from code changes. Everything that results in defines that control what gets built should have to be adjusted manually.

Assignee: nobody → kaie
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

I've tried the attached patch on the 91.x branch, and it fixes the use of the provided test key, it can be imported and its details are shown in key manager, and I no longer get the message from comment 5.

Is this a sufficient patch for the immediate hotfix, which we most likely should uplift quickly to the esr91 branch?

Afterwards we should work on a better patch for comm-central to handle that in a better way, maybe Rob has suggestions?

That code changes config.h.in so that it is in autoconf-style preprocessable form rather than CMake. At build time, rnp/src/lib/config.h is generated from it.

I should have reviewed the CMakeLists.txt file in the RNP source root where the new defaults are set.

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

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

Rob, do you understand why our scripts added the following?

I found this:
https://hg.mozilla.org/comm-central/file/tip/python/thirdroc/thirdroc/rnp.py#l99

This kind of automatism feels risky.

We should make sure that all changes to config need a manual inspection.

I recommend that we separate config changes from code changes. Everything that results in defines that control what gets built should have to be adjusted manually.

You're certain that the #define will always be seen later than #undef ?

Yes, because of the preprocessing that happens on that file via https://hg.mozilla.org/comm-central/file/tip/third_party/rnp/moz.build#l45

In your object-dir you will have rnp/src/lib/config.h with the result.

Keywords: checkin-needed-tb

Comment on attachment 9271941 [details]
Bug 1764242 - Re-enable RNP features that got accidentally disabled during upgrade to v0.16.0. r=rjl

[Approval Request Comment]
Regression caused by (bug #): 1750969
User impact if declined: broken functionality
Testing completed (on c-c, etc.): manually - if you're worried, please ask someone to test manually (import the key from the referenced debian bug and confirm it can be imported and viewed with openpgp key manager)
Risk to taking this patch (and alternatives if risky): low

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

Comment on attachment 9271941 [details]
Bug 1764242 - Re-enable RNP features that got accidentally disabled during upgrade to v0.16.0. r=rjl

[Approval Request Comment]
So 100 beta 3 will have the fix as well.

Attachment #9271941 - Flags: approval-comm-beta?
Target Milestone: --- → 101 Branch

Pushed by thunderbird@calypsoblue.org:
https://hg.mozilla.org/comm-central/rev/595e38dcc8bd
Re-enable RNP features that got accidentally disabled during upgrade to v0.16.0. r=rjl

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

Comment on attachment 9271941 [details]
Bug 1764242 - Re-enable RNP features that got accidentally disabled during upgrade to v0.16.0. r=rjl

[Triage Comment]
Approved for beta
Approved for ers91

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

Attachment

General

Creator:
Created:
Updated:
Size: