1898832 - TB with external GnuPG may send corrupted signed+encrypted messages, for example if GnuPG is configured to use "armor" by default

Status: RESOLVED FIXED

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

Description

[Kai Engert [:KaiE:]]

Using TB 115.x
External GnuPG allowed, and account configured to use a personal key that is externally managed by GnuPG.
Sending an email that is both signed and encrypted.

For some users, this always creates corrupted messages.
Corrupted means: The copy stored in the Sent folder cannot be processed by Thunderbird.
Other OpenPGP software fails to process it, too.

On my system, with the equivalent configuration, I get good messages.

I have been given test messages and I have a theory what happens.

In the given scenario, we call GPGME to create a signed message, and then we use a RNP API to create the encrypted message.

In the broken test messages I've been given:
With gpg --decrypt or --list-packets, it beings to decrypt, but then reports a series of invalid packets.
Looking carefully at the packet identifiers, I see that the packet versions match the ASCII codes of the string ---- BEGIN PGP MESSAGE

Using gpg --unwrap I get an ASCII armored message that gpg can successfully decode and verify.

So apparently, Thunderbird (sometimes) obtains an ASCII armored message when asking GPGME to create a signed message (gpgme_op_sign).

I believe the reason for the unpredictable behavior is that a difference in behavior between GPGME versions. I recall having seen that in the past. I suspect some versions may default to generating armored, other versions might generate binary signed messages.

This would explain why I cannot reproduce the generation of the corrupted messages on my system, assuming that I have a version that produces binary messages by default, while on the system that produces the corrupted messages, a GPGME version is used that produces armored messages by default.

(It is a theory that it depends on the GPGME version. Maybe there are other factors that influence the behavior.)

In Thunderbird, when using GPGME, we currently don't call gpgme_set_armor to explicitly disable armoring.
I can produce the seen corrupted format when calling gpgme_set_armor(ctx, 1);

It seems the issue could be fixed by always calling gpgme_set_armor(ctx, 0); in this scenario.

While the above already suggests a fix, I'd like to describe another surprising effect, which makes it confusing to reproduce and test the issue with Thunderbird.

On my system, if Thunderbird HAS an internal copy of the related secret key, then RNP's initial attempt to decrypt the message will result in error code BAD_FORMAT. When TB sees this code, it stops processing, and shows the broken padlock error icon. I'm guessing, if RNP has the secret key available, it starts by decrypting, but then is surprised by the inner armored data, and fails to process it, and returns the error.

However, if Thunderbird does NOT have an internal copy of the related secret key, then RNP's processing will complain that it cannot decrypt, and in that scenario, we currently route the message to GPGME, which can decrypt the message. In this scenario, I see that Thunderbird will continue to process the message and display it.

So in order to be able to process past broken messages, it might be useful to fall back to GPGME also when seeing the BAD_FORMAT error when trying to decrypt. I'm not sure we should always do that, because in the past, when trying a similar approach in bug 1679768, we saw some regressions when handling the BAD_FORMAT error in this way.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Bug 1898832 - With external GnuPG config, never use inner ASCII armor for signature, and fall back to GPGME decoding when seeing BAD_FORMAT error. r=mkmelin [already in tb-128]

Comment 2

[Kai Engert [:KaiE:]]

Attachment: 1898832-backport-esr115.patch

Comment 3

[Kai Engert [:KaiE:]]

Test builds based on Thunderbird 115.x:

linux 64
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/XHfKP291TjC7rfVfiNQDxg/runs/0/artifacts/public/build/target.tar.bz2

windows 64
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/JEnwRAs3Sxemje6jxmY5VQ/runs/0/artifacts/public/build/target.zip

windows 32
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/TAW0u7nmR-aTOFNq11JMbA/runs/0/artifacts/public/build/target.zip

( https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=789d8d2739ee18fc4c512f85a00bba3ed6a1d07f )

Comment 4

[Kai Engert [:KaiE:]]

Attachment: 1898832-backport-esr115.patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a743748c3628
With external GnuPG config, never use inner ASCII armor for signature, and fall back to GPGME decoding when seeing BAD_FORMAT error. r=mkmelin

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 9403916 [details]
Bug 1898832 - With external GnuPG config, never use inner ASCII armor for signature, and fall back to GPGME decoding when seeing BAD_FORMAT error. r=mkmelin [already in tb-128]

[Approval Request Comment]
Regression caused by (bug #): no
User impact if declined: some users of optional gnupg configuration may send corrupted messages, or may have received corrupted messages
Testing completed (on c-c, etc.): only manually, because I couldn't reproduce the bug
Risk to taking this patch (and alternatives if risky): very low, the patch tries to explicitly avoid the specific corruption by explicitly requesting a specific gnupg mode (which was previously only assumed implicitly, and which some configurations might not have used), and the change is restricted to users who have deliberately enabled the external gnupg configuration

Comment 7

[Corey Bryant]

Comment on attachment 9403916 [details]
Bug 1898832 - With external GnuPG config, never use inner ASCII armor for signature, and fall back to GPGME decoding when seeing BAD_FORMAT error. r=mkmelin [already in tb-128]

[Triage Comment]
Approved for beta

Comment 8

[Daniel Darnell [:dandarnell]]

Thunderbird 128.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/53bf1c7c1329

Comment 9

[Kai Engert [:KaiE:]]

Because the issue isn't fixed yet, and I have a new idea, and because we're still in the same release cycle, I'm reopening this bug.

Comment 10

[Kai Engert [:KaiE:]]

So despite the earlier patch to explicitly request binary output, people still claim we have this bug.
And the only reason for the bug (that I can think of) is that GnuPG still returns us ASCII armored data in some circumstances.

I have a patch that will add more defensive logic.
Let's check the data that we receive from GPGME signing, and if it's ASCII armored, then decode it, prior to processing it further.

Comment 11

[Kai Engert [:KaiE:]]

Attachment: Bug 1898832 - Convert GPGME signing output from ASCII armor to binary, if necessary. r=mkmelin

Comment 12

[Kai Engert [:KaiE:]]

I have an experimental build, based on latest Thunderbird 115.x, with both patches added.

Windows 64 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/b4SG4SwiSYubXCcv0heVVg/runs/0/artifacts/public/build/target.zip

Linux 64 bit:
https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/RR3BZQAySD2QlGlK9pd_lw/runs/0/artifacts/public/build/target.tar.bz2

Comment 13

[snqk]

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

So despite the earlier patch to explicitly request binary output, people still claim we have this bug.
And the only reason for the bug (that I can think of) is that GnuPG still returns us ASCII armored data in some circumstances.

I have a patch that will add more defensive logic.
Let's check the data that we receive from GPGME signing, and if it's ASCII armored, then decode it, prior to processing it further.

I believe in my case setting the armor keyword in gpg.conf took precedence. Unsure why gpg ignores explicit instructions, but it would make sense to account for this. I'm running TBv115.12.0 (64-bit).

Comment 14

[Kai Engert [:KaiE:]]

Thanks for confirming.

This means the additional proposed patch should fix the issue.

Comment 15

[Kai Engert [:KaiE:]]

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

This means the additional proposed patch should fix the issue.

Confirmed also using the 115 test build by the original reporter of this issue.

Comment 16

[Kai Engert [:KaiE:]]

Attachment: 1898832-part2-backport-esr115.patch

Comment 17

[Kai Engert [:KaiE:]]

Magnus, can you please review the additional patch?

Comment 18

[Kai Engert [:KaiE:]]

We're getting reports of this issue repeatedly. Also bug 1906903.

While we have a fix in hand to avoid creating this broken encoding, we might unfortunately have plenty of emails out there are people have already received and that are unreadable.

The workaround for decrypting, which I had added with the first patch from this bug, only works if the recipient has external GnuPG configured. This doesn't help the majority of Thunderbird users, who are not using GnuPG themselves, but who received an encrypted message from another Thunderbird+GnuPG user.

Comment 19

[Kai Engert [:KaiE:]]

Because of comment 18, I think it's a high priority to try to implement a way to process this broken encoding with our internal RNP engine.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/163c80476e09
Convert GPGME signing output from ASCII armor to binary, if necessary. r=mkmelin

Comment 21

[Kai Engert [:KaiE:]]

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

Because of comment 18, I think it's a high priority to try to implement a way to process this broken encoding with our internal RNP engine.

I've been thinking about this more, and also made experiments.

I found no way to use RNP code as is to process these corrupted (badly encoded) messages.
Processing them would require modifications to the RNP code, and I think a mistake made at the Thunderbird code level cannot justify an RNP change to process badly, nonstandard-encoded data.

However, I'm able to process the bad messages using GnuPG, because it offers an "unwrap" API, and I have a working patch for that.

I think it can be argued, that offering a repair with GnuPG is useful and sufficient. Users who used the TB+GnuPG configuration will have broken messages in their Sent folder. Potentially, this might be the only place where those users have recorded the information that was sent out. Offering those affected users a way to access that data is useful.

However, for everyone else, who has received such messages, it seems acceptable to simply reject (and not be able to decrypt) those messages. The sender has produced bad data, and the recipients shouldn't be obliged to handle them. The recipient can immediately complain to the sender about the inability to decrypt. And future versions of Thunderbird will no longer produce such messages.

If there are recipients who really need to access the received broken messages, there is a manual workaround available. The message can be saved to disk, and then the user shall use the "gpg --unwrap" command and save the result to a file. Afterwards, the user can use "gpg --decrypt > msg.eml" to decode the message. The file msg.eml can then be opened with Thunderbird and viewed, allowing to view complex MIME message with their attachments and formatting.

So, I'll attach an additional patch, which allows TB+GnuPG users to decrypt those broken messages.
I'll do that in a separate bug.

Comment 22

[Kai Engert [:KaiE:]]

Comment on attachment 9410747 [details] [diff] [review]
1898832-part2-backport-esr115.patch

I'll have to create an updated backported patch for 115.

Comment 23

[Kai Engert [:KaiE:]]

Comment on attachment 9410215 [details]
Bug 1898832 - Convert GPGME signing output from ASCII armor to binary, if necessary. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): 1688863
User impact if declined: users of external gnupg may produce corrupted email messages that recipients cannot open
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): low, risk is limited to users of a non-standard configuration

Comment 24

[Kai Engert [:KaiE:]]

BTW, this bug was introduced in bug 1688863 in version 115.

Comment 25

[Corey Bryant]

Comment on attachment 9410215 [details]
Bug 1898832 - Convert GPGME signing output from ASCII armor to binary, if necessary. r=mkmelin

[Triage Comment]
Approved for beta
Note: There are two patches for this bug and the former was picked up in 128.0b3.

Comment 26

[Daniel Darnell [:dandarnell]]

Thunderbird 129.0b4:
https://hg.mozilla.org/releases/comm-beta/rev/cc5c79962020 (backed out)
https://hg.mozilla.org/releases/comm-beta/rev/78f5cec943f5

Comment 27

[Kai Engert [:KaiE:]]

Daniel, Rob, can you please keep this bug on your radar?
We'll eventually want to uplift this bug to esr128, which already have the first patch.

Your automated lists might not show this bug, even when it gets an approval for esr128.

Comment 28

[Kai Engert [:KaiE:]]

Attachment: 1898832-part2-version2-backport-esr115.patch

Update version of second patch, backported to esr115

Comment 29

[Kai Engert [:KaiE:]]

It would be good if we used a shortened test period.
I'd like to see this pushed out to 128.x soon, to avoid that more bad emails are produced.

Shortly after 128.x is out with that fix, I'd appreciate shipping in 115.

Given that this change is limited to users of the nonstandard gnupg configuration, I think the risk of a shorter test period is acceptable.

Comment 30

[Kai Engert [:KaiE:]]

While bug 1906903 is less urgent, it would be confusing if we delivered the fixes at different times.
I'd suggest to ship both at the same time.

Comment 31

[Corey Bryant]

It sounds reasonable to me to pick this up in 128 soon and 115 after.

Kai, do you want to add the corresponding uplift flags and details?

Comment 32

[Kai Engert [:KaiE:]]

Comment on attachment 9410215 [details]
Bug 1898832 - Convert GPGME signing output from ASCII armor to binary, if necessary. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #): 1688863
User impact if declined: users of external gnupg may produce corrupted email messages that recipients cannot open
Testing completed (on c-c, etc.):
Risk to taking this patch (and alternatives if risky): low, risk is limited to users of a non-standard configuration

Comment 33

[Wayne Mery (:wsmwk)]

Comment on attachment 9410215 [details]
Bug 1898832 - Convert GPGME signing output from ASCII armor to binary, if necessary. r=mkmelin

[Triage Comment]
Approved for esr128

Comment 34

[Rob Lemley [:rjl]]

Thunderbird 128.1.0esr:
https://hg.mozilla.org/releases/comm-esr128/rev/d27380dd6fda

Comment 35

[Corey Bryant]

Kai, is the README.md update needed in 1898832-backport-esr115.patch?

Comment 36

[Kai Engert [:KaiE:]]

(In reply to Corey Bryant from comment #35)

Kai, is the README.md update needed in 1898832-backport-esr115.patch?

no sorry, please remove.

I had added this when I needed to force a rebuild on treeherder

Comment 37

[Corey Bryant]

Comment on attachment 9403944 [details] [diff] [review]
1898832-backport-esr115.patch

[Triage Comment]
Approved for esr115

Rob, There are 2 patches here. And can you revert the README.md change when you uplift? Thanks!

Comment 38

[Corey Bryant]

Comment on attachment 9414194 [details] [diff] [review]
1898832-part2-version2-backport-esr115.patch

[Triage Comment]
Approved for esr115

Comment 39

[Rob Lemley [:rjl]]

Thunderbird 115.14.0:
https://hg.mozilla.org/releases/comm-esr115/rev/16b75a5e0b09
https://hg.mozilla.org/releases/comm-esr115/rev/41d40d325101